https://bugzilla.redhat.com/show_bug.cgi?id=1314865 --- Comment #6 from Jan Pokorný <jpokorny@xxxxxxxxxx> --- Thanks again for your detailed review. Note that in the meantime, the upstream has released 1.0 version, and I made quite a few changes, upstreamed or not (pending PRs, spec file customization). Also note that I split the package as per the description within the spec file itself and as sketched out in discussion with upstream to receive the feedback: https://github.com/ClusterLabs/booth/issues/25 Also added "Conflicts" tags so as to achieve a desired behavior when upgrading from pre-split installation (such as the mentioned https://copr.fedorainfracloud.org/coprs/jpokorny/booth/build/165647/ or possibly OBS: https://build.opensuse.org/package/show/network:ha-clustering:Stable/booth) New round (with comments on the points you raised state below): https://people.redhat.com/jpokorny/pkgs/booth/review/booth.spec https://people.redhat.com/jpokorny/pkgs/booth/review/booth-1.0-1.eb4256a.git.src.rpm re [comment 3]: > FIX: Source0 is does point to upstream release tar ball. Follow > <https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Hosting_Services>. Now it points to URL that can be used to retrieve the very archive. > FIX: The spec file contains code for SuSE. Remove spec file code not related to > Fedora or EPEL. Should be fixed. > License review: > src/transport.c: GPLv3+ (the code refers to GPLv2.1 or later. > There is no GPLv2.1. The nearest higher is GPLv3.) > script/ocf/geostore: GPLv2 > script/ocf/geo_attr.sh: GPLv2 > docs/geostore.8.txt: GPL+ > docs/Makefile.am: GPLv2+ > Makefile.am: BSD > GNUmakefile: GPLv2+ > COPYING: GPLv2 text > > FIX: While the "GPLv2.1" looks like an omission from porting from LGPLv2+, we > cannot assume author's intention. Please ask authors about clarification and > add it to this package. Otherwise the License tag must be changed to "GPLv3+". > You can also tell him about the other files no being GPLv2+. > FIX: Some files are GPLv2 only. Add it to the License tag. I started a discussion with upstream regarding this: http://oss.clusterlabs.org/pipermail/developers/2016-March/000172.html with the outcome being (beside 2 other, related packages getting similar attention): https://github.com/ClusterLabs/booth/pull/23 https://github.com/ClusterLabs/booth/pull/24 Thanks for the initial impulse! > FIX: Build-require bash (autogen.sh:1) Added "BR: /bin/sh". > TODO: Use plain commands instead of the macros (install instead of > %{__install}). Also discussed in [comment 4] and [comment 5]. Kept just these: - %{configure}: sets the right variables - %{make_build}: already uses correct multiprocessing option - %{make_install}: sets correct DESTDIR (furthermore, if one wants to go as far as using RPM on BSD system, having "make" symbol defined as "gmake" is really handy) > TODO: Then build-reqire coreutils and make because you will invoke them > directly. Done. > TODO: Execute tests in the %check section. The %run_build_tests macro is > undefined now. Done. The suite of unit tests required some love: https://github.com/ClusterLabs/booth/pull/31 (included as extra patches). > FIX: Require(pre) gawk (booth.spec:218). > FIX: Require(pre) grep (boot.spec:220). > FIX: Require(pre) procps-ng (boot.spec:223). No longer applicable. > FIX: Build-require gcc (configure.ac:56). Done. > TODO: Build-require pkgconfig if it is used (configure.ac:62). Done. > TODO: I recommend to remove adding -O3 and -ggdb3 to CFLAGS in > configure.ac:383. It changes distribution-wide settings. Done using ./configure --enable-user-flags. > FIX: Build-require coreutils (Makefile.am:90). Done. > FIX: Make booth-test dependency on booth architecture-specific or change the > boot-test architecture to noarch. (It will prevent from mixing packages with > architectures.) Also require exact booth version and release to match the booth > package. -test package is now noarch. > $ rpmlint booth.spec ../SRPMS/booth-1.0-0.1.rc1.fc25.src.rpm > [...] > TODO: Correct the mixed white space. Done. > FIX: The /usr/sbin/rcbooth-arbitrator is a symlink to /usr/sbin/service that is > provided by initscripts. Either remove the symlink or run-require initscripts. > Because /usr/sbin/service is for compatibility with SystemV init and that is > forbidden in Fedora now > <https://fedoraproject.org/wiki/Packaging:Guidelines#Initscripts>, please port > it to systemctl(8) command. In fact even the last occurrence of its possible use will effectively skip that, asking for complete removal on systemd-enabled system: https://github.com/ClusterLabs/booth/blob/v1.0/test/live_test.sh#L165 > TODO: Move /etc/booth/booth.conf.example to /usr/share/doc or make it a proper > noreplacable /etc/booth/booth.conf configuration file. Used the former. > FIX: Package ChangeLog as a documentation. Done. > $ rpm -q -lv -p ../RPMS/x86_64/booth-1.0-0.1.rc1.fc25.x86_64.rpm > [...] > TODO: Do not own /usr/lib/ocf. This is provided by pacemaker. Done, and the same treatment is applied to: - /usr/lib/ocf/resource.d (resource-agents, pacemaker) - /usr/lib/ocf/resource.d/pacemaker (pacemaker) > $ rpm -q --requires -p ../RPMS/x86_64/booth-test-1.0-0.1.rc1.fc25.x86_64.rpm | > sort -f | uniq -c > 1 /bin/bash > 1 /bin/sh > 1 /usr/bin/python > 1 booth > 1 python > 1 rpmlib(CompressedFileNames) <= 3.0.4-1 > 1 rpmlib(FileDigests) <= 4.6.0-1 > 1 rpmlib(PartialHardlinkSets) <= 4.0.4-1 > 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 > 1 rpmlib(PayloadIsXz) <= 5.2-1 > TODO: The /usr/bin/python is autodected. I think you should not run-require > python package. > I do not know much about Python packaging. I hope it's Ok to have the bytecode > files under /usr/share and not run-require specific Python ABI. At least if the > code can work with Python 2 and 3. AFAIK it's Python2-only. Added "Requires: python(abi) < 3". -test comes as a package supporting the maintenance and could be omitted altogether if there are conflicts about how to deal with it. -- 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 http://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx