https://bugzilla.redhat.com/show_bug.cgi?id=2241414 --- Comment #6 from Aaron Rainbolt <arraybolt3@xxxxxxxxx> --- (In reply to Neal Gompa from comment #5) > Initial spec review: > > > %global pkg_version 1.9.0 > > This seems redundant to %version? Do you need this? Doesn't appear so, dropped and adjusted the place that used it to use %{version} instead. > > BuildRequires: cmake-data > > This is not needed, as cmake-data is pulled in by cmake Dropped. > > BuildRequires: libicu-devel icu > > Please avoid putting multiple dependencies on a single line, as that makes > it difficult to diff later when changes happen. Fixed. > > Patch0: cmake-perl-bindings.diff > > Patch1: migrate-to-setuptools.diff > > While not required, stylistically usually these lines are right below Source > lines. Feel free to choose to move it or keep as-is. Makes sense, moved. > > %setup -q > > > > %patch -P0 -p1 -b .perl > > %patch -P1 -p1 > > This can be replaced with "%autosetup -p1" Done. > > find %{buildroot} -type f -name "*.la" -delete -print > > Does this CMake build even produce libtool archives? If not, this can be > dropped. Dropped, seems to work and I can't find any .la files in the resulting RPMs (though I didn't check the debug info RPMs). > > %doc AUTHORS COPYING ChangeLog LICENSE NEWS README > > "COPYING" and "LICENSE" need to be listed as "%license", like so: > > %license COPYING LICENSE Done. > > # Re-enable after upstream includes it with CMake builds > > %config(noreplace) %{_sysconfdir}/sword.conf > > This comment makes no sense? What's it for? I... have no idea :P The original maintainer of SWORD in Fedora was also an upstream developer, so I'm guessing he was leaving himself a note? In any event the actual line of code seems sensible, but I'm leery of dropping the comment in the event something happens in the upstream code that reminds me of this and helps me make a change. I'll ask the original maintainer about it when I get the chance. > > %{_datadir}/sword > > Please put a trailing slash here to ensure RPM knows to track it as a > directory. Done. > > %{perl_vendorarch}/* > > This glob is too greedy and needs to be scoped tighter, similar to the > Python one. Are you sure? I can scope this tighter, but according to the Perl Packaging Guidelines at https://docs.fedoraproject.org/en-US/packaging-guidelines/Perl/, > a arch-specific Perl package must own: > > # For arch-specific packages: vendorarch > %{perl_vendorarch}/* > %exclude %dir %{perl_vendorarch}/auto/ The links in the description should point to the updated files. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2241414 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202241414%23c6 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
