https://bugzilla.redhat.com/show_bug.cgi?id=1314865
--- Comment #6 from Jan Pokorný <jpokorny(a)redhat.com> ---
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....
re [comment 3]:
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