https://bugzilla.redhat.com/show_bug.cgi?id=2126943
Bug ID: 2126943 Summary: Review Request: perl-Template-Plugin-CGI - Simple Template Toolkit plugin interfacing to the CGI.pm module Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: dwrobel@ertelnet.rybnik.pl QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/perl-Template-Plugi... SRPM URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/perl-Template-Plugi... Description: Simple Template Toolkit plugin interfacing to the CGI.pm module Fedora Account System Username: dwrobel
Fedora Review Log: https://download.copr.fedorainfracloud.org/results/dwrobel/perl-Template-Plu...
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
Damian Wrobel dwrobel@ertelnet.rybnik.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2124543
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2124543 [Bug 2124543] perl-SOAP-WSDL-3.004-11.fc38 FTBFS: Can't locate CGI.pm in @INC at t/SOAP/WSDL/Server/Simple.t line 6
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value Assignee|nobody@fedoraproject.org |ppisar@redhat.com Status|NEW |ASSIGNED Flags| |fedora-review? CC| |ppisar@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #1 from Petr Pisar ppisar@redhat.com --- URL and Source0 addresses are usable. Ok. Source0 archive (SHA512: 2ca0a9f0a72a2c26e73a5e7202ae3773349c032acc0c15b016a84e629fe326ac1fd53f45f0ffa6d23311bc1fa7f7ac1645df187373c59f5c6b1046985655d1cd) is original. Ok. Summary verified from lib/Template/Plugin/CGI.pm. Ok. TODO: I recommed spelling "the CGI.pm module" without the file name extension as "the CGI module".
Description verified from lib/Template/Plugin/CGI.pm. Ok.
FIX: The License is incomplete. Licenses found in the archive are:
lib/Template/Plugin/CGI.pm:11: GPL-1.0-or-later OR Artistic-1.0-Perl /Tlibemplate/Plugin/CGI.pm:126: GPL-1.0-or-later OR Artistic-1.0-Perl lib/Template/Plugin/CGI.pm:141: X11 Makefile.PL:16: An unspecified MIT variant LICENSE: MIT text.
The license tag must comprehend all the licenses.
The first two occurrences (GPL-1.0-or-later OR Artistic-1.0-Perl) must be added. Reading the POD, one can see doubled NAME and other sections. That's probably a copy-and-paste mistake, but we need to respect all the licenses.
The last three licenses are probably a sloppy expression for a single MIT license https://spdx.org/licenses/MIT.html and author did not mean X11 variant of MIT license https://spdx.org/licenses/X11.html as suggested at lib/Template/Plugin/CGI.pm:141.
Please change the License tag to "(GPL-1.0-or-later OR Artistic-1.0-Perl) AND MIT" and contact upstream to clarify and preferably correct the mistakes.
No XS code, noArch BuildArch is Ok.
TODO: Qualify 'perl(ExtUtils::MakeMaker)' with >= '6.76' because of Makefile.PL arguments NO_PACKLIST=1 NO_PERLLOCAL=1. FIX: Build-require 'perl(CGI) >= 4.44' (lib/Template/Plugin/CGI.pm:26, META.json:32). FIX: Build-require 'perl(strict)' (Makefile.PL:2). FIX: Build-require 'perl(warnings)' (Makefile.PL:3). FIX: Build-require 'perl(base)' (lib/Template/Plugin/CGI.pm:25). FIX: Build-require 'perl(lib)' (t/cgi.t:20). FIX: Build-require 'perl(Template) >= 3.100' (t/cgi.t:21). FIX: Build-require 'perl(Template::Test)' (t/cgi.t:22).
FIX: Remove unused dependencies: perl(Carp), perl(CGI::Cookie), perl(CGI::File::Temp), perl(CGI::Util), perl(constant), perl(Cwd), perl(Encode*, perl(Exporter), perl(Exporter::Heavy), perl(File::Path), perl(File::Spec::Unix), perl(File::Temp), perl(HTML::Entities), perl(HTML::Parser), perl(List::Util), perl(parent), perl(Scalar::Util), perl(Socket), perl(Storable), perl(Template::Base), perl(Template::Constants), perl(Test2::Bundle::Extended), perl(Test2::Plugin::NoWarnings), perl(Test2::Suite), perl(Test2::Tools::Explain), perl(Test::Builder), perl(Test::CPAN::Meta), perl(Term::Cap), perl(Text::ParseWords). Where did you get these dependencies from? I cannot see them used anywhere in the code.
TODO: Unset AUTHOR_TESTING environment variable %check section to make the tests more reproducible (t/author-pod-syntax.t:4).
FIX: The packeged module was part for perl-Template-Toolkit. You need to add "Conflicts: perl-Template-Toolkit < 3.010-5" to prevent from installing both of them at the same time.
Please correct the FIX items, consider fixing TODO items, and provide an updated spec file.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #2 from Damian Wrobel dwrobel@ertelnet.rybnik.pl --- (In reply to Petr Pisar from comment #1)
Thank you for taking the review and for your valuable comments. I'll fix the issues.
FIX: Remove unused dependencies: perl(Carp), perl(CGI::Cookie),
Where did you get these dependencies from? I cannot see them used anywhere in the code.
It's a sum (excluding duplicates) from the following two tools: $ scandeps.pl lib/Template/Plugin/CGI.pm $ cpanspec Template-Plugin-CGI-3.101.tar.gz
I'm not an expert in perl, but I liked the output in the form of:
perl(CGI::Cookie) perl(CGI::File::Temp) perl(CGI::Util)
instead of using
perl(CGI)
alone just to avoid a situation which we have in bug#2124543#c3.
My assumption is (maybe I'm wrong) that if at some point in time someone will decide to spin off or move e.g CGI:Util to a separate package then this would be transparent to me (it might still require to package CGI::Util, though).
So, is it acceptable to use this "more granular" form of "Requires:" or I have to use just the top module name (in the above example perl(CGI))?
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #3 from Petr Pisar ppisar@redhat.com --- Your are completely right: If a Perl module requires CGI::Cookie, then an RPM package which delivers the module should require perl(CGI::Cookie). Not perl(CGI).
However, in Template-Plugin-CGI-3.101 I cannot see anywhere e.g. CGI::Cookie to be mentioned. Neither "grep -Hnr CGI::Cookie" command shows it, neither "cpanspec Template-Plugin-CGI-3.101.tar.gz" generates it.
I did not know scandeps.pl tool, but I think tool is broken or I cannot use it properly. First you want to run it with -R, otherwise it reports dependencies of dependencies. (Which is why your original spec file contains to much modules. RPM packages should not list transitive dependencies in general). Second you want to call it with -B, otherwise it hides core modules. Third it hides uninstalled direct dependencies, it probably only does dynamic analysis from %INC hash after loading the file or it does not account that current perl does not have ./ in @INC. Dynamic analysis is not bad, but we also need static analysis because Perl can load modules only in a specific code path (e.g. from a called subroutine.) See:
$ scandeps.pl -V -B -R ./lib/Template/Plugin/CGI.pm # Could not find source file 'Template/Plugin/CGI.pm' in @INC or @IncludeLibs. Skipping it. # Could not find source file 'Template/Plugin.pm' in @INC or @IncludeLibs. Skipping it. # Could not find source file 'CGI.pm' in @INC or @IncludeLibs. Skipping it. # Template/Plugin/CGI.pm [] # base.pm [module] # strict.pm [module] # warnings.pm [module] # # Legend: [C]ore [X]ternal [S]ubmodule [?]NotOnCPAN 'base' => '0', # C? # Template::Plugin::CGI 'strict' => '0', # C? # Template::Plugin::CGI 'warnings' => '0', # C? # Template::Plugin::CGI
It is missing Template::Plugin and CGI.
For scanning sources for Perl dependencies I use tangerine tool (installed from a same-named RPM package).
That's why I think the dependency I listed should not be there.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #4 from Damian Wrobel dwrobel@ertelnet.rybnik.pl --- Here is what I see:
[dw@dell Plugin]$ scandeps.pl CGI.pm 'CGI::Cookie' => '4.54', 'CGI::File::Temp' => '4.54', 'CGI::Util' => '4.54', 'Carp' => '1.52', 'Cwd' => '3.80', 'Encode' => '3.17', 'Encode::Alias' => '2.24', 'Encode::Config' => '2.05', 'Encode::Encoding' => '2.08', 'Encode::MIME::Name' => '1.03', 'Exporter' => '5.76', 'Exporter::Heavy' => '5.76', 'Fh' => '4.54', 'File::Path' => '2.18', 'File::Spec' => '3.80', 'File::Spec::Unix' => '3.80', 'File::Temp' => '0.2311', 'HTML::Entities' => '3.78', 'HTML::Parser' => '3.78', 'List::Util' => '1.63', 'Scalar::Util' => '1.63', 'Socket' => '2.035', 'Storable' => '3.25', 'Template::Base' => '3.010', 'Template::Constants' => '3.010', 'Template::Plugin' => '3.010', 'Term::Cap' => '1.17', 'Text::ParseWords' => '3.31', 'constant' => '1.33', 'parent' => '0.238',
[dw@dell Plugin]$ rpm -qf /usr/bin/scandeps.pl perl-Module-ScanDeps-1.31-6.fc36.noarch
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #5 from Damian Wrobel dwrobel@ertelnet.rybnik.pl --- For the reverse direction I'm getting:
[dw@dell Plugin]$ scandeps.pl -V -B -R CGI.pm # Template/Plugin.pm [module] # Template/Plugin/CGI.pm [module] # base.pm [module] # strict.pm [module] # warnings.pm [module] # # Legend: [C]ore [X]ternal [S]ubmodule [?]NotOnCPAN 'Template::Plugin' => '0', # ? # Template::Plugin::CGI 'base' => '0', # C? # Template::Plugin::CGI 'strict' => '0', # C? # Template::Plugin::CGI 'warnings' => '0', # C? # Template::Plugin::CGI
While cpanspec gives me:
[dw@dell cpan]$ cpanspec Template-Plugin-CGI-3.101.tar.gz [dw@dell cpan]$ grep '^BuildRequires:' perl-Template-Plugin-CGI.spec BuildRequires: perl(CGI) >= 4.44 BuildRequires: perl(ExtUtils::MakeMaker) BuildRequires: perl(File::Spec) BuildRequires: perl(File::Temp) BuildRequires: perl(IO::Handle) BuildRequires: perl(IPC::Open3) BuildRequires: perl(Template) >= 3.100 BuildRequires: perl(Test2::Bundle::Extended) BuildRequires: perl(Test2::Plugin::NoWarnings) BuildRequires: perl(Test2::Suite) BuildRequires: perl(Test2::Tools::Explain) BuildRequires: perl(Test::Builder) BuildRequires: perl(Test::CPAN::Meta) BuildRequires: perl(Test::More)
Is there any authoritative method of automatically retriving list of BRs for a perl module a I could use?
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #6 from Damian Wrobel dwrobel@ertelnet.rybnik.pl --- (In reply to Petr Pisar from comment #1)
I'm in a process of fixing the issue and have the following doubts:
FIX: Build-require 'perl(Template) >= 3.100' (t/cgi.t:21).
This doesn't exist. dnf builddeps comlains:
No matching package to install: 'perl(Template) >= 3.100'
FIX: Remove unused dependencies: perl(Carp), perl(CGI::Cookie), perl(Template::Constants), perl(Test2::Bundle::Extended), perl(Test2::Plugin::NoWarnings), perl(Test2::Suite), perl(Test2::Tools::Explain), perl(Test::Builder), perl(Test::CPAN::Meta), perl(Term::Cap), perl(Text::ParseWords). Where did you get these dependencies from? I cannot see them used anywhere in the code.
Test seems to require the following: # === Test Requires === # # Module Want Have # ------------------------- ----- -------- # ExtUtils::MakeMaker any 7.64 # File::Spec any 3.84 # File::Temp any 0.2311 # IO::Handle any 1.48 # IPC::Open3 any 1.22 # Template 3.100 3.101 # Test2::Bundle::Extended any missing <= you recommended to remove it # Test2::Plugin::NoWarnings any missing <= you recommended to remove it # Test2::Suite any missing <= you recommended to remove it # Test2::Tools::Explain any missing <= you recommended to remove it # Test::Builder any 1.302191 # Test::CPAN::Meta any missing <= you recommended to remove it # Test::More any 1.302191
So, It's probably good to keep them.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #7 from Damian Wrobel dwrobel@ertelnet.rybnik.pl --- (In reply to Petr Pisar from comment #1)
URL and Source0 addresses are usable. Ok. Source0 archive (SHA512: 2ca0a9f0a72a2c26e73a5e7202ae3773349c032acc0c15b016a84e629fe326ac1fd53f45f0ffa 6d23311bc1fa7f7ac1645df187373c59f5c6b1046985655d1cd) is original. Ok. Summary verified from lib/Template/Plugin/CGI.pm. Ok. TODO: I recommed spelling "the CGI.pm module" without the file name
Done.
extension as "the CGI module".
Description verified from lib/Template/Plugin/CGI.pm. Ok.
FIX: The License is incomplete. Licenses found in the archive are:
Done.
lib/Template/Plugin/CGI.pm:11: GPL-1.0-or-later OR Artistic-1.0-Perl /Tlibemplate/Plugin/CGI.pm:126: GPL-1.0-or-later OR Artistic-1.0-Perl lib/Template/Plugin/CGI.pm:141: X11 Makefile.PL:16: An unspecified MIT variant LICENSE: MIT text.
The license tag must comprehend all the licenses.
The first two occurrences (GPL-1.0-or-later OR Artistic-1.0-Perl) must be added. Reading the POD, one can see doubled NAME and other sections. That's probably a copy-and-paste mistake, but we need to respect all the licenses.
The last three licenses are probably a sloppy expression for a single MIT license https://spdx.org/licenses/MIT.html and author did not mean X11 variant of MIT license https://spdx.org/licenses/X11.html as suggested at lib/Template/Plugin/CGI.pm:141.
Please change the License tag to "(GPL-1.0-or-later OR Artistic-1.0-Perl) AND MIT" and contact upstream to clarify and preferably correct the mistakes.
No XS code, noArch BuildArch is Ok.
TODO: Qualify 'perl(ExtUtils::MakeMaker)' with >= '6.76' because of
Done.
Makefile.PL arguments NO_PACKLIST=1 NO_PERLLOCAL=1. FIX: Build-require 'perl(CGI) >= 4.44' (lib/Template/Plugin/CGI.pm:26, META.json:32). FIX: Build-require 'perl(strict)' (Makefile.PL:2). FIX: Build-require 'perl(warnings)' (Makefile.PL:3). FIX: Build-require 'perl(base)' (lib/Template/Plugin/CGI.pm:25). FIX: Build-require 'perl(lib)' (t/cgi.t:20). FIX: Build-require 'perl(Template) >= 3.100' (t/cgi.t:21). FIX: Build-require 'perl(Template::Test)' (t/cgi.t:22).
FIX: Remove unused dependencies: perl(Carp), perl(CGI::Cookie), perl(CGI::File::Temp), perl(CGI::Util), perl(constant), perl(Cwd), perl(Encode*, perl(Exporter), perl(Exporter::Heavy), perl(File::Path), perl(File::Spec::Unix), perl(File::Temp), perl(HTML::Entities), perl(HTML::Parser), perl(List::Util), perl(parent), perl(Scalar::Util), perl(Socket), perl(Storable), perl(Template::Base), perl(Template::Constants), perl(Test2::Bundle::Extended), perl(Test2::Plugin::NoWarnings), perl(Test2::Suite), perl(Test2::Tools::Explain), perl(Test::Builder), perl(Test::CPAN::Meta), perl(Term::Cap), perl(Text::ParseWords). Where did you get these dependencies from? I cannot see them used anywhere in the code.
Done. With the exceptions aforementioned in #c6
TODO: Unset AUTHOR_TESTING environment variable %check section to make the tests more reproducible (t/author-pod-syntax.t:4).
Done.
FIX: The packeged module was part for perl-Template-Toolkit. You need to add "Conflicts: perl-Template-Toolkit < 3.010-5" to prevent from installing both of them at the same time.
Done.
Please correct the FIX items, consider fixing TODO items, and provide an updated spec file.
Please find updated files: Spec URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/perl-Template-Plugi... SRPM URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/perl-Template-Plugi...
Fedora Review Log: https://download.copr.fedorainfracloud.org/results/dwrobel/perl-Template-Plu...
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=92042450
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #8 from Petr Pisar ppisar@redhat.com --- (In reply to Damian Wrobel from comment #4)
Here is what I see:
[dw@dell Plugin]$ scandeps.pl CGI.pm 'CGI::Cookie' => '4.54', 'CGI::File::Temp' => '4.54', 'CGI::Util' => '4.54', 'Carp' => '1.52',
[...] That's because you already have the dependencies installed. In other words, to use scandeps.pl to report dependencies, you first need to install the dependencies, and then the tool reports them. This is not helpful for discovering new dependencies. That's only good for confirming already known dependencies. E.g. if you uninstall 'perl(CGI::Cookie)', it will disappear from the from the list. Moreover it mixes direct and indirect dependencies (= dependencies of dependencies). We are interested only in the direct ones.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #9 from Petr Pisar ppisar@redhat.com --- (In reply to Damian Wrobel from comment #5)
For the reverse direction I'm getting:
[dw@dell Plugin]$ scandeps.pl -V -B -R CGI.pm # Template/Plugin.pm [module] # Template/Plugin/CGI.pm [module] # base.pm [module] # strict.pm [module] # warnings.pm [module] # # Legend: [C]ore [X]ternal [S]ubmodule [?]NotOnCPAN 'Template::Plugin' => '0', # ? # Template::Plugin::CGI 'base' => '0', # C? # Template::Plugin::CGI 'strict' => '0', # C? # Template::Plugin::CGI 'warnings' => '0', # C? # Template::Plugin::CGI
Yet if you look at CGI.pm, line 26:
19 package Template::Plugin::CGI; 20 our $AUTHORITY = 'cpan:TODDR'; 21 # ABSTRACT: Simple Template Toolkit plugin interfacing to the CGI.pm module 22 $Template::Plugin::CGI::VERSION = '3.101'; 23 use strict; 24 use warnings; 25 use base 'Template::Plugin'; → 26 use CGI;
There is a clear use of CGI module. And the tool misses it. I don't know why. On my system it at least discovers it and than hides it. That's why I wrote that the tool probably has bugs.
While cpanspec gives me:
[dw@dell cpan]$ cpanspec Template-Plugin-CGI-3.101.tar.gz [dw@dell cpan]$ grep '^BuildRequires:' perl-Template-Plugin-CGI.spec BuildRequires: perl(CGI) >= 4.44 BuildRequires: perl(ExtUtils::MakeMaker) BuildRequires: perl(File::Spec) BuildRequires: perl(File::Temp) BuildRequires: perl(IO::Handle) BuildRequires: perl(IPC::Open3) BuildRequires: perl(Template) >= 3.100 BuildRequires: perl(Test2::Bundle::Extended) BuildRequires: perl(Test2::Plugin::NoWarnings) BuildRequires: perl(Test2::Suite) BuildRequires: perl(Test2::Tools::Explain) BuildRequires: perl(Test::Builder) BuildRequires: perl(Test::CPAN::Meta) BuildRequires: perl(Test::More)
Because cpanspec does not read the Perl source code at all. It only looks into META.yaml (and maybe META.json) and spits out what's written there.
Is there any authoritative method of automatically retriving list of BRs for a perl module a I could use?
It isn't. That's an inherent property of all Turing machine-complete programming languages. You can only have tools which provide approximate results and then you need a packager who use a common sense and experience to judge the results.
The dynamic analysis scandeps does is actually pretty good, but to use is seriously, one would need to instrument it more and apply on different pieces of code (e.g. "perl Makefile.PL" and "make test"). In the past I started developing a tool like that but I stopped for two reasons: First perl interpreter's introspection facilities for loading modules is buggy and differs from a perl version to version. It was impossible to make it reliable and portable. Second reason was that at some level the introspection itself influences perl interpreter and the analyzed code. It resulted into a garbage output, or the analyzed code diverted from it's original code path, or the analysis hit Perl's recursion limit and crashed or never halted.
My recommended way is reading metadata provided by upstream (META.json, META.yaml, or Makefile.PL, or dist.ini). Then doing a static analysis with tangerine tool. But you cannot blindly trust them. If it is feasible, it's good to verify a usage of the declared/discovered dependencies by grepping, and reading the source code. Also rewriting the module identifier in the source code and then verifying that Makefile.PL && make test fails is sometimes necessary. Also optional dependencies, especially in tests can be tricky. Sometimes they do not do anything helpful (like pretty-formatting a debugging output, or reporting module versions, or criticizing a code style) and it's better to omit them.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #10 from Petr Pisar ppisar@redhat.com --- (In reply to Damian Wrobel from comment #6)
(In reply to Petr Pisar from comment #1)
I'm in a process of fixing the issue and have the following doubts:
FIX: Build-require 'perl(Template) >= 3.100' (t/cgi.t:21).
This doesn't exist. dnf builddeps comlains:
No matching package to install: 'perl(Template) >= 3.100'
It only exists in Fedora 38:
# dnf --quiet --enablerepo=f38-build repoquery --whatprovides 'perl(Template)
= 3.100'
perl-Template-Toolkit-0:3.101-1.fc38.x86_64
FIX: Remove unused dependencies: perl(Carp), perl(CGI::Cookie), perl(Template::Constants), perl(Test2::Bundle::Extended), perl(Test2::Plugin::NoWarnings), perl(Test2::Suite), perl(Test2::Tools::Explain), perl(Test::Builder), perl(Test::CPAN::Meta), perl(Term::Cap), perl(Text::ParseWords). Where did you get these dependencies from? I cannot see them used anywhere in the code.
Test seems to require the following: # === Test Requires === # # Module Want Have # ------------------------- ----- -------- # ExtUtils::MakeMaker any 7.64 # File::Spec any 3.84 # File::Temp any 0.2311 # IO::Handle any 1.48 # IPC::Open3 any 1.22 # Template 3.100 3.101 # Test2::Bundle::Extended any missing <= you recommended to remove it # Test2::Plugin::NoWarnings any missing <= you recommended to remove it # Test2::Suite any missing <= you recommended to remove it # Test2::Tools::Explain any missing <= you recommended to remove it # Test::Builder any 1.302191 # Test::CPAN::Meta any missing <= you recommended to remove it # Test::More any 1.302191
So, It's probably good to keep them.
I think keeping them is not good. The modules are not used for anything except of reporting their versions. E.g. look at Test2::Plugin::NoWarnings:
$ grep -Hnr Test2::Plugin::NoWarnings cpanfile.plugins:11: requires "Test2::Plugin::NoWarnings" => 0; Makefile.PL:29: "Test2::Plugin::NoWarnings" => 0, Makefile.PL:52: "Test2::Plugin::NoWarnings" => 0, META.json:47: "Test2::Plugin::NoWarnings" : "0", META.yml:13: Test2::Plugin::NoWarnings: '0' cpanfile:11: requires "Test2::Plugin::NoWarnings" => 0; t/00-report-prereqs.dd:32: 'Test2::Plugin::NoWarnings' => '0',
The first 6 locations are only a declaration of the dependency. A metadata. No Code. The last and the only location is an input for 00-report-prereqs.t which loads the module only in order to print its version. Therefore I conclude that it's better to remove them.
Theoretically it could be a plugin into tests whose mere presence causes its load and use. But looking at its documentation https://metacpan.org/pod/Test2::Plugin::NoWarnings#SYNOPSIS reveals it's not that case. To use it you need to explicitly load it in a test. And as you can see in the grep output, the module is never loaded.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #11 from Petr Pisar ppisar@redhat.com --- Now reviewing the second release of the spec file.
TODO: You can use a plain 'perl' command instead of macro '%{__perl}' (perl-Template-Plugin-CGI.spec:32, perl-Template-Plugin-CGI.spec:47).
FIX: Build-require 'perl(File::Spec)' (t/00-compile.t:23). FIX: Build-require 'perl(Template) >= 3.100' (t/cgi.t:21). It seems you forgot to add dependencies for the tests.
TODO: Do not build-require perl(Test2::Bundle::Extended), perl(Test2::Plugin::NoWarnings), perl(Test2::Suite), perl(Test2::Tools::Explain), perl(Test::Builder). They are probably copy-paste leftovers from separating the module by upstream. As I showed in my previous comment, depending on them does not bring any benefit. It only make a dependency tree of this packages larger and more prone to build failures (if the dependent package breaks). This in an example where upstream did a mistake.
FIX: Do not build-require perl(Test::CPAN::Meta). It's not used because a test never reaching that place (t/author-distmeta.t:12). This is the same case as for Test::NoTabs.
All tests pass. Ok.
$ rpmlint perl-Template-Plugin-CGI.spec ../SRPMS/perl-Template-Plugin-CGI-3.101-2.fc38.src.rpm ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm ========================================================== rpmlint session starts ========================================================= rpmlint: 2.2.0 configuration: /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 3
perl-Template-Plugin-CGI.noarch: W: invalid-license GPL-1.0-or-later perl-Template-Plugin-CGI.noarch: W: invalid-license Artistic-1.0-Perl perl-Template-Plugin-CGI.src: W: invalid-license GPL-1.0-or-later perl-Template-Plugin-CGI.src: W: invalid-license Artistic-1.0-Perl =========================== 2 packages and 1 specfiles checked; 0 errors, 4 warnings, 0 badness; has taken 0.5 s ========================== rpmlint output is Ok.
$ rpm -q -lv -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm drwxr-xr-x 2 root root 0 Sep 16 11:13 /usr/share/doc/perl-Template-Plugin-CGI -rw-r--r-- 1 root root 703 Apr 27 18:27 /usr/share/doc/perl-Template-Plugin-CGI/Changes drwxr-xr-x 2 root root 0 Sep 16 11:13 /usr/share/licenses/perl-Template-Plugin-CGI -rw-r--r-- 1 root root 1069 Apr 27 18:27 /usr/share/licenses/perl-Template-Plugin-CGI/LICENSE -r--r--r-- 1 root root 2062 Sep 16 11:13 /usr/share/man/man3/Template::Plugin::CGI.3pm.gz drwxr-xr-x 2 root root 0 Sep 16 11:13 /usr/share/perl5/vendor_perl/Template drwxr-xr-x 2 root root 0 Sep 16 11:13 /usr/share/perl5/vendor_perl/Template/Plugin -r--r--r-- 1 root root 3549 Apr 27 18:27 /usr/share/perl5/vendor_perl/Template/Plugin/CGI.pm File layout and permissions are Ok.
$ rpm -q --requires -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm | sort -f | uniq -c 1 perl(:MODULE_COMPAT_5.36.0) 1 perl(base) 1 perl(CGI) 1 perl(strict) 1 perl(Template::Plugin) 1 perl(warnings) 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsZstd) <= 5.4.18-1 TODO: Constrain 'perl(CGI)' run-time dependency with '>= 4.44' version (META.json:32). This can be done either by adding an explicit 'Requires: perl(CGI) >= 4.44' line to the spec file or patching a source code to 'use CGI 4.44;' at lib/Template/Plugin/CGI.pm:26.
$ rpm -q --provides -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm | sort -f | uniq -c 1 perl(Template::Plugin::CGI) = 3.101 1 perl-Template-Plugin-CGI = 3.101-2.fc38 Binary provides are Ok.
$ rpm -q --conflicts -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm | sort -f | uniq -c 1 perl-Template-Toolkit < 3.010-5 Binary conflicts are Ok.
$ resolvedeps rawhide ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm Binary dependencies are resolvable. Ok.
The package builds in Fedora 38 (https://koji.fedoraproject.org/koji/taskinfo?taskID=92071243). Ok.
Otherwise, the package is in line with Fedora and Perl packaging guidelines. Please correct the FIX items, consider correcting TODO items, and provide an updates spec file.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #12 from Damian Wrobel dwrobel@ertelnet.rybnik.pl --- (In reply to Petr Pisar from comment #11)
Now reviewing the second release of the spec file.
TODO: You can use a plain 'perl' command instead of macro '%{__perl}' (perl-Template-Plugin-CGI.spec:32, perl-Template-Plugin-CGI.spec:47).
Done.
FIX: Build-require 'perl(File::Spec)' (t/00-compile.t:23). FIX: Build-require 'perl(Template) >= 3.100' (t/cgi.t:21). It seems you forgot to add dependencies for the tests.
Done.
TODO: Do not build-require perl(Test2::Bundle::Extended), perl(Test2::Plugin::NoWarnings), perl(Test2::Suite), perl(Test2::Tools::Explain), perl(Test::Builder). They are probably copy-paste leftovers from separating the module by upstream. As I showed in my previous comment, depending on them does not bring any benefit. It only make a dependency tree of this packages larger and more prone to build failures (if the dependent package breaks). This in an example where upstream did a mistake.
Done.
FIX: Do not build-require perl(Test::CPAN::Meta). It's not used because a test never reaching that place (t/author-distmeta.t:12). This is the same case as for Test::NoTabs.
Done.
All tests pass. Ok.
$ rpmlint perl-Template-Plugin-CGI.spec ../SRPMS/perl-Template-Plugin-CGI-3.101-2.fc38.src.rpm ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm ========================================================== rpmlint session starts ========================================================= rpmlint: 2.2.0 configuration: /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 3
perl-Template-Plugin-CGI.noarch: W: invalid-license GPL-1.0-or-later perl-Template-Plugin-CGI.noarch: W: invalid-license Artistic-1.0-Perl perl-Template-Plugin-CGI.src: W: invalid-license GPL-1.0-or-later perl-Template-Plugin-CGI.src: W: invalid-license Artistic-1.0-Perl =========================== 2 packages and 1 specfiles checked; 0 errors, 4 warnings, 0 badness; has taken 0.5 s ========================== rpmlint output is Ok.
$ rpm -q -lv -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm drwxr-xr-x 2 root root 0 Sep 16 11:13 /usr/share/doc/perl-Template-Plugin-CGI -rw-r--r-- 1 root root 703 Apr 27 18:27 /usr/share/doc/perl-Template-Plugin-CGI/Changes drwxr-xr-x 2 root root 0 Sep 16 11:13 /usr/share/licenses/perl-Template-Plugin-CGI -rw-r--r-- 1 root root 1069 Apr 27 18:27 /usr/share/licenses/perl-Template-Plugin-CGI/LICENSE -r--r--r-- 1 root root 2062 Sep 16 11:13 /usr/share/man/man3/Template::Plugin::CGI.3pm.gz drwxr-xr-x 2 root root 0 Sep 16 11:13 /usr/share/perl5/vendor_perl/Template drwxr-xr-x 2 root root 0 Sep 16 11:13 /usr/share/perl5/vendor_perl/Template/Plugin -r--r--r-- 1 root root 3549 Apr 27 18:27 /usr/share/perl5/vendor_perl/Template/Plugin/CGI.pm File layout and permissions are Ok.
$ rpm -q --requires -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm | sort -f | uniq -c 1 perl(:MODULE_COMPAT_5.36.0) 1 perl(base) 1 perl(CGI) 1 perl(strict) 1 perl(Template::Plugin) 1 perl(warnings) 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsZstd) <= 5.4.18-1 TODO: Constrain 'perl(CGI)' run-time dependency with '>= 4.44' version (META.json:32). This can be done either by adding an explicit 'Requires: perl(CGI) >= 4.44' line to the spec file or patching a source code to 'use CGI 4.44;' at lib/Template/Plugin/CGI.pm:26.
Done (in .spec file).
$ rpm -q --provides -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm | sort -f | uniq -c 1 perl(Template::Plugin::CGI) = 3.101 1 perl-Template-Plugin-CGI = 3.101-2.fc38 Binary provides are Ok.
$ rpm -q --conflicts -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm | sort -f | uniq -c 1 perl-Template-Toolkit < 3.010-5 Binary conflicts are Ok.
$ resolvedeps rawhide ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-2.fc38.noarch.rpm Binary dependencies are resolvable. Ok.
The package builds in Fedora 38 (https://koji.fedoraproject.org/koji/taskinfo?taskID=92071243). Ok.
Otherwise, the package is in line with Fedora and Perl packaging guidelines. Please correct the FIX items, consider correcting TODO items, and provide an updates spec file.
Yesterday, I contacted upstream w.r.t. to clarify licensing tag.
Please find updated files: Spec URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/perl-Template-Plugi... SRPM URL: https://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/perl-Template-Plugi...
Fedora Review Log: https://download.copr.fedorainfracloud.org/results/dwrobel/perl-Template-Plu...
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=92076549
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #13 from Petr Pisar ppisar@redhat.com --- Thanks for polishing the package.
Changes in the spec file:
--- perl-Template-Plugin-CGI.spec.old 2022-09-15 19:42:47.000000000 +0200 +++ perl-Template-Plugin-CGI.spec 2022-09-16 14:02:03.000000000 +0200 @@ -1,7 +1,7 @@ Summary: Simple Template Toolkit plugin interfacing to the CGI module Name: perl-Template-Plugin-CGI Version: 3.101 -Release: 2%{?dist} +Release: 3%{?dist} License: (GPL-1.0-or-later OR Artistic-1.0-Perl) AND MIT URL: https://metacpan.org/release/Template-Plugin-CGI Source: https://cpan.metacpan.org/modules/by-module/Template/Template-Plugin-CGI-%%7... @@ -12,24 +12,21 @@ BuildRequires: perl(blib) BuildRequires: perl(CGI) >= 4.44 BuildRequires: perl(ExtUtils::MakeMaker) >= 6.76 +BuildRequires: perl(File::Spec) BuildRequires: perl-generators BuildRequires: perl-interpreter BuildRequires: perl(IO::Handle) BuildRequires: perl(IPC::Open3) BuildRequires: perl(lib) BuildRequires: perl(strict) +BuildRequires: perl(Template) >= 3.100 BuildRequires: perl(Template::Plugin) BuildRequires: perl(Template::Test) -BuildRequires: perl(Test2::Bundle::Extended) -BuildRequires: perl(Test2::Plugin::NoWarnings) -BuildRequires: perl(Test2::Suite) -BuildRequires: perl(Test2::Tools::Explain) -BuildRequires: perl(Test::Builder) -BuildRequires: perl(Test::CPAN::Meta) BuildRequires: perl(Test::More) BuildRequires: perl(warnings)
-Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version)) +Requires: perl(CGI) >= 4.44 +Requires: perl(:MODULE_COMPAT_%(eval "`perl -V:version`"; echo $version)) Conflicts: perl-Template-Toolkit < 3.010-5
%{?perl_default_filter} @@ -44,7 +41,7 @@
%build -%{__perl} Makefile.PL INSTALLDIRS=vendor NO_PACKLIST=1 NO_PERLLOCAL=1 +perl Makefile.PL INSTALLDIRS=vendor NO_PACKLIST=1 NO_PERLLOCAL=1 %make_build
@@ -65,6 +62,9 @@
%changelog +* Fri Sep 16 2022 Damian Wrobel dwrobel@ertelnet.rybnik.pl - 3.101-3 +- Address comments from review process + * Thu Sep 15 2022 Damian Wrobel dwrobel@ertelnet.rybnik.pl - 3.101-2 - Address comments from review process
$ rpmlint perl-Template-Plugin-CGI.spec ../SRPMS/perl-Template-Plugin-CGI-3.101-3.fc38.src.rpm ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-3.fc38.noarch.rpm ========================================================== rpmlint session starts ========================================================= rpmlint: 2.2.0 configuration: /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 3
perl-Template-Plugin-CGI.noarch: W: invalid-license GPL-1.0-or-later perl-Template-Plugin-CGI.noarch: W: invalid-license Artistic-1.0-Perl perl-Template-Plugin-CGI.src: W: invalid-license GPL-1.0-or-later perl-Template-Plugin-CGI.src: W: invalid-license Artistic-1.0-Perl =========================== 2 packages and 1 specfiles checked; 0 errors, 4 warnings, 0 badness; has taken 0.5 s ========================== rpmlint is Ok.
$ rpm -q --requires -p ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-3.fc38.noarch.rpm | sort -f | uniq -c 1 perl(:MODULE_COMPAT_5.36.0) 1 perl(base) 1 perl(CGI) 1 perl(CGI) >= 4.44 1 perl(strict) 1 perl(Template::Plugin) 1 perl(warnings) 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsZstd) <= 5.4.18-1 TODO: You can remove the underspecified, and now redundant, dependency on unversioned 'perl(CGI)' with this line placed after %{?perl_default_filter} line:
%global __requires_exclude %{?__requires_exclude:%{__requires_exclude}|}^perl\(CGI\)$
Removing underspecified dependencies helps to decrease a size of YUM repository metadata users needs to download on their systems.
$ resolvedeps rawhide ../RPMS/noarch/perl-Template-Plugin-CGI-3.101-3.fc38.noarch.rpm Binary dependencies are resolvable. Ok.
The package builds in Fedora 38 (https://koji.fedoraproject.org/koji/taskinfo?taskID=92077799). Ok.
Please consider correcting the TODO item before building this package. Resolution: Package APROVED.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #14 from Damian Wrobel dwrobel@ertelnet.rybnik.pl --- (In reply to Petr Pisar from comment #13)
Thanks for polishing the package.
Thank you for the review.
TODO: You can remove the underspecified, and now redundant, dependency on unversioned 'perl(CGI)' with this line placed after %{?perl_default_filter} line:
%global __requires_exclude %{?__requires_exclude:%{__requires_exclude}|}^perl\(CGI\)$
Removing underspecified dependencies helps to decrease a size of YUM repository metadata users needs to download on their systems.
While I agree that it make sense to minimize the size of the YUM metadata, shouldn't it be rather fixed in the rpm?
It seems to be quite generic issue: $ rpm -qa --requires | uniq -cd | wc -l 689
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #15 from Petr Pisar ppisar@redhat.com --- It would make sense and I asked for it RPM developers some time ago. (I cannot a link now.) But the idea was rejected with an explanation that there are actually two kinds of run-time dependencies.
One is RPM package-level dependencies specified explicitly in a spec file. Another is dependencies generated from the packaged files. The later dependencies coming from a file are tight to the file in the RPM package metadata. Why to distinguish the two? A catch is that when installing a package, an rpm tool can actually exclude some files from the installation (e.g. documentation or localization files). In that case a dependency tight to the uninstalled file will actually disappear from the installed package because the file simply is not installed. Hence RPM developers did not supported the idea of removing the less specific dependencies.
Instead they recommended augmenting the dependency generator which scans the files (perl-generators) and conveys the discovered file-level dependencies to an rpmbuild tool. Alas, this idea was never implemented. (A spec file would pass a list of more specific versions to the generator through positional arguments defined with a macro.) Hence we ended up with filtering the dependencies at build time.
Another approach could be minimizing the dependencies when gathering YUM metadata from the RPM packages. That could be doable. At the end, both metadata and DNF do not distinguish between the two kinds of dependencies. But I never got to requesting this feature to createrepo_c tool which generates YUM metadata.
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
--- Comment #16 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/perl-Template-Plugin-CGI
https://bugzilla.redhat.com/show_bug.cgi?id=2126943
Damian Wrobel dwrobel@ertelnet.rybnik.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |RAWHIDE Status|ASSIGNED |CLOSED Last Closed| |2022-09-19 19:49:55
package-review@lists.fedoraproject.org