Bug 985967 - Review Request: python-arc - Autotest RPC Client libraries and tools
Review Request: python-arc - Autotest RPC Client libraries and tools
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-18 11:36 EDT by Cleber Rosa
Modified: 2015-07-21 08:29 EDT (History)
5 users (show)

See Also:
Fixed In Version: python-arc-0.7.1-1.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-02-03 21:44:46 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tcallawa: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Cleber Rosa 2013-07-18 11:36:39 EDT
Spec URL: https://raw.github.com/clebergnu/arc/master/python-arc.spec
SRPM URL: http://www.tallawa.org/python-arc-0.2.0-1.fc20.src.rpm
Description: Autotest RPC Client is a pure Python library that servers as the client part of Autotest's server interface. It aims to be portable and only depend on Python's Standard Library.
Fedora Account System Username: cleber
Comment 1 Cleber Rosa 2013-07-18 11:52:56 EDT
This is very first version of the package submitted to Fedora.
Comment 2 Christopher Meng 2013-07-19 01:39:09 EDT
You forgot to block needsponsor. No problem.

I honestly think that you can drop this:

%define shortname arc
%if ! (0%{?fedora} > 12 || 0%{?rhel} > 5)
%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
%endif

If you are still interested in EPEL5.

And a question is here, will you push the package to a very old system?

I'm waiting for your answer(this will affect the spec issue.)

Thanks.
Comment 3 Eduardo Echeverria 2013-07-21 04:47:13 EDT
Hi clever:

%if ! (0%{?fedora} > 12 || 0%{?rhel} > 5)
%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
%endif isn't neccesary in newest versions of Fedora, please remove from the spec.

Please provide the full url in Source0 (https://github.com/clebergnu/arc) , for this, handle the url following the recommendations exposed in https://fedoraproject.org/wiki/Packaging:SourceURL#Github

Use in BuildRequires: python2-devel instead of python >= 2.7, see http://fedoraproject.org/wiki/Packaging:Python#BuildRequires

Requires: python >= 2.7

- %clean isn't needed
- BuildRoot isn't needed
- cleaning of buildroot in %install isn't needed
- %defattr is not needed
all this only are necessary for el5, see https://fedoraproject.org/wiki/EPEL:Packaging

the version of python in el5 is 2.4, and in el6 is 2.6, afaik, your package needs 2.7, so el5/6 not are valid versions for your package, Just for curiosity,since you are the programmer, don't works in python2.6?

rpmlint out: 

python-arc.noarch: E: description-line-too-long C Arc is the Autotest RPC Client, a library and command line API for controlling an Autotest RPC Server.

- please not exceed your description of the 80 characters per line

python-arc.noarch: W: no-documentation
Add license, README, etc in %doc (btw, not is included in your spec)
 
python-arc.noarch: E: incorrect-fsf-address /usr/lib/python2.7/site-packages/arc/jsonrpc.py
the license header haven't a fsf updated address, that would a minor problem, if it were not because also part of a library from another project, this in fedora have a clear policy, see  https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

please fix this permissions
python-arc.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/arc/connection_unittest.py 0644L /usr/bin/env
python-arc.noarch: E: wrong-script-interpreter /usr/lib/python2.7/site-packages/arc/cli/app.py /usr/bin/env/python
python-arc.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/arc/cli/app.py 0644L /usr/bin/env/python
python-arc.noarch: W: no-manual-page-for-binary arcli

A question, why don't build the documentation of the package?
Comment 4 Eduardo Echeverria 2013-07-21 04:51:36 EDT
oh i forgot, remove the bundled egg in section %prep, not in %install
Comment 5 Cleber Rosa 2013-07-22 08:48:29 EDT
(In reply to Christopher Meng from comment #2)
> You forgot to block needsponsor. No problem.

Sorry about this.

> 
> I honestly think that you can drop this:
> 
> %define shortname arc
> %if ! (0%{?fedora} > 12 || 0%{?rhel} > 5)
> %{!?python_sitelib: %global python_sitelib %(%{__python} -c "from
> distutils.sysconfig import get_python_lib; print(get_python_lib())")}
> %endif
> 
> If you are still interested in EPEL5.
> 
> And a question is here, will you push the package to a very old system?
> 
> I'm waiting for your answer(this will affect the spec issue.)
> 
> Thanks.

No, this package can not make into EPEL5 or even EPEL6 because of Python version requirements.

I'll remove this block from the spec.
Comment 6 Cleber Rosa 2013-07-22 09:32:11 EDT
(In reply to Eduardo Echeverria from comment #3)
> Hi clever:

Thanks for calling me "clever", but judging from the amount of mistakes here I wouldn't call myself that :)

> 
> %if ! (0%{?fedora} > 12 || 0%{?rhel} > 5)
> %{!?python_sitelib: %global python_sitelib %(%{__python} -c "from
> distutils.sysconfig import get_python_lib; print(get_python_lib())")}
> %endif isn't neccesary in newest versions of Fedora, please remove from the
> spec.
> 

As suggested earlier, I'm removing this block altogether.

> Please provide the full url in Source0 (https://github.com/clebergnu/arc) ,
> for this, handle the url following the recommendations exposed in
> https://fedoraproject.org/wiki/Packaging:SourceURL#Github

Is is possible to only use the version tag for the Source URL, such as:

Source0: https://github.com/clebergnu/%{shortname}/archive/v%{version}.tar.gz

Instead of the commit hash? Since I plan to also host the Fedora spec file on the upstream repo, this would create an "egg and chicken" kind of problem.

> 
> Use in BuildRequires: python2-devel instead of python >= 2.7, see
> http://fedoraproject.org/wiki/Packaging:Python#BuildRequires
> 
> Requires: python >= 2.7

OK! Looks like I got away with depending only on python (instead of -devel) because I included the site lib macro.

> 
> - %clean isn't needed

OK

> - BuildRoot isn't needed

OK

> - cleaning of buildroot in %install isn't needed

OK

> - %defattr is not needed

OK

> all this only are necessary for el5, see
> https://fedoraproject.org/wiki/EPEL:Packaging
> 
> the version of python in el5 is 2.4, and in el6 is 2.6, afaik, your package
> needs 2.7, so el5/6 not are valid versions for your package, Just for
> curiosity,since you are the programmer, don't works in python2.6?

No, it's designed to work with python 2.7 only. The reason is that I took care to also make it source compatible with python 3. At a later time I want to also make python 3 packages out of this same spec.

> 
> rpmlint out: 
> 
> python-arc.noarch: E: description-line-too-long C Arc is the Autotest RPC
> Client, a library and command line API for controlling an Autotest RPC
> Server.
> 
> - please not exceed your description of the 80 characters per line

New description:

Arc is the Autotest RPC Client. It provides libraries and tools that interact
with an Autotest RPC Server. It allows one to send test jobs, add test hosts,
query available tests, etc.

> 
> python-arc.noarch: W: no-documentation
> Add license, README, etc in %doc (btw, not is included in your spec)

OK. BTW, is it OK to use "README.md" as the "official README"?

>  
> python-arc.noarch: E: incorrect-fsf-address
> /usr/lib/python2.7/site-packages/arc/jsonrpc.py
> the license header haven't a fsf updated address, that would a minor
> problem, if it were not because also part of a library from another project,
> this in fedora have a clear policy, see 
> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

Since this is a "library" with about 60 lines of code that was modified for arc's requirements, and it's not packaged in Fedora, I assume it's OK to "bundle" it, but update the FSF address to remove this noise. Right?

> 
> please fix this permissions
> python-arc.noarch: E: non-executable-script
> /usr/lib/python2.7/site-packages/arc/connection_unittest.py 0644L

This file should not have made into the SRPM because it should not have made into the tarball in the first place. I was generating the tarball with "python setup.py sdist", but I'll now use the github generated tarball to avoid shipping files that are not tracked upstream.

> /usr/bin/env
> python-arc.noarch: E: wrong-script-interpreter

Fixed.

> /usr/lib/python2.7/site-packages/arc/cli/app.py /usr/bin/env/python
> python-arc.noarch: E: non-executable-script
> /usr/lib/python2.7/site-packages/arc/cli/app.py 0644L /usr/bin/env/python

Fixed.

> python-arc.noarch: W: no-manual-page-for-binary arcli
> 
> A question, why don't build the documentation of the package?

That's a good question. I'll include the docs and add the arcli manpage.
Comment 7 Eduardo Echeverria 2013-07-23 16:17:18 EDT
(In reply to Cleber Rodrigues from comment #6)
> (In reply to Eduardo Echeverria from comment #3)
> > Hi clever:
> 
> Thanks for calling me "clever", but judging from the amount of mistakes here
> I wouldn't call myself that :)

Hi Cleber, have mistakes doesn't make less "clever" ;)


> > Please provide the full url in Source0 (https://github.com/clebergnu/arc) ,
> > for this, handle the url following the recommendations exposed in
> > https://fedoraproject.org/wiki/Packaging:SourceURL#Github
> 
> Is is possible to only use the version tag for the Source URL, such as:
> 
> Source0: https://github.com/clebergnu/%{shortname}/archive/v%{version}.tar.gz
> 
> Instead of the commit hash? Since I plan to also host the Fedora spec file
> on the upstream repo, this would create an "egg and chicken" kind of problem.

No, isn't possible, precisely this guideline contemplates the need of a hash for identify the commit that you are packing, i cite the guideline "For a number of reasons (immutability, availability, uniqueness), you must use the full commit revision hash when referring to the sources"

This can be solved:
- You can make a version of the spec on-demand generated by a makefile target in the upstream repo, doing a snapshot automatically. FYI, this method isn't valid for fedora

> > 
> > Use in BuildRequires: python2-devel instead of python >= 2.7, see
> > http://fedoraproject.org/wiki/Packaging:Python#BuildRequires
> > 
> > Requires: python >= 2.7
> 
For build a package made in python, you should have as BR python{2,3}-devel depending of the implementation (in this case python2-devel).
in the Requires isn't neccesary use explicit versioning since that the system-wide Fedora version is 2.7 

➜  ~  repoquery -qf python
python-0:2.7.5-1.fc19.x86_64
python-0:2.7.5-3.fc19.x86_64
python-0:2.7.5-3.fc19.i686
python-0:2.7.5-1.fc19.i686


> > 
> > rpmlint out: 
> > 
> > python-arc.noarch: W: no-documentation
> > Add license, README, etc in %doc (btw, not is included in your spec)
> 
> OK. BTW, is it OK to use "README.md" as the "official README"?

Yes, i don't see any problem.

> >  
> > python-arc.noarch: E: incorrect-fsf-address
> > /usr/lib/python2.7/site-packages/arc/jsonrpc.py
> > the license header haven't a fsf updated address, that would a minor
> > problem, if it were not because also part of a library from another project,
> > this in fedora have a clear policy, see 
> > https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
> 
> Since this is a "library" with about 60 lines of code that was modified for
> arc's requirements, and it's not packaged in Fedora, I assume it's OK to
> "bundle" it, but update the FSF address to remove this noise. Right?
> 

What would happen if that project ends up being packaged in Fedora? 
Can you commit your changes in upstream? 
Are your changes a deviation of the original project? 


Best Regards
Comment 8 Cleber Rosa 2013-07-25 10:22:02 EDT
(In reply to Eduardo Echeverria from comment #7)
> (In reply to Cleber Rodrigues from comment #6)
> > (In reply to Eduardo Echeverria from comment #3)
> > > Hi clever:
> > 
> > Thanks for calling me "clever", but judging from the amount of mistakes here
> > I wouldn't call myself that :)
> 
> Hi Cleber, have mistakes doesn't make less "clever" ;)
> 
> 
> > > Please provide the full url in Source0 (https://github.com/clebergnu/arc) ,
> > > for this, handle the url following the recommendations exposed in
> > > https://fedoraproject.org/wiki/Packaging:SourceURL#Github
> > 
> > Is is possible to only use the version tag for the Source URL, such as:
> > 
> > Source0: https://github.com/clebergnu/%{shortname}/archive/v%{version}.tar.gz
> > 
> > Instead of the commit hash? Since I plan to also host the Fedora spec file
> > on the upstream repo, this would create an "egg and chicken" kind of problem.
> 
> No, isn't possible, precisely this guideline contemplates the need of a hash
> for identify the commit that you are packing, i cite the guideline "For a
> number of reasons (immutability, availability, uniqueness), you must use the
> full commit revision hash when referring to the sources"
> 
> This can be solved:
> - You can make a version of the spec on-demand generated by a makefile
> target in the upstream repo, doing a snapshot automatically. FYI, this
> method isn't valid for fedora

OK, I think I'll handle this by having a branch/tag with the commit changes to the spec file only. Say, for the 1.0.0 release, I would have:

v1.0.0
v1.0.0-spec

The diff between the two would simply be the "%global commit ..." line.

> 
> > > 
> > > Use in BuildRequires: python2-devel instead of python >= 2.7, see
> > > http://fedoraproject.org/wiki/Packaging:Python#BuildRequires
> > > 
> > > Requires: python >= 2.7
> > 
> For build a package made in python, you should have as BR python{2,3}-devel
> depending of the implementation (in this case python2-devel).
> in the Requires isn't neccesary use explicit versioning since that the
> system-wide Fedora version is 2.7 
> 
> ➜  ~  repoquery -qf python
> python-0:2.7.5-1.fc19.x86_64
> python-0:2.7.5-3.fc19.x86_64
> python-0:2.7.5-3.fc19.i686
> python-0:2.7.5-1.fc19.i686
> 

OK, got it!

> 
> > > 
> > > rpmlint out: 
> > > 
> > > python-arc.noarch: W: no-documentation
> > > Add license, README, etc in %doc (btw, not is included in your spec)
> > 
> > OK. BTW, is it OK to use "README.md" as the "official README"?
> 
> Yes, i don't see any problem.
> 
> > >  
> > > python-arc.noarch: E: incorrect-fsf-address
> > > /usr/lib/python2.7/site-packages/arc/jsonrpc.py
> > > the license header haven't a fsf updated address, that would a minor
> > > problem, if it were not because also part of a library from another project,
> > > this in fedora have a clear policy, see 
> > > https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
> > 
> > Since this is a "library" with about 60 lines of code that was modified for
> > arc's requirements, and it's not packaged in Fedora, I assume it's OK to
> > "bundle" it, but update the FSF address to remove this noise. Right?
> > 
> 
> What would happen if that project ends up being packaged in Fedora? 
> Can you commit your changes in upstream? 
> Are your changes a deviation of the original project? 

I've decided to rewrite this module to remove this problem and make the code more in line with the general code style of the project.

> 
> 
> Best Regards

Thank you for the review/help so far!
Comment 9 Cleber Rosa 2013-07-25 10:51:07 EDT
Updated versions:

Spec URL: https://raw.github.com/clebergnu/arc/master/python-arc.spec
SRPM URL: http://www.tallawa.org/python-arc-0.3.0-1.fc19.src.rpm

pylint now generates 2 warnings:

python-arc.noarch: W: spelling-error Summary(en_US) Autotest -> Auto test, Auto-test, Astutest
python-arc.noarch: W: no-manual-page-for-binary arcli

The first one can be ignored, and I'm planning to address the second (man page) in the next. The reason is that I want to have automatically generated man pages because arcli is already mostly self documented. Example:

$ arcli  host --help
usage: arcli host [-h] (-l | -j | -a | -d | -L | -U | -r) [-n NAME] [-i ID]

optional arguments:
  -h, --help            show this help message and exit
  -n NAME, --name NAME  name (usually the FQDN) of the host to manipulated
  -i ID, --id ID        numeric identification of the host to manipulated

ACTION:
  Action to be performed

  -l, --list-brief      list all records briefly
  -j, --list-jobs       list the jobs running on the listed hosts
  -a, --add             add a new entry
  -d, --delete          delete an existing entry
  -L, --lock            locks the host (make it unavailable to new jobs)
  -U, --unlock          unlocks the host (make it available to new jobs)
  -r, --reverify        schedules a reverification for the host

Is missing the man page a blocker?
Comment 10 Eduardo Echeverria 2013-07-25 11:57:13 EDT
(In reply to Cleber Rodrigues from comment #9)
> Updated versions:
> Is missing the man page a blocker?

No, isn't blocker, is a "SHOULD" in our review guidelines, however your package is a CLI, so, have sense to have a man page; anyway can be addressed later.

Now, I would like that you can be involved in some informal reviews at other packagers, doing that; would be happy to sponsor you

Best Regards
Comment 11 Eduardo Echeverria 2013-07-25 12:05:17 EDT
btw, Where is the license of the package and the documentation?
Comment 12 Cleber Rosa 2013-09-19 17:43:03 EDT
Hello,

It's been a while, and a couple of upstream versions later, here I am with an improved version of the package:

Spec URL: https://raw.github.com/autotest/arc/master/python-arc.spec
SRPM URL: http://www.tallawa.org/python-arc-0.5.0-1.fc19.src.rpm

It now features complete documentation, including a man page.

Now I'm up to the "informal package review"!

Thanks a lot for the time you took here and the great feedback.
Comment 13 Till Maas 2013-10-21 15:37:27 EDT
- The file "LICENSE" is missing from %doc
- Why is there 'Requires: python' - it should not be needed
- Please use %{__python2} instead of %{__python}:
http://fedoraproject.org/wiki/Packaging:Python#Macros

As written in comment:10, please perform informal reviews of other packages and post links to them here, i.e. review other package submissions to show that you know the guidelines. If you have further questions, feel free to ask.
Comment 14 Cleber Rosa 2013-11-21 00:20:08 EST
A couple of packages I have done reviews:

 - https://bugzilla.redhat.com/show_bug.cgi?id=579925
 - https://bugzilla.redhat.com/show_bug.cgi?id=826037

I'm working on a couple more.
Comment 15 Cleber Rosa 2013-12-05 13:20:17 EST
Another package review:

 - https://bugzilla.redhat.com/show_bug.cgi?id=1013363
Comment 16 Cleber Rosa 2013-12-05 13:22:27 EST
Hello,

I've collected a package fixes and general project fixes into another upstream release:

Spec URL: https://raw.github.com/autotest/arc/master/python-arc.spec
SRPM URL: http://www.tallawa.org/python-arc-0.6.0-1.fc19.src.rpm

Thanks.
Comment 17 Lucas Meneghel Rodrigues 2014-01-14 11:25:59 EST
Guys, I wonder what is going on here. It's been months that my team is trying to become package maintainers for autotest-framework, and Cleber has done everything that was asked.
Comment 18 Christopher Meng 2014-01-14 11:36:14 EST
(In reply to Lucas Meneghel Rodrigues from comment #17)
> Guys, I wonder what is going on here. It's been months that my team is
> trying to become package maintainers for autotest-framework, and Cleber has
> done everything that was asked.

You can find some person of RH who can sponsor new people.
Comment 19 Tom "spot" Callaway 2014-01-21 10:25:21 EST
== Review ==

- rpmlint checks return:

python-arc.src: W: spelling-error Summary(en_US) Autotest -> Auto test, Auto-test, Astutest
python-arc.noarch: W: spelling-error Summary(en_US) Autotest -> Auto test, Auto-test, Astutest
python-arc.noarch: W: hidden-file-or-dir /usr/share/doc/python-arc/api/.buildinfo
python-arc.noarch: W: manual-page-warning /usr/share/man/man1/arcli.1.gz 39: warning: macro `..' not defined
2 packages and 0 specfiles checked; 0 errors, 4 warnings.

All safe to ignore, although, you might want to look at fixing those latter two items at some point. (the .buildinfo file is probably extraneous and the man page probably needs a minor cleanup)

- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2) OK, text in %doc
- spec file legible, in am. english
- source matches upstream (9907afa0e840b292a20d8bd4c07345cbfef6aa7ea91ec3ee6a2e8fb77fc2ce19)
- package compiles on f20 (noarch)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 

== Non-Blocker Items ==
* There is a lack of proper license attribution in the source. Please include some sort of per-file license attribution in the source files, like this:

"""
Copyright (C) 2014 John Doe <jdoe@redhat.com>
License: GPLv2 (see LICENSE for details)
"""

At the very least, please include some text which indicates the license (and version of the license) in README.md. 

This is not technically a review blocker, but since you're also the upstream here, I'm pointing this out as something you should do.

== Blocker Items ==
* Please use %{__python2} instead of %{__python} (see: https://fedoraproject.org/wiki/Packaging:Python#Macros)


Fix that blocker (and the non-blocker item, please) and I will approve this package and sponsor you.
Comment 20 Cleber Rosa 2014-01-21 12:10:24 EST
Tom,

Thanks very much for the review. I'm working on the blocker (and other items too) and will updated ASAP.

Thanks again!
Comment 21 Cleber Rosa 2014-01-23 08:38:29 EST
Tom,

Here's a new SPEC and SRPM with the blocker item resolved:

* Spec URL: https://raw.github.com/autotest/arc/master/python-arc.spec
* SRPM URL: http://www.tallawa.org/python-arc-0.7.0-1.fc20.src.rpm

The other issues are being tracked on the upstream issue tracker:

* https://github.com/autotest/arc/issues/5

Thanks again for your help, and hope to push this to Fedora ASAP!
Comment 22 Cleber Rosa 2014-01-23 09:45:47 EST
And a minor release with an upstream (setup.py) packaging bugfix[1]:

* Spec URL: https://raw.github.com/autotest/arc/master/python-arc.spec
* SRPM URL: http://www.tallawa.org/python-arc-0.7.1-1.fc20.src.rpm

[1] - https://github.com/autotest/arc/commit/743779de63bcea6f1d539ac9b0b6043794e517fb
Comment 23 Tom "spot" Callaway 2014-01-23 11:40:26 EST
This package is approved, thanks for the quick fix (and don't forget those other two items that rpmlint saw). You seem to already be sponsored, so I don't need to do anything there.
Comment 24 Cleber Rosa 2014-01-23 12:26:25 EST
New Package SCM Request
=======================
Package Name: python-arc
Short Description: Arc is the Autotest RPC Client.
Owners: cleber
Branches: f20 epel7
InitialCC:
Comment 25 Jon Ciesla 2014-01-23 13:00:43 EST
Git done (by process-git-requests).
Comment 26 Fedora Update System 2014-01-23 14:19:40 EST
python-arc-0.7.1-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-arc-0.7.1-1.fc20
Comment 27 Fedora Update System 2014-01-24 21:24:51 EST
python-arc-0.7.1-1.fc20 has been pushed to the Fedora 20 testing repository.
Comment 28 Fedora Update System 2014-02-03 21:44:46 EST
python-arc-0.7.1-1.fc20 has been pushed to the Fedora 20 stable repository.

Note You need to log in before you can comment on or make changes to this bug.