Bug 883472

Not a full review, just some comments:


> # Turn off the brp-python-bytecompile script
> %global __os_install_post %(echo '%{__os_install_post}' |
> sed -e 's!/usr/lib[^[:space:]]*/brp-python-bytecompile[[:space:]].*$!!g')

As package spec files are like source code, it would be good to add the comment _why_ you turn off the byte-compilation. You do explain that in the review request, but not in the spec file.


> Requires:   python2 >= 2.6

Such an explicit dependency doesn't work well, since there is an automatic dependency on a specific python(abi) version. This explicit one would be inaccurate and superfluous.

http://fedoraproject.org/wiki/Packaging:Python


> BuildRequires:	python2-devel >= 2.6, python3-devel, systemd-units

Both Python versions? The spec file doesn't handle that.


> %{python_sitelib}/%{name}/__init__.*
> %{python_sitelib}/%{name}/Common/*
> %{python_sitelib}/%{name}/Controller/*
> %{_datadir}/%{name}/*
> %{python_sitelib}/%{name}/Slave/*

These cause several "unowned" directories:
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

/bin/rpmls is very convenient for listing package contents (and an alternative to rpm -qlv …).


> %{_mandir}/man1/%{name}-ctl.1.gz
> %{_mandir}/man1/%{name}-slave.1.gz

Many reviewers here point out that it may be more future-proof/versatile to use a wildcard to allow for a changed/reconfigured compression technique:
 
  %{_mandir}/man1/%{name}-ctl.1.*
  %{_mandir}/man1/%{name}-slave.1.*

(In reply to comment #1)
> > # Turn off the brp-python-bytecompile script
> > %global __os_install_post %(echo '%{__os_install_post}' |
> > sed -e 's!/usr/lib[^[:space:]]*/brp-python-bytecompile[[:space:]].*$!!g')
> 
> As package spec files are like source code, it would be good to add the
> comment _why_ you turn off the byte-compilation. You do explain that in the
> review request, but not in the spec file.

You're right, it should be explained in the spec file as well. I added the comment.

> > Requires:   python2 >= 2.6
> 
> Such an explicit dependency doesn't work well, since there is an automatic
> dependency on a specific python(abi) version. This explicit one would be
> inaccurate and superfluous.
> 
> http://fedoraproject.org/wiki/Packaging:Python

Oh, I'm sorry, I must have missed that. I changed it to require python2 only.

> > BuildRequires:	python2-devel >= 2.6, python3-devel, systemd-units
> 
> Both Python versions? The spec file doesn't handle that.

I had to include python3-devel for the py_byte_compile macro. It is not used
anything else. I might have missed here something too. Is there any way to
get the macro without the necessity to require python3-devel?

> These cause several "unowned" directories:
> https://fedoraproject.org/wiki/Packaging:UnownedDirectories
> 

> > %{_mandir}/man1/%{name}-ctl.1.gz
> > %{_mandir}/man1/%{name}-slave.1.gz
> 
> Many reviewers here point out that it may be more future-proof/versatile to
> use a wildcard to allow for a changed/reconfigured compression technique:

I fixed these as well.

Thank you very much for your comments! I really appreciate your help.
I fixed them and will repost the updated spec and srcrpm shortly.

Reposting the SRCRPM and SPEC with fixes of the mistakes Michael pointed out.
I also added proper handling of the systemd service for lnst-slave that was
missing in the previous version.

Spec URL: http://www.stud.fit.vutbr.cz/~xpazde00/soubory/v2/lnst.spec
SRPM URL: http://www.stud.fit.vutbr.cz/~xpazde00/soubory/v2/lnst-0.1-2.20121204git.fc19.src.rpm

--- koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4835856

--- rpmlint output:
$ rpmlint lnst.spec lnst-0.1-2.20121204git.fc19.src.rpm lnst-common-0.1-2.20121204git.fc19.noarch.rpm lnst-ctl-0.1-2.20121204git.fc19.noarch.rpm lnst-slave-0.1-2.20121204git.fc19.noarch.rpm 
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/sockopt_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/max_groups.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/parameters_igmp.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/igmp_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_source_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_source_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_block_source.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/multicast_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/client/send_simple.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_block_source.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/parameters_multicast.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_ttl.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/client/send_igmp_query.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_simple.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_loop.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_if.c
4 packages and 1 specfiles checked; 0 errors, 18 warnings.

These warnings are explained in the original post above.

Python byte-compiling macros are included in the rpm-build package, too, so you don't need to pull in python3-devel. Usage example:

  %include %{_rpmconfigdir}/macros.python
  %py_ocomp %{buildroot}%{python_sitelib}

Alright :). I removed the dependency on python3-devel and used the macros instead. Thanks again!

Spec URL: http://www.stud.fit.vutbr.cz/~xpazde00/soubory/v3/lnst.spec
SRPM URL: http://www.stud.fit.vutbr.cz/~xpazde00/soubory/v3/lnst-0.1-3.20121204git.fc19.src.rpm

--- koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4855315

--- rpmlint output:
rpmlint lnst.spec lnst-0.1-3.20121204git.fc19.src.rpm lnst-common-0.1-3.20121204git.fc19.noarch.rpm lnst-ctl-0.1-3.20121204git.fc19.noarch.rpm lnst-slave-0.1-3.20121204git.fc19.noarch.rpm 
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/sockopt_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/max_groups.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/igmp_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_source_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/parameters_igmp.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_source_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_block_source.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/multicast_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/client/send_simple.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_block_source.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/parameters_multicast.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_ttl.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/client/send_igmp_query.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_simple.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_loop.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_if.c
4 packages and 1 specfiles checked; 0 errors, 18 warnings.

These warnings are explained in the original review request above.

Since most of the work seems to be done, I'll take this for a reivew :)

- Why is the main package empty and doesn't get built? Wouldn't it make more sense to move the files from the -common subpackage to the main package?
- AFAICS the license is GPLv2+
- Just to say it here, the .c and .h files are not in the -devel subpackage because they are needed for "runtime" of the package, is that correct?
- According to [1], it seems that your package is a prerelease and therefore the release tag should be "0.x" instead of "x" (while keeping the git stuff, of course).

[1] https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

(In reply to comment #7)
> - Why is the main package empty and doesn't get built? Wouldn't it make more
> sense to move the files from the -common subpackage to the main package?

I used this to indicate that the things currently lnst-common alone are
practically useless without either lnst-ctl or lnst-slave. Installing "lnst"
pacakge only might lead someone to to think that it is sufficient to get
everything (which is not).

But if this is not the preferred way, I can move the -common to the main
package.

> - AFAICS the license is GPLv2+

You're right. I'm sorry for the confusion. Will fix it.

> - Just to say it here, the .c and .h files are not in the -devel subpackage
> because they are needed for "runtime" of the package, is that correct?

That is correct. It is sort-of library of test cases (very small at the
moment - the project is young) that the controller will distribute to its
slaves to compile and execute remotely.

> - According to [1], it seems that your package is a prerelease and therefore
> the release tag should be "0.x" instead of "x" (while keeping the git stuff,
> of course).

I must have missed that as well. I'll fix it.

> 
> [1]
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-
> Release_packages

Thank you very much for taking this review :-)! I will re-post the package
after we agree whether to use -common or not.

(In reply to comment #8)
> (In reply to comment #7)
> > - Why is the main package empty and doesn't get built? Wouldn't it make more
> > sense to move the files from the -common subpackage to the main package?
> 
> I used this to indicate that the things currently lnst-common alone are
> practically useless without either lnst-ctl or lnst-slave. Installing "lnst"
> pacakge only might lead someone to to think that it is sufficient to get
> everything (which is not).
> 
> But if this is not the preferred way, I can move the -common to the main
> package.

I'd say that having lnst package with the files from common and proper documentation saying what's needed would suffice here. The problem with practically useless package will always be there, you just transferred it to the subpackage :)
Does that make sense?

> Thank you very much for taking this review :-)! I will re-post the package
> after we agree whether to use -common or not.

Ok.

(In reply to comment #9)
> I'd say that having lnst package with the files from common and proper
> documentation saying what's needed would suffice here. The problem with
> practically useless package will always be there, you just transferred it to
> the subpackage :)
> Does that make sense?

You're right, you would still be able to install just lnst-common alone. I just
thought marking it in some way would somehow force people to actually look into
the documentation and read what they should install :).

Would it be acceptable to use the base package, but rename it to something like
lnst-libs or lnst-common?

(In reply to comment #10)
> (In reply to comment #9)
> > I'd say that having lnst package with the files from common and proper
> > documentation saying what's needed would suffice here. The problem with
> > practically useless package will always be there, you just transferred it to
> > the subpackage :)
> > Does that make sense?
> 
> You're right, you would still be able to install just lnst-common alone. I
> just
> thought marking it in some way would somehow force people to actually look
> into
> the documentation and read what they should install :).
> 
> Would it be acceptable to use the base package, but rename it to something
> like
> lnst-libs or lnst-common?

Do you mean renaming the whole package? I don't really think that's wise, as people would think it is a subpackage and search for the main package "lnst". I'd advise using lnst (for what is now lnst-common) and keeping the other two subpackages.

Ok, I removed -common and put its contents to the base package, changed the license and fixed the release tag. I also fixed the byte compilation, which
didn't work previously.

Thank you for your patience :-).

Spec URL: http://www.stud.fit.vutbr.cz/~xpazde00/soubory/v4/lnst.spec
SRPM URL: http://www.stud.fit.vutbr.cz/~xpazde00/soubory/v4/lnst-0.1-0.4.20121204git.fc19.src.rpm

--- koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4870130

--- rpmlint output:
$ rpmlint lnst-0.1-0.4.20121204git.fc19.src.rpm lnst-0.1-0.4.20121204git.fc19.noarch.rpm lnst-ctl-0.1-0.4.20121204git.fc19.noarch.rpm lnst-slave-0.1-0.4.20121204git.fc19.noarch.rpm 
lnst.src: W: spelling-error Summary(en_US) ctl -> ct, cl, cal
lnst.noarch: W: spelling-error Summary(en_US) ctl -> ct, cl, cal
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/sockopt_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/max_groups.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/parameters_igmp.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/igmp_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_source_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_source_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_block_source.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_membership.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/multicast_utils.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/client/send_simple.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_block_source.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/parameters_multicast.h
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_ttl.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/client/send_igmp_query.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/server/recv_simple.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_loop.c
lnst-ctl.noarch: W: devel-file-in-non-devel-package /usr/share/lnst/test_tools/multicast/offline/sockopt_if.c
4 packages and 0 specfiles checked; 0 errors, 20 warnings.

There are two more warnings this time -- spelling issues with the word "ctl"
in the summary of the base package. "lnst-ctl" is the name of one of the
subpackages.

Good, I'm completely satisfied now :)

This package is APPROVED.

New Package SCM Request
=======================
Package Name: lnst
Short Description: Framework for performing network tests
Owners: rpazdera
Branches: f18
InitialCC:

Bohuslav, please set the review flag to +.

I'm sorry, I must have accidentally reset the flag back to ? :(.

Done. Please set the fedora-cvs to ? instead :)

New Package SCM Request
=======================
Package Name: lnst
Short Description: Framework for performing network tests
Owners: rpazdera
Branches: f18
InitialCC:

Git done (by process-git-requests).

lnst-0.1-0.4.20121204git.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/lnst-0.1-0.4.20121204git.fc18

lnst-0.1-0.4.20121204git.fc18 has been pushed to the Fedora 18 testing repository.

lnst-0.1-0.4.20121204git.fc18 has been pushed to the Fedora 18 stable repository.

New Package SCM Request
=======================
Package Name: lnst
Short Description: Framework for performing network tests
Owners: jirka
Branches: el6 el7
InitialCC:

please ignore comment 23

Ignoring per comment 24.

Jon, Comment 24 is valid request. Comment 23 should be ignored (set RH private so it might not be visible to you). Sorry for the confusion.

Just to make sure, once again:
New Package SCM Request
=======================
Package Name: lnst
Short Description: Framework for performing network tests
Owners: jirka
Branches: el6 el7
InitialCC:

Ah, understood.  However, this is not a new pacakge, but new branches, so it
should be a Package Change Request instead.

Package Change Request
======================
Package Name: lnst
New Branches: el6 epel7
Owners: jirka

Git done (by process-git-requests).