Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://smani.fedorapeople.org/review/perl-Hash-DefHash.spec SRPM URL: https://smani.fedorapeople.org/review/perl-Hash-DefHash-0.071-1.fc32.src.rpm Description: Manipulate defhash Fedora Account System Username: smani
Spec URL: https://smani.fedorapeople.org/review/perl-Hash-DefHash.spec SRPM URL: https://smani.fedorapeople.org/review/perl-Hash-DefHash-0.071-1.fc32.src.rpm %changelog * Tue Jan 07 2020 Sandro Mani <manisandro> - 0.071-2 - Fix / constrain BRs - Pass NO_PACKLIST=1 to Makefile.PL - Run fixperms on buildroot
Spec URL: https://smani.fedorapeople.org/review/perl-Hash-DefHash.spec SRPM URL: https://smani.fedorapeople.org/review/perl-Hash-DefHash-0.071-2.fc32.src.rpm %changelog * Tue Jan 07 2020 Sandro Mani <manisandro> - 0.071-2 - Fix / constrain BRs - Pass NO_PACKLIST=1 to Makefile.PL - Run fixperms on buildroot
The standalone spec file differs seems to be newer than the archived one. I will use the standalone one for this review. URL and Source0 addresses are usable. Ok. The Source0 archive (SHA-256: 21886ee5eb129c595455bccb4a9ab146445b1ac4c558932cf462bade9273133d) is original. Ok. Summary verified from lib/Hash/DefHash.pm. Ok. TODO: The description is not very explaining. Please add at least a link to DefHash specification <https://metacpan.org/pod/DefHash> to the description. License verified from README, lib/Hash/DefHash.pm, LICENSE, and Makefile.PL. Ok. No XS code, noarch BuildArch is Ok. TODO: Constrain 'perl(Exporter)' dependency with '>= 5.57' (META.json:35). Test::Pod::Coverage, Pod::Coverage::TrustPod, Test::Perl::Critic, Test::Pod are not used. Ok. FIX: Build-require 'perl(blib)' for tests (t/00-compile.t:20). All tests pass. Ok. $ rpmlint perl-Hash-DefHash.spec ../SRPMS/perl-Hash-DefHash-0.071-2.fc32.src.rpm ../RPMS/noarch/perl-Hash-DefHash-0.071-2.fc32.noarch.rpm sh: /usr/bin/python2: No such file or directory 2 packages and 1 specfiles checked; 0 errors, 0 warnings. rpmlint is Ok. $ rpm -q -lv -p ../RPMS/noarch/perl-Hash-DefHash-0.071-2.fc32.noarch.rpm drwxr-xr-x 2 root root 0 Jan 9 09:51 /usr/share/doc/perl-Hash-DefHash -rw-r--r-- 1 root root 1211 Jan 4 01:09 /usr/share/doc/perl-Hash-DefHash/Changes -rw-r--r-- 1 root root 6546 Jan 4 01:09 /usr/share/doc/perl-Hash-DefHash/README drwxr-xr-x 2 root root 0 Jan 9 09:51 /usr/share/licenses/perl-Hash-DefHash -rw-r--r-- 1 root root 18457 Jan 4 01:09 /usr/share/licenses/perl-Hash-DefHash/LICENSE -rw-r--r-- 1 root root 3543 Jan 9 09:51 /usr/share/man/man3/Hash::DefHash.3pm.gz drwxr-xr-x 2 root root 0 Jan 9 09:51 /usr/share/perl5/vendor_perl/Hash -rw-r--r-- 1 root root 15916 Jan 4 01:09 /usr/share/perl5/vendor_perl/Hash/DefHash.pm File layout and permissions are Ok. $ rpm -q --requires -p ../RPMS/noarch/perl-Hash-DefHash-0.071-2.fc32.noarch.rpm | sort -f | uniq -c 1 perl(:MODULE_COMPAT_5.30.1) 1 perl(:VERSION) >= 5.10.1 1 perl(Exporter) 1 perl(Scalar::Util) 1 perl(strict) 1 perl(String::Trim::More) 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(Exporter)' run-time dependency with '>= 5.57' (META.json:35). This is usually done by writing the dependency explicitly 'Requires: perl(Exporter) >= 5.57' into the spec file and the filtering out the unversioned one that was automatically generated by adding '%global __requires_exclude %{?__requires_exclude:%{__requires_exclude}|}^perl\\(Exporter)\\)$' just before %description section. See <https://docs.fedoraproject.org/en-US/packaging-guidelines/AutoProvidesAndRequiresFiltering/>. This is to keep the dependency list free from duplicates and thus minimizing repository metadata. Another option is to patch the sources like this: - use Exporter qw(import); + use Exporter 1.15 qw(import); $ rpm -q --provides -p ../RPMS/noarch/perl-Hash-DefHash-0.071-2.fc32.noarch.rpm | sort -f | uniq -c 1 perl(Hash::DefHash) = 0.071 1 perl-Hash-DefHash = 0.071-2.fc32 Binary provides are Ok. $ resolvedeps f32-build ../RPMS/noarch/perl-Hash-DefHash-0.071-2.fc32.noarch.rpm Binary dependencies are resolvable. Ok. The package builds in F32 (https://koji.fedoraproject.org/koji/taskinfo?taskID=40312018). Ok. Otherwise the package is in line with Fedora and Perl packaging guidelines. Please correct the 'FIX' item, consider fixing the 'TODO' items and provide a new spec file. Resolution: Package NOT approved.
Thanks for your comments. Some questions, also in the interest of improving my other perl packages: - How do you pick up all the BRs? Based on what I've learned so far, I'm doing something like $ tangerine -c Makefile.PL <subdirs> and then omitting the dependencies only required by author tests. - When do you decide to constrain a BR or run-time dependency? There are typically version constraints for most requires in the various metadata files. Wouldn't the package fail to build anyway if the requirement is not met? - And specifically to - use Exporter qw(import); + use Exporter 1.15 qw(import); How is this related to the >= 5.57 requirement?
(In reply to Sandro Mani from comment #4) > Thanks for your comments. Some questions, also in the interest of improving > my other perl packages: > You are doing very well. Your packages are almost perfect. > - How do you pick up all the BRs? Based on what I've learned so far, I'm > doing something like > > $ tangerine -c Makefile.PL <subdirs> > > and then omitting the dependencies only required by author tests. > Tangerine is a very good tool. I start with checking for a presence of common dependencies like perl-generators and perl(:MODULE_COMPAT...), then I read META.json or META.yml, then I scan Makefile.PL with tangerine. Then I read Makefile.PL as it sometimes contains very contrived code that can load of probe for other modules and scripts. Then I scan ./lib, then ./t or ./test.pl. Whenever I see a suspicious dependency or almost any non-trivial run-time loaded dependency in the output, I check the code to see whether the dependency is indeed needed. Then I also grep sources for 'eval' keyword to discover typically weak dependencies. I also grep sources for 'ENV' variable because especially tests tend to skip or perform some parts based on the environment variables. Then I read the spec file to see whether all executed commands (like 'make') are build-required. Special care is also devoted to bundled code in ./inc that I try to unbundle first. Then I build the package. Sometimes the test fails because of a missed dependency. That happened to me with the "blib" module here. blib module is now in perl-interpreter package, but I have locally a developmental perl that actually have it in a separate package that happened not to be installed. Then if there are some dependencies I'm not sure about their necessity, I will mangle their identifiers in the sources and run 'make test' on them to see what happens. Finally I compare the dependencies listed in the resulting binary package (rpm -q --requires ...) with those listed as BuildRequires. For that purpose I usually keep the dependencies for the packaged code and for the tests separated in the spec file. That eases the comparison. > - When do you decide to constrain a BR or run-time dependency? There are > typically version constraints for most requires in the various metadata > files. I usually follow the versions listed in metadata. Authors usually had a good reason for adding them there. But that's not always true. Some people or various release tools, like DistZilla, tend to write there versions from their current system. This case becomes obvious when rebasing a package to a new version. In that case you can see the versions changed to the latest version available on CPAN or changes without any reason (e.g. a new release changes only documentation, but there is a bump in a version of a dependency). The issue with specifying minimal versions is that those are usually historical versions that Fedora abandoned many releases ago. There is a good deal of packagers that do think that listing the versions is pointless because every supported Fedora contains them. Therefore I usually mark them as TODO in my reviews. It's also true that authors very often list the version in metadata without listing it in the sources ('use MODULE VERSION;'). Then the packaging becomes tedious because it requires writing a lot of lines for the packager (like the __requires_exlcude macro). On the other hand when somebody decides to port the package to an old EPEL, having the version in spec files becomes very handy because it reveals the incompatibility very early. > Wouldn't the package fail to build anyway if the requirement is not > met? The build should fail. (Although there are cases when the build script ignores the missing dependencies). But that does not prevent people from getting bitten at run-time. Imagine a code needs the latest Exporter. You build the package with it, the package gets installed to user's system, and then the user decides that he does not like the latest Exporter and downgrades perl-Exporter package. If the version constrain weren't specified for the run-time dependency, DNF would allow the user to get the system into a wrong state. It's true that Fedora users usually do not downgrade, partially because Fedora removes older packages from mirrors, but nevertheless the possibility is there. (E.g. a downgrade from updates-testing to updates Fedora repository. Or in RHEL where all historical packages are kept available indefinitly.) > - And specifically to > > - use Exporter qw(import); > + use Exporter 1.15 qw(import); > > How is this related to the >= 5.57 requirement? Oh, sorry. That's my mistake. There should be 'use Exporter 5.57 qw(import);'.
Thanks for the detailed explanations. Spec URL: https://smani.fedorapeople.org/review/perl-Hash-DefHash.spec SRPM URL: https://smani.fedorapeople.org/review/perl-Hash-DefHash-0.071-3.fc32.src.rpm %changelog * Thu Jan 09 2020 Sandro Mani <manisandro> - 0.071-3 - Improve description - Constrain Exporter dependency - BR: perl(blib)
> TODO: The description is not very explaining. Please add at least a link to DefHash specification <https://metacpan.org/pod/DefHash> to the description. Ok. > TODO: Constrain 'perl(Exporter)' dependency with '>= 5.57' (META.json:35). Ok. > FIX: Build-require 'perl(blib)' for tests (t/00-compile.t:20). Ok. $ rpmlint perl-Hash-DefHash.spec ../SRPMS/perl-Hash-DefHash-0.071-3.fc32.src.rpm ../RPMS/noarch/perl-Hash-DefHash-0.071-3.fc32.noarch.rpm sh: /usr/bin/python2: No such file or directory 2 packages and 1 specfiles checked; 0 errors, 0 warnings. rpmlint is Ok. > TODO: Constrain 'perl(Exporter)' run-time dependency with '>= 5.57' (META.json:35). $ rpm -q --requires -p ../RPMS/noarch/perl-Hash-DefHash-0.071-3.fc32.noarch.rpm | sort -f | uniq -c 1 perl(:MODULE_COMPAT_5.30.1) 1 perl(:VERSION) >= 5.10.1 1 perl(Exporter) >= 5.57 1 perl(Scalar::Util) 1 perl(strict) 1 perl(String::Trim::More) 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 Binary requires are Ok. Resolution: Package APPROVED.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/perl-Hash-DefHash