Spec URL: https://people.redhat.com/jpokorny/pkgs/booth/review/booth.spec SRPM URL: https://people.redhat.com/jpokorny/pkgs/booth/review/booth-1.0-0.1.rc1.src.rpm Description: Booth manages tickets which authorize cluster sites located in geographically dispersed locations to run resources. It facilitates support of geographically distributed clustering in Pacemaker. Fedora Account System Username: jpokorny Legal note: Initial 0.1.X releases contained code that referred to Paxos algorithm, which is likely patent-encumbered, so booth might have or might not have been legally tainted. See also an answer for such a point from a SUSE developer [1], however detailed follow-up question [2] stayed unreplied. Despite the original defense, Paxos-related code got dropped [3] in favor of Raft algorithm [4] that was in being integrated since around that time [5], hitting 0.2.0+ releases. Bottom line: booth 0.2.0+ (booth-1.0rc1 + patches is being considered for distro inclusion as of this review) should be fine. [1] http://oss.clusterlabs.org/pipermail/pacemaker/2013-February/016909.html [2] http://oss.clusterlabs.org/pipermail/pacemaker/2013-April/017823.html [3] https://github.com/ClusterLabs/booth/commit/5f1807f [4] https://github.com/ClusterLabs/booth/blob/v1.0rc1/README#L109 [5] https://github.com/ClusterLabs/booth/commit/720349f Notes: - COPR builds: https://copr.fedorainfracloud.org/coprs/jpokorny/booth/build/165647/ - incorrect FSF address to be resolved in upstream: https://github.com/ClusterLabs/booth/pull/23 - offering a review swap (just in case someone randomly browses new package reviews, but will haunt down that via devel ML eventually)
Per Legal note in [comment 0], I am sincerely asking the legal team to validate the current state (i.e., as of the version under review) of booth.
Based on the data presented and the state of the 1.0rc1 code, I see no reason to block this package. Lifting FE-Legal.
Url is usable. Ok. FIX: Source0 is does point to upstream release tar ball. Follow <https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Hosting_Services>. Source archive is original (SHA-256: 434c4781a52df58b18cb4c5c1b570439d173c7616114c4740f89965f8d397bf5). Ok. FIX: The spec file contains code for SuSE. Remove spec file code not related to Fedora or EPEL. 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. Summary is Ok. Description verified from README. Ok. FIX: Build-require bash (autogen.sh:1) TODO: Use plain commands instead of the macros (install instead of %{__install}). TODO: Then build-reqire coreutils and make because you will invoke them directly. TODO: Execute tests in the %check section. The %run_build_tests macro is undefined now. FIX: Require(pre) gawk (booth.spec:218). FIX: Require(pre) grep (boot.spec:220). FIX: Require(pre) procps-ng (boot.spec:223). FIX: Build-require gcc (configure.ac:56). TODO: Build-require pkgconfig if it is used (configure.ac:62). plumb not needed. Ok. libqb >= 1.0 is optional. Ok. TODO: I recommend to remove adding -O3 and -ggdb3 to CFLAGS in configure.ac:383. It changes distribution-wide settings. FIX: Build-require coreutils (Makefile.am:90). 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. Package builds. Ok. $ rpmlint booth.spec ../SRPMS/booth-1.0-0.1.rc1.fc25.src.rpm ../RPMS/x86_64/booth-* booth.spec:43: W: non-standard-group %{pkg_group} booth.spec:183: E: hardcoded-library-path in /usr/lib/ocf booth.spec:184: E: hardcoded-library-path in /usr/lib/ocf/resource.d booth.spec:185: E: hardcoded-library-path in /usr/lib/ocf/resource.d/pacemaker booth.spec:186: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth booth.spec:187: E: hardcoded-library-path in /usr/lib/ocf/lib booth.spec:188: E: hardcoded-library-path in /usr/lib/ocf/lib/booth booth.spec:191: E: hardcoded-library-path in /usr/lib/ocf/resource.d/pacemaker/booth-site booth.spec:192: E: hardcoded-library-path in /usr/lib/ocf/lib/booth/geo_attr.sh booth.spec:193: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/geostore booth.spec:232: W: non-standard-group %{pkg_group} booth.spec:243: E: hardcoded-library-path in /usr/lib/ocf booth.spec:244: E: hardcoded-library-path in /usr/lib/ocf/resource.d booth.spec:245: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth booth.spec:246: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/sharedrsc booth.spec:23: W: mixed-use-of-spaces-and-tabs (spaces: line 23, tab: line 23) booth.spec: W: invalid-url Source0: booth-1.0rc1.tar.gz booth.src:183: E: hardcoded-library-path in /usr/lib/ocf booth.src:184: E: hardcoded-library-path in /usr/lib/ocf/resource.d booth.src:185: E: hardcoded-library-path in /usr/lib/ocf/resource.d/pacemaker booth.src:186: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth booth.src:187: E: hardcoded-library-path in /usr/lib/ocf/lib booth.src:188: E: hardcoded-library-path in /usr/lib/ocf/lib/booth booth.src:191: E: hardcoded-library-path in /usr/lib/ocf/resource.d/pacemaker/booth-site booth.src:192: E: hardcoded-library-path in /usr/lib/ocf/lib/booth/geo_attr.sh booth.src:193: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/geostore booth.src:243: E: hardcoded-library-path in /usr/lib/ocf booth.src:244: E: hardcoded-library-path in /usr/lib/ocf/resource.d booth.src:245: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth booth.src:246: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/sharedrsc booth.src:23: W: mixed-use-of-spaces-and-tabs (spaces: line 23, tab: line 23) booth.src: W: invalid-url Source0: booth-1.0rc1.tar.gz booth.x86_64: W: conffile-without-noreplace-flag /etc/booth/booth.conf.example booth.x86_64: E: incorrect-fsf-address /usr/sbin/booth-keygen booth.x86_64: E: incorrect-fsf-address /usr/lib/ocf/resource.d/pacemaker/booth-site booth.x86_64: E: incorrect-fsf-address /usr/lib/ocf/lib/booth/geo_attr.sh booth.x86_64: W: dangling-symlink /usr/sbin/rcbooth-arbitrator /usr/sbin/service booth.x86_64: W: no-manual-page-for-binary rcbooth-arbitrator booth-test.x86_64: W: only-non-binary-in-usr-lib booth-test.x86_64: W: dangling-symlink /usr/share/booth/tests/src/boothd /usr/sbin/boothd 4 packages and 1 specfiles checked; 29 errors, 11 warnings. The /usr/lib/ocf is pacemaker stuff. Ok. TODO: Correct the mixed white space. 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. TODO: Move /etc/booth/booth.conf.example to /usr/share/doc or make it a proper noreplacable /etc/booth/booth.conf configuration file. FIX: Package ChangeLog as a documentation. $ rpm -q -lv -p ../RPMS/x86_64/booth-1.0-0.1.rc1.fc25.x86_64.rpm drwxr-xr-x 2 root root 0 Mar 8 15:43 /etc/booth -rw-r--r-- 1 root root 965 Mar 8 15:43 /etc/booth/booth.conf.example drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf/lib drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf/lib/booth -rw-r--r-- 1 root root 5323 Mar 8 15:43 /usr/lib/ocf/lib/booth/geo_attr.sh drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf/resource.d drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf/resource.d/booth -rwxr-xr-x 1 root root 3216 Mar 8 15:43 /usr/lib/ocf/resource.d/booth/geostore drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf/resource.d/pacemaker -rwxr-xr-x 1 root root 4914 Mar 8 15:43 /usr/lib/ocf/resource.d/pacemaker/booth-site -rw-r--r-- 1 root root 338 Jan 12 11:50 /usr/lib/systemd/system/booth@.service lrwxrwxrwx 1 root root 16 Mar 8 15:43 /usr/sbin/booth -> /usr/sbin/boothd -rwxr-xr-x 1 root root 1626 Mar 8 15:43 /usr/sbin/booth-keygen -rwxr-xr-x 1 root root 189792 Mar 8 15:43 /usr/sbin/boothd lrwxrwxrwx 1 root root 16 Mar 8 15:43 /usr/sbin/geostore -> /usr/sbin/boothd lrwxrwxrwx 1 root root 17 Mar 8 15:43 /usr/sbin/rcbooth-arbitrator -> /usr/sbin/service drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/share/booth -rwxr-xr-x 1 root root 1728 Mar 8 15:43 /usr/share/booth/service-runnable drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/share/doc/booth -rw-r--r-- 1 root root 585 Jan 12 11:50 /usr/share/doc/booth/AUTHORS -rw-r--r-- 1 root root 17987 Mar 8 15:43 /usr/share/doc/booth/COPYING -rw-r--r-- 1 root root 8031 Jan 12 11:50 /usr/share/doc/booth/README -rw-r--r-- 1 root root 1668 Jan 12 11:50 /usr/share/doc/booth/README.upgrade-from-v0.1 -rw-r--r-- 1 root root 860 Mar 8 15:43 /usr/share/man/man8/booth-keygen.8.gz lrwxrwxrwx 1 root root 11 Mar 8 15:43 /usr/share/man/man8/booth.8.gz -> boothd.8.gz -rw-r--r-- 1 root root 7469 Mar 8 15:43 /usr/share/man/man8/boothd.8.gz -rw-r--r-- 1 root root 1682 Mar 8 15:43 /usr/share/man/man8/geostore.8.gz TODO: Do not own /usr/lib/ocf. This is provided by pacemaker. $ rpm -q -lv -p ../RPMS/x86_64/booth-test-1.0-0.1.rc1.fc25.x86_64.rpm drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf/resource.d drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/lib/ocf/resource.d/booth -rwxr-xr-x 1 root root 4984 Mar 8 15:43 /usr/lib/ocf/resource.d/booth/sharedrsc drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/share/booth/tests drwxr-xr-x 2 root root 0 Jan 12 11:50 /usr/share/booth/tests/conf -rw-r--r-- 1 root root 965 Jan 12 11:50 /usr/share/booth/tests/conf/booth.conf.example -rw-r--r-- 1 root root 338 Jan 12 11:50 /usr/share/booth/tests/conf/booth@.service drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/share/booth/tests/src lrwxrwxrwx 1 root root 16 Mar 8 15:43 /usr/share/booth/tests/src/boothd -> /usr/sbin/boothd drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/share/booth/tests/test -rwxr-xr-x 1 root root 122 Jan 12 11:50 /usr/share/booth/tests/test/arbtests.py -rw-r--r-- 2 root root 417 Mar 8 15:43 /usr/share/booth/tests/test/arbtests.pyc -rw-r--r-- 2 root root 417 Mar 8 15:43 /usr/share/booth/tests/test/arbtests.pyo -rwxr-xr-x 1 root root 2317 Jan 12 11:50 /usr/share/booth/tests/test/assertions.py -rw-r--r-- 2 root root 2336 Mar 8 15:43 /usr/share/booth/tests/test/assertions.pyc -rw-r--r-- 2 root root 2336 Mar 8 15:43 /usr/share/booth/tests/test/assertions.pyo -rwxr-xr-x 1 root root 786 Jan 12 11:50 /usr/share/booth/tests/test/booth_path -rwxr-xr-x 1 root root 3153 Jan 12 11:50 /usr/share/booth/tests/test/boothrunner.py -rw-r--r-- 2 root root 4290 Mar 8 15:43 /usr/share/booth/tests/test/boothrunner.pyc -rw-r--r-- 2 root root 4290 Mar 8 15:43 /usr/share/booth/tests/test/boothrunner.pyo -rwxr-xr-x 1 root root 2859 Jan 12 11:50 /usr/share/booth/tests/test/boothtestenv.py -rw-r--r-- 2 root root 3301 Mar 8 15:43 /usr/share/booth/tests/test/boothtestenv.pyc -rw-r--r-- 2 root root 3301 Mar 8 15:43 /usr/share/booth/tests/test/boothtestenv.pyo -rwxr-xr-x 1 root root 1025 Jan 12 11:50 /usr/share/booth/tests/test/clientenv.py -rw-r--r-- 2 root root 1503 Mar 8 15:43 /usr/share/booth/tests/test/clientenv.pyc -rw-r--r-- 2 root root 1503 Mar 8 15:43 /usr/share/booth/tests/test/clientenv.pyo -rwxr-xr-x 1 root root 843 Jan 12 11:50 /usr/share/booth/tests/test/clienttests.py -rw-r--r-- 2 root root 1217 Mar 8 15:43 /usr/share/booth/tests/test/clienttests.pyc -rw-r--r-- 2 root root 1217 Mar 8 15:43 /usr/share/booth/tests/test/clienttests.pyo -rwxr-xr-x 1 root root 25503 Jan 12 11:50 /usr/share/booth/tests/test/live_test.sh -rwxr-xr-x 1 root root 1540 Jan 12 11:50 /usr/share/booth/tests/test/runtests.py -rw-r--r-- 2 root root 1573 Mar 8 15:43 /usr/share/booth/tests/test/runtests.pyc -rw-r--r-- 2 root root 1573 Mar 8 15:43 /usr/share/booth/tests/test/runtests.pyo -rwxr-xr-x 1 root root 7478 Jan 12 11:50 /usr/share/booth/tests/test/serverenv.py -rw-r--r-- 2 root root 7262 Mar 8 15:43 /usr/share/booth/tests/test/serverenv.pyc -rw-r--r-- 2 root root 7262 Mar 8 15:43 /usr/share/booth/tests/test/serverenv.pyo -rwxr-xr-x 1 root root 6138 Jan 12 11:50 /usr/share/booth/tests/test/servertests.py -rw-r--r-- 2 root root 5777 Mar 8 15:43 /usr/share/booth/tests/test/servertests.pyc -rw-r--r-- 2 root root 5777 Mar 8 15:43 /usr/share/booth/tests/test/servertests.pyo -rwxr-xr-x 1 root root 110 Jan 12 11:50 /usr/share/booth/tests/test/sitetests.py -rw-r--r-- 2 root root 407 Mar 8 15:43 /usr/share/booth/tests/test/sitetests.pyc -rw-r--r-- 2 root root 407 Mar 8 15:43 /usr/share/booth/tests/test/sitetests.pyo -rwxr-xr-x 1 root root 504 Jan 12 11:50 /usr/share/booth/tests/test/utils.py -rw-r--r-- 2 root root 804 Mar 8 15:43 /usr/share/booth/tests/test/utils.pyc -rw-r--r-- 2 root root 804 Mar 8 15:43 /usr/share/booth/tests/test/utils.pyo -rwxr-xr-x 1 root root 21732 Mar 8 15:42 /usr/share/booth/tests/unit-test.py -rw-r--r-- 1 root root 18949 Mar 8 15:43 /usr/share/booth/tests/unit-test.pyc -rw-r--r-- 1 root root 18809 Mar 8 15:43 /usr/share/booth/tests/unit-test.pyo drwxr-xr-x 2 root root 0 Mar 8 15:42 /usr/share/booth/tests/unit-tests -rw-r--r-- 1 root root 572 Jan 12 11:50 /usr/share/booth/tests/unit-tests/001_init-get-heartbeat.txt -rw-r--r-- 1 root root 1304 Jan 12 11:50 /usr/share/booth/tests/unit-tests/002_bad_packets.txt -rw-r--r-- 1 root root 494 Jan 12 11:50 /usr/share/booth/tests/unit-tests/003_pacemaker.txt -rw-r--r-- 1 root root 1440 Jan 12 11:50 /usr/share/booth/tests/unit-tests/010_retries.txt -rw-r--r-- 1 root root 1201 Mar 8 15:42 /usr/share/booth/tests/unit-tests/020_ext-verifier.txt -rw-r--r-- 1 root root 920 Jan 12 11:50 /usr/share/booth/tests/unit-tests/060_catchup_same_owner.txt -rw-r--r-- 1 root root 845 Jan 12 11:50 /usr/share/booth/tests/unit-tests/100_abort-after-retries.txt -rw-r--r-- 1 root root 1223 Mar 8 15:42 /usr/share/booth/tests/unit-tests/_defaults.txt drwxr-xr-x 2 root root 0 Jan 12 11:50 /usr/share/booth/tests/unit-tests/bin -rwxr-xr-x 1 root root 869 Jan 12 11:50 /usr/share/booth/tests/unit-tests/bin/crm_ticket -rw-r--r-- 1 root root 511 Jan 12 11:50 /usr/share/booth/tests/unit-tests/booth.conf -rw-r--r-- 1 root root 1021 Jan 12 11:50 /usr/share/booth/tests/unit-tests/init-catchup.txt drwxr-xr-x 2 root root 0 Mar 8 15:43 /usr/share/doc/booth-test -rw-r--r-- 1 root root 6011 Jan 12 11:50 /usr/share/doc/booth-test/README-testing /usr/lib/ocf/resource.d/booth/sharedrsc is for tests. Ok. $ rpm -q --requires -p ../RPMS/x86_64/booth-1.0-0.1.rc1.fc25.x86_64.rpm | sort -f | uniq -c 1 /bin/bash 2 /bin/sh 1 config(booth) = 1.0-0.1.rc1.fc25 1 libc.so.6()(64bit) 1 libc.so.6(GLIBC_2.14)(64bit) 1 libc.so.6(GLIBC_2.15)(64bit) 1 libc.so.6(GLIBC_2.2.5)(64bit) 1 libc.so.6(GLIBC_2.3)(64bit) 1 libc.so.6(GLIBC_2.3.4)(64bit) 1 libc.so.6(GLIBC_2.4)(64bit) 1 libdl.so.2()(64bit) 1 libgcrypt.so.20()(64bit) 1 libgcrypt.so.20(GCRYPT_1.6)(64bit) 1 libglib-2.0.so.0()(64bit) 1 libm.so.6()(64bit) 1 libpthread.so.0()(64bit) 1 libpthread.so.0(GLIBC_2.2.5)(64bit) 1 libqb.so.0()(64bit) 1 librt.so.1()(64bit) 1 librt.so.1(GLIBC_2.2.5)(64bit) 1 libsystemd.so.0()(64bit) 1 libsystemd.so.0(LIBSYSTEMD_209)(64bit) 1 libxml2.so.2()(64bit) 1 libxml2.so.2(LIBXML2_2.4.30)(64bit) 1 libxml2.so.2(LIBXML2_2.6.0)(64bit) 1 libz.so.1()(64bit) 1 pacemaker >= 1.1.8 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsXz) <= 5.2-1 1 rtld(GNU_HASH) $ 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 Pyhon ABI. At least if the code can work with Python 2 and 3. $ rpm -q --provides -p ../RPMS/x86_64/booth-1.0-0.1.rc1.fc25.x86_64.rpm | sort -f | uniq -c 1 booth = 1.0-0.1.rc1.fc25 1 booth(x86-64) = 1.0-0.1.rc1.fc25 1 config(booth) = 1.0-0.1.rc1.fc25 $ rpm -q --provides -p ../RPMS/x86_64/booth-test-1.0-0.1.rc1.fc25.x86_64.rpm | sort -f | uniq -c 1 booth-test = 1.0-0.1.rc1.fc25 1 booth-test(x86-64) = 1.0-0.1.rc1.fc25 Binary provides are Ok. $ resolvedeps rawhide ../RPMS/x86_64/booth-1.0-0.1.rc1.fc25.x86_64.rpm ../RPMS/x86_64/booth-test-1.0-0.1.rc1.fc25.x86_64.rpm Binary dependencies resolvable. Ok. Package builds in F25 (http://koji.fedoraproject.org/koji/taskinfo?taskID=13275897). Ok. Please correct all `FIX' items, consider fixing `TODO' items and provide a new spec file. Resolution: Package NOT approved.
First, thanks for the very in-depth review. Really good points were raised, some will be discussed with upstream for sure. > TODO: Use plain commands instead of the macros (install instead of > %{__install}). Any particular justification (beside that in some instances, command != %{__command} as some more flags are predefined)? Was expecting the main purpose of having these macros predefined is to: - avoid using shell builtins - be independent on particular authoritative path of the binary/script (cf. /bin vs. /usr/bin) - %{__command_flag} formatted macros signal it's desired to run them like these (cf. cp -a)
I don't know what purpose of the macros is. I think the macros are considered overengineered. Especially if plain command does the same job. See <https://fedoraproject.org/wiki/Packaging:Guidelines#Macros>: > Macro forms of system executables SHOULD NOT be used except when there > is a need to allow the location of those executables to be configurable.
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.
Source URL is usable. Ok. Source archive is orignal (SHA-256: 66420985bee695d9b9e77d8f6d26f6400c29f492f3df5270aadf6452f918bf09). Ok. License verified from COPYING and various source files. Ok. TODO: Source files docs/geostore.8.txt and docs/boothd.8.txt and thus manual pages geostore(8) and boothd(8) still declare GPL+. It would be great to notify the upstream. All tests pass. Ok. $ rpmlint booth.spec ../SRPMS/booth-1.0-1.eb4256a.git.fc25.src.rpm ../RPMS/{noarch,x86_64}/booth-* booth.spec:241: E: hardcoded-library-path in /usr/lib/ocf/resource.d/pacemaker/booth-site booth.spec:242: E: hardcoded-library-path in /usr/lib/ocf/lib/booth booth.spec:243: E: hardcoded-library-path in /usr/lib/ocf/lib/booth/geo_attr.sh booth.spec:248: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth booth.spec:249: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/geostore booth.spec:259: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/sharedrsc booth.src:241: E: hardcoded-library-path in /usr/lib/ocf/resource.d/pacemaker/booth-site booth.src:242: E: hardcoded-library-path in /usr/lib/ocf/lib/booth booth.src:243: E: hardcoded-library-path in /usr/lib/ocf/lib/booth/geo_attr.sh booth.src:248: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth booth.src:249: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/geostore booth.src:259: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/sharedrsc booth-arbitrator.noarch: W: spelling-error %description -l en_US multi -> mulch, mufti booth-arbitrator.noarch: W: no-documentation booth-site.noarch: W: spelling-error %description -l en_US multi -> mulch, mufti booth-site.noarch: W: only-non-binary-in-usr-lib booth-site.noarch: W: dangling-symlink /usr/sbin/geostore /usr/sbin/boothd booth-test.noarch: W: spelling-error %description -l en_US multi -> mulch, mufti booth-test.noarch: W: only-non-binary-in-usr-lib booth-test.noarch: W: dangling-symlink /usr/share/booth/tests/src/boothd /usr/sbin/boothd booth.x86_64: E: no-binary booth.x86_64: W: no-documentation booth-core.x86_64: W: spelling-error Summary(en_US) executables -> executable, executable s, executrices booth-core.x86_64: W: spelling-error %description -l en_US executables -> executable, executable s, executrices booth-core.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti 7 packages and 1 specfiles checked; 13 errors, 12 warnings. rpmlint is Ok. $ rpm -q -l -p ../RPMS/noarch/booth-arbitrator-1.0-1.eb4256a.git.fc25.noarch.rpm /usr/lib/systemd/system/booth-arbitrator.service /usr/lib/systemd/system/booth@.service $ rpm -q --requires -p ../RPMS/noarch/booth-arbitrator-1.0-1.eb4256a.git.fc25.noarch.rpm | sort -f | uniq -c 1 booth-core = 1.0-1.eb4256a.git.fc25 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsXz) <= 5.2-1 FIX: booth-arbitrator should package transaction scripts and require systemd <https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#Systemd>. # rpm -i --excludedocs ~test/rpmbuild/RPMS/x86_64/booth-core-1.0-1.eb4256a.git.fc25.x86_64.rpm # rpm -ql booth-core /etc/booth /usr/sbin/booth /usr/sbin/booth-keygen /usr/sbin/boothd /usr/share/doc/booth/AUTHORS /usr/share/doc/booth/COPYING /usr/share/doc/booth/ChangeLog /usr/share/doc/booth/README /usr/share/doc/booth/booth.conf.example /usr/share/man/man8/booth-keygen.8.gz /usr/share/man/man8/booth.8.gz /usr/share/man/man8/boothd.8.gz # LC_ALL=en_US.UTF-8 stat /usr/share/doc/booth/COPYING stat: cannot stat '/usr/share/doc/booth/COPYING': No such file or directory FIX: Packaging COPYING by "%license %{_pkgdocdir}/COPYING" does not work. It still marks COPYING as a documentation and removes the license file if documentation is excluded. Try package it by relative path or report a bug against rpm. Otherwise file layout, permissions, and dependencies are Ok. Package builds in F25 (http://koji.fedoraproject.org/koji/taskinfo?taskID=13903771). Ok. Please correct the `FIX' issues and provide a new spec file. Resolution: Package NOT approved.
re [comment 7]: > FIX: Packaging COPYING by "%license %{_pkgdocdir}/COPYING" does not > work. It still marks COPYING as a documentation and removes the license > file if documentation is excluded. Try package it by relative path done > or report a bug against rpm. this as well: [bug 1333509] > Please correct the `FIX' issues and provide a new spec file. https://people.redhat.com/jpokorny/pkgs/booth/review/booth.spec https://people.redhat.com/jpokorny/pkgs/booth/review/booth-1.0-2.eb4256a.git.src.rpm > @@ -98,7 +98,7 @@ > BuildRequires: hostname psmisc > BuildRequires: python2-devel > # spec file specifics > -## for _unitdir macro > +## for _unitdir, systemd_requires and specific scriptlet macros > BuildRequires: systemd > ## for autosetup > BuildRequires: git > @@ -136,6 +136,7 @@ > Group: System Environment/Daemons > BuildArch: noarch > Requires: %{name}-core = %{version}-%{release} > +%{?systemd_requires} > # deal with pre-split arrangement > Conflicts: %{name} < 1.0-1 > > @@ -143,6 +144,15 @@ > Support for running Booth, ticket manager for multi-site clusters, > as an arbitrator. > > +%post arbitrator > +%systemd_post booth@.service booth-arbitrator.service > + > +%preun arbitrator > +%systemd_preun booth@.service booth-arbitrator.service > + > +%postun arbitrator > +%systemd_postun_with_restart booth@.service booth-arbitrator.service > + > %package site > Summary: Booth support for running as a full-fledged site > Group: System Environment/Daemons > @@ -201,9 +211,10 @@ > ln -s boothd.8 %{buildroot}/%{_mandir}/man8/booth.8 > cp -a -t %{buildroot}/%{_pkgdocdir} \ > -- ChangeLog README-testing conf/booth.conf.example > -# drop what we don't package anyway > +# drop what we don't package anyway (COPYING added via tarball-relative path) > rm -rf %{buildroot}/%{_initrddir}/booth-arbitrator > rm -rf %{buildroot}/%{_pkgdocdir}/README.upgrade-from-v0.1 > +rm -rf %{buildroot}/%{_pkgdocdir}/COPYING > # tests > mkdir -p %{buildroot}/%{test_path} > cp -a -t %{buildroot}/%{test_path} \ > @@ -219,7 +230,7 @@ > VERBOSE=1 make check > > %files core > -%license %{_pkgdocdir}/COPYING > +%license COPYING > %doc %{_pkgdocdir}/AUTHORS > %doc %{_pkgdocdir}/ChangeLog > %doc %{_pkgdocdir}/README > @@ -259,5 +270,14 @@ > /usr/lib/ocf/resource.d/booth/sharedrsc > > %changelog > +* Thu May 05 2016 Jan Pokorný <jpokorny+rpm-booth> - 1.0-2.eb4256a.git > +- pre-inclusion cleanups in the spec (apply systemd scriptlet operations > + with booth-arbitrator, avoid overloading file implicitly considered %%doc > + as %%license) > +- update a subset of out-of-tree patches per > + https://github.com/ClusterLabs/booth/pull/22#issuecomment-216936987 > + Resolves: rhbz#1314865 > + Related: rhbz#1333509 > + > * Thu Apr 28 2016 Jan Pokorný <jpokorny+rpm-booth> - 1.0-1.eb4256a.git > - initial build
$ rpmlint booth.spec ../SRPMS/booth-1.0-2.eb4256a.git.fc25.src.rpm ../RPMS/{noarch,x86_64}/booth-*-2.* booth.spec:252: E: hardcoded-library-path in /usr/lib/ocf/resource.d/pacemaker/booth-site booth.spec:253: E: hardcoded-library-path in /usr/lib/ocf/lib/booth booth.spec:254: E: hardcoded-library-path in /usr/lib/ocf/lib/booth/geo_attr.sh booth.spec:259: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth booth.spec:260: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/geostore booth.spec:270: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/sharedrsc booth.src:252: E: hardcoded-library-path in /usr/lib/ocf/resource.d/pacemaker/booth-site booth.src:253: E: hardcoded-library-path in /usr/lib/ocf/lib/booth booth.src:254: E: hardcoded-library-path in /usr/lib/ocf/lib/booth/geo_attr.sh booth.src:259: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth booth.src:260: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/geostore booth.src:270: E: hardcoded-library-path in /usr/lib/ocf/resource.d/booth/sharedrsc booth-arbitrator.noarch: W: spelling-error %description -l en_US multi -> mulch, mufti booth-arbitrator.noarch: W: no-documentation booth-site.noarch: W: spelling-error %description -l en_US multi -> mulch, mufti booth-site.noarch: W: only-non-binary-in-usr-lib booth-site.noarch: W: dangling-symlink /usr/sbin/geostore /usr/sbin/boothd booth-test.noarch: W: spelling-error %description -l en_US multi -> mulch, mufti booth-test.noarch: W: only-non-binary-in-usr-lib booth-test.noarch: W: dangling-symlink /usr/share/booth/tests/src/boothd /usr/sbin/boothd booth.x86_64: E: no-binary booth.x86_64: W: no-documentation booth-core.x86_64: W: spelling-error Summary(en_US) executables -> executable, executable s, executrices booth-core.x86_64: W: spelling-error %description -l en_US executables -> executable, executable s, executrices booth-core.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti 7 packages and 1 specfiles checked; 13 errors, 12 warnings. rpmlint is Ok. > FIX: booth-arbitrator should package transaction scripts and require systemd $ rpm -q --requires -p ../RPMS/noarch/booth-arbitrator-1.0-2.eb4256a.git.fc25.noarch.rpm |sort -f | uniq -c 3 /bin/sh 1 booth-core = 1.0-2.eb4256a.git.fc25 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsXz) <= 5.2-1 3 systemd $ rpm -q --scripts -p ../RPMS/noarch/booth-arbitrator-1.0-2.eb4256a.git.fc25.noarch.rpm postinstall scriptlet (using /bin/sh): if [ $1 -eq 1 ] ; then # Initial installation systemctl --no-reload preset booth@.service booth-arbitrator.service >/dev/null 2>&1 || : fi preuninstall scriptlet (using /bin/sh): if [ $1 -eq 0 ] ; then # Package removal, not upgrade systemctl --no-reload disable --now booth@.service booth-arbitrator.service > /dev/null 2>&1 || : fi postuninstall scriptlet (using /bin/sh): if [ $1 -ge 1 ] ; then # Package upgrade, not uninstall systemctl try-restart booth@.service booth-arbitrator.service >/dev/null 2>&1 || : fi Ok. > FIX: Packaging COPYING by "%license %{_pkgdocdir}/COPYING" does not work. $ rpm -q -l -p ../RPMS/x86_64/booth-core-1.0-2.eb4256a.git.fc25.x86_64.rpm /etc/booth /usr/sbin/booth /usr/sbin/booth-keygen /usr/sbin/boothd /usr/share/doc/booth/AUTHORS /usr/share/doc/booth/ChangeLog /usr/share/doc/booth/README /usr/share/doc/booth/booth.conf.example /usr/share/licenses/booth-core /usr/share/licenses/booth-core/COPYING /usr/share/man/man8/booth-keygen.8.gz /usr/share/man/man8/booth.8.gz /usr/share/man/man8/boothd.8.gz Ok. Package builds in F25 (http://koji.fedoraproject.org/koji/taskinfo?taskID=13942807). Ok. Package is good. Resolution: Package APPROVED.
Petře, thanks! Btw. I forgot to mention progress with one on the TODO action items: > TODO: Source files docs/geostore.8.txt and docs/boothd.8.txt and thus > manual pages geostore(8) and boothd(8) still declare GPL+. It would be > great to notify the upstream. There's this PR to address that: https://github.com/ClusterLabs/booth/pull/33
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/booth
booth-1.0-2.eb4256a.git.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2016-deacf31bbe
booth-1.0-2.eb4256a.git.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-8e16102ef8
booth-1.0-2.eb4256a.git.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-a8c84b0711
Thank you, Jon, as well.
booth-1.0-2.eb4256a.git.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-deacf31bbe
booth-1.0-2.eb4256a.git.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-8e16102ef8
booth-1.0-2.eb4256a.git.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-a8c84b0711
booth-1.0-2.eb4256a.git.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.
booth-1.0-2.eb4256a.git.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.
booth-1.0-2.eb4256a.git.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.