https://bugzilla.redhat.com/show_bug.cgi?id=1507103 Fabio Massimo Di Nitto <fdinitto@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(jpokorny@redhat.c | |om) --- Comment #59 from Fabio Massimo Di Nitto <fdinitto@xxxxxxxxxx> --- (In reply to Jan Pokorný from comment #57) > >> 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.] The current format still achieve the same technical goal. terse or verbose is still a matter of personal preference. The code is not obfuscated in either forms. > > >> 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. Please provide an example of which commands are you referring to. It´s hard to guess what you see without some data. > (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. the number of architectures is indeed a benefit, but we do test rebuild and check daily in CI to catch issues on latest of each distribution exactly for that use case. I am less interested in automatic rebuilds. Who does full rebuilds of fedora is never going to look at each single failure. > > > 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. Reasonable. > > >> 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. You haven´t answered either of my questions. > > * * * > > 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+ ok. > > > 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. It reflects the version of the onwire protocol. There are plenty libraries in Fedora that use similar convention. We can document it somewhere in the spec file. it´s not going to drop. > > 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 which version of the package did you test? we fixed that already upstream in 1.1. ./configure.ac does check if it is necessary to link to libm or not at build time to use ceil(). > > 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
