This is very first version of the package submitted to Fedora.

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.

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?

oh i forgot, remove the bundled egg in section %prep, not in %install

(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.

(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.

(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

(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!

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?

(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

btw, Where is the license of the package and the documentation?

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.

- 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.

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.

Another package review:

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

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.

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.

(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.

== 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>
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.

Tom,

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

Thanks again!

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!

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

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.

New Package SCM Request
=======================
Package Name: python-arc
Short Description: Arc is the Autotest RPC Client.
Owners: cleber
Branches: f20 epel7
InitialCC:

Git done (by process-git-requests).

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

python-arc-0.7.1-1.fc20 has been pushed to the Fedora 20 testing repository.

python-arc-0.7.1-1.fc20 has been pushed to the Fedora 20 stable repository.