https://bugzilla.redhat.com/show_bug.cgi?id=1507103 --- Comment #57 from Jan Pokorný <jpokorny@xxxxxxxxxx> --- >> Some concerns that remain: >> >> A. [Comment 28] 4.: no reason to mangle with debuginfo generation >> - one can always use command-line switches to achieve the same: >> http://lists.rpm.org/pipermail/rpm-list/2013-April/001416.html >> (definitely not a mainstream need, even less in Fedora context) >> >> B. [Comment 11]: I'd still suggest using >> >> %{configure} \ >>> %{?with_sctp:--enable-libknet-sctp} \ >>> %{!?with_sctp:--disable-libknet-sctp} \ >>> [...] >> >> Reason is also practical, e.g. the whole "configure" statement >> barely fits a single laptop screen for me currently, because >> the notation of choice is excessively line-hungry. > > This is only a matter of personal preference. It doesn´t interfere > in any way with the review. Looks like bigger picture is neglected: having packages in somewhat unsurprising state (goal of package review, afterall) is not to the benefit of selected people, but for overall community. https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_Legibility Packages get routinely modified by proven packagers when new packaging/technical needs arise. And this general eligibility/predictability applies to both above points. Do not discount this by unsound personal preference claims, please. It takes effort on both sides to undergo the review if the result is not to become maintainance (and daily distro usage, e.g. in case of ldconfig point H. below) burden anytime soon. For instance, when a package attempts to juggle with debuginfo generation on its own, it attracts attention needlessly, and it's not clear at all why this is needed in Fedora context. If you think this is relevant for Fedora, the justification shall be provided in the form of a comment. Regarding using superfluous globals/non-terse conditionals, that is something killing said general eligibility. Instead of: > %bcond_without sctp > [...] > %if %{with sctp} > %global buildsctp 1 > %endif > [...] > %if %{defined buildsctp} > BuildRequires: lksctp-tools-devel > %endif > [...] > %{configure} \ > %if %{defined buildsctp} > --enable-libknet-sctp \ > %else > --disable-libknet-sctp \ > %endif please use: > %bcond_without sctp > [...] > %if %{with sctp} > BuildRequires: lksctp-tools-devel > %endif > [...] > %{configure} \ > %{?with_sctp:--enable-libknet-sctp} \ > %{!?with_sctp:--disable-libknet-sctp} \ [Voila, some bogus lines down, while eligibility raises.] >> Also, please: >> >> C. Refrain from initial spaces/indentation in %description-s. > > rationale? Not looking weird in comparison to other packages, e.g. in various output of console programs dealing with packages. (see the above note on unsurprising state) >> D. Check whether there are some tests that could be run as part of >> the build under %check scriptlet (to be added if that's the case). > > this is a good point, but FYI upstream already has an extensive CI/CD > including different versions of Fedora. This is indeed nice and respectable. But to provide other use cases, some preliminary toolchain rebuilds can be performed on the package base in sole discretion of the people involved, and having some kind of smoke test will help establish the confidence all work alright, before even hitting Rawhide. Also the number of architectures covered this way is rather impressive. And there are more benefits down the road AIUI, like test-rebuilding the inverse dependencies, IOW when some of kronosnet's dependency is receiving an update, for instance. > My only concern is that the testsuite does play with the network > (loopback interface) and should be very safe, but in the unlucky > event of bugs, we should probably avoid DoS´ing the fedora builders > by generating unwanted traffic. I think that Digimer choice to avoid > running the test suite is more of a safe precaution. Might be at least worth adding the %check scriptlet triggering some straightforward test in a commented-out form together with a brief explanation why it is deactivated per above. >> E. Because libknet1-devel requires (indirectly) libknet1, it may >> drop the %license files, as these will be present thanks to >> libknet1 installed in parallel, hence satisfying: >> >> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/ >> LicensingGuidelines#Subpackage_Licensing > > Is this a blocker for the review or a wishlist level? what are the > consequences of not doing it? Purpose of the package review is that at least at one well-defined point the packaging is known to be in order and in-line with the guidelines, otherwise it's mere relying on the eventual/hypothetical settlement (for which COPR repositories exist). In this light, let's just do it. * * * In addition, I have these (and no more, I swear) complaints: F. only and/or and parentheses are meta-connectives for the license tag: > GPLv2+ + LGPLv2+ should become > GPLv2+ and LGPLv2+ https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios G. regarding naming guidelines, explanation is missing why digit-ending packages (libknet1) are present, which is not customary in Fedora and only expected when multiple versions of a package can be installed simultaneously: https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:NamingGuidelines#MultiplePackages Is this indeed the case with kronosnet? It doesn't look like that to me, otherwise the plugin dir (%{_libdir}/kronosnet) would also be versioned (e.g., %{_libdir}/kronosnet1). Then, please drop those trailing digits from package names at all places. H. instead of > %post -n SUBPKG -p /sbin/ldconfig use > %ldconfig_scriptlets SUBPKG the appropriate place: https://fedoraproject.org/wiki/Packaging:Scriptlets#Shared_Libraries * * * FYI only: I. possible overlinking reported by rpmlint: > libknet1.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libknet.so.1.1.0 /lib64/libm.so.6 J. "Suggests:" hints may be suitable for libknet on *-plugins-all ("Recommends:" will be understood in dnf case as "Requires:" by default, which may not be always desired, turning Suggests to Recommends looks less problematic than the opposite should the change become appealing) -- 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 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
