Bug 752829

This is my first package and I am seeking a sponsor.

A first pass just looking at the .spec file:

1) The Source0: can't be located see:
   http://fedoraproject.org/wiki/Packaging:SourceURL

2) EGEE is I'm fairly positive an approved license 

3) %setup is not -q'd

4) Changelog version numbers don't match

5) Vendor must never be present.

6) The INSTALLED_FILES trick with setup tools is unusual. There
   are some setuptools examples on the python guidelines page:
 http://fedoraproject.org/wiki/Packaging:Python


Please pass your packages through rpmlint and reduce what it finds to
a minimum. It will have found all of the above I expect.

Become familiar with 
http://fedoraproject.org/wiki/Packaging:Guidelines

and then provide new .spec and .src.rpm

For the purposes of being sponsored then preferably submit at least
another package for review and provide informal review or two to other pending reviews.
Grab a pending review from here:
http://fedoraproject.org/PackageReviewStatus/NEW.html
Once you have done a review or two leave a link to it in this bug.

Steve.

(In reply to comment #2)
> 2) EGEE is I'm fairly positive an approved license 
EGEE is I'm fairly positive NOT an approved license

I have addressed the issue mentioned. Here is the new version

Spec URL: http://lfield.web.cern.ch/lfield/glue-validator.spec
SRPM URL: http://lfield.web.cern.ch/lfield/glue-validator-1.0.0-1.el5.src.rpm

Another package from Laurence: bug #754137

First informal reviews of other packages:

https://bugzilla.redhat.com/show_bug.cgi?id=749562#c1

More detailed review of the comoonics-base-py package

https://bugzilla.redhat.com/show_bug.cgi?id=749562#c2

Another informal review, this time for abootimg

https://bugzilla.redhat.com/show_bug.cgi?id=734410#c1

Fails to build with mock on EPEL6, I assume you are targeting EPEL6.

equires: /usr/bin/python python(abi) = 2.6
Checking for unpackaged file(s): /usr/lib/rpm/check-files /builddir/build/BUILDROOT/glue-validator-1.0.0-1.el6.x86_64
error: Installed (but unpackaged) file(s) found:
   /usr/lib/python2.6/site-packages/glue_validator-1.0.0-py2.6.egg-info



I've added a whiteboard entry above 

http://fedoraproject.org/wiki/Package_Review_Process#The_Whiteboard

be sure to remove it once the package builds.

This file is not created in RHEL 5 but is created on RHEL 6. The specfile has been updated according to this example.

http://fedoraproject.org/wiki/Packaging:Python_Eggs#Providing_Eggs_using_Setuptools

Same , please provide new links to .spec file and .src.rpm with a new release number at least.

This is done at every iteration.

Steve.

It's a good idea generally to execute and provide links to scratch builds
in koji to confirm that this builds on .el5, .el6, f16, rawhide at least.

Here is a new version with and increased release number.

Spec URL: http://lfield.web.cern.ch/lfield/glue-validator.spec
SRPM URL: http://lfield.web.cern.ch/lfield/glue-validator-1.0.0-2.el5.src.rpm

Here is a link to the mock build. 

https://koji.afroditi.hellasgrid.gr/koji/taskinfo?taskID=23873

Please use the redhat koji instance for reviews. There is no guarantee that this koji has the "correct" configuration. You have already all the permissions you need to do so.  Also that koji is not public it appears.

Build targets: dist-5E-epel , dist-6E-epel , dist-f16 and dist-rawhide are I guess your targets.

As I mentioned in comment #2 you should now remove the BuildFails flag.


Review:


 +:ok, =:needs attention, -:needs fixing

MUST Items:
[=] MUST: rpmlint must be run on every package.
$ rpmlint SPECS/glue-validator.spec
RPMS/noarch/glue-validator-1.0.0-2.el6.noarch.rpm
SRPMS/glue-validator-1.0.0-2.el6.src.rpm 
SPECS/glue-validator.spec: W: no-cleaning-of-buildroot %install

If you plan to target EPEL5 as well this must change.

SPECS/glue-validator.spec: W: invalid-url Source0: glue-validator-1.0.0.tar.gz
Expected since a build from SVN.

glue-validator.noarch: W: spelling-error Summary(en_US) Valiation ->
Valuation, Validation, Variation

Valiation is clearly a typo.

glue-validator.noarch: W: incoherent-version-in-changelog 1.0.0-1
['1.0.0-2.el6', '1.0.0-2']

The changelog does not match the version and release.

glue-validator.noarch: W: no-documentation
Validation, Variation
Valuation, Validation, Variation

[+] MUST: The package must be named according to the Package Naming Guidelines.

[+] MUST: The spec file name must match the base package %{name}
list and more]
[+] MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
ASL 2.0
[+] MUST: The License field in the package spec file must match the actual
license.
License specified in setup.py.
[+] MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc.
No license file available.
[=] MUST: The spec file must be written in American English.
See typo  in rpmlint.
[+] MUST: The spec file for the package MUST be legible.
[=] MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL.

svn export http://svnweb.cern.ch/guest/gridinfo/glue-validator/tags/R_1_0_0 \
   glue-validator-1.0.0
and comparing that to the tar ball in the .spec file.

$ diff -r --brief glue-validator-1.0.0 ../SPECS/glue-validator-1.0.0
Only in ../SPECS/glue-validator-1.0.0/bin: glue-schema-validator
Only in ../SPECS/glue-validator-1.0.0: glue-validator.spec
Only in ../SPECS/glue-validator-1.0.0/lib: glue1
Only in ../SPECS/glue-validator-1.0.0/lib/glue2: tests
Only in ../SPECS/glue-validator-1.0.0/lib: validator
Only in ../SPECS/glue-validator-1.0.0: Makefile
Only in ../SPECS/glue-validator-1.0.0: MANIFEST.in
Only in glue-validator-1.0.0: PKG-INFO
Only in ../SPECS/glue-validator-1.0.0: tests


[+] MUST: The package must successfully compile and build into binary rpms on
at least one supported architecture.
[+] MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch.
[+] MUST: All build dependencies must be listed in BuildRequires
[+] MUST: The spec file MUST handle locales properly. This is done by using the
%find_lang macro.
No locales presnet.
[+] MUST: Every binary RPM package which stores shared library files (not just
symlinks) in any of the dynamic linker's default paths, must call ldconfig in
%post and %postun.
No shared libs.
[=] MUST: If the package is designed to be relocatable, the packager must state
this fact in the request for review

You have a Prefix but I can't believe you meant this package to be relocatable?
If you do intend relocation then provide a reason but it has to be very strong.
http://fedoraproject.org/wiki/Packaging:Guidelines#Relocatable_packages

[+] MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example. Every %files section must include a
%defattr(...) line.
[+] MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[=] MUST: Each package must consistently use macros, as described in the macros
section of Packaging Guidelines.

You use 'python' on one line but '%{__python}' else where. Be consistant,
there may be others.

[+] MUST: The package must contain code, or permissible content. This is
described in detail in the code vs. content section of Packaging Guidelines.
[+] MUST: Large documentation files should go in a doc subpackage.
No documentation :-)
[+] MUST: If a package includes something as %doc, it must not affect the
runtime of the application.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[=] MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
Needed for EPEL5 anyway.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[=] SHOULD: If the source package does not include license text(s) as a
separate file from upstream, the packager SHOULD query upstream to include it.
Please request upstream add one.

[+] SHOULD: The reviewer should test that the package builds in mock.
supported architectures.

Summary of things wrong.
(1)
A clean of the buildroot is required if you plan epel5?
See 
rpmlint -I no-cleaning-of-buildroot

(2)
Valiation is a typo.

(3)
The changelog does not match the version and release
See: http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
Please actually update changelogs during this process. These 
"releases" count to someextent and the history should extend
back to this process.

(4)
Source does not match upstream, see above.

(5)
Be consistant with macros, see above.


Comments:
(1)
Prefix: %{_prefix}  must not be present unless you really desire relocation,
I doubt it.
(2)
Url: http://cern.ch/glue  is better as URL as it's an ancronymn
(3)
BuildArchitectures: noarch is normally BuildArch: noarch. Apart from
anything else this does not syntax correctly in vim... It does seem to work
thouhg.
(4) 
Convention is that %globals are set at the top of the .spec file to 
ensure they really are global, i.e the sitelib and sitearch ones.
(5)
Please at least request that upstream add a license file.

(6) 
You set both python_sitelib and python_sitearch but one is redundant.

(7)
The %description is very terse. Please expand on it a bit, what is glue?

(8)
It's strange I think to hard code a schema version in the summary, 
in particular 2.0. Better to say in the description only that 'glue-validate'
will currently validate the 2.0 schema and leave the summary as non-schema
specific.

These comments indicate more that you have not learnt the guidelines more
than you have. Please at least try and correct rpmlint errors at the very
least and if they are not fixed justify why they are not fixed in the review
submission. I normally ignore such packages with a comment 
'rpmlint fails like this, please fix or justify the failiures.'

Upstream has provided a new version 1.0.1 that includes a LICENSE file. I have addresses all the issues mentioned in the the previous report. 

Spec URL: http://lfield.web.cern.ch/lfield/glue-validator.spec
SRPM URL: http://lfield.web.cern.ch/lfield/glue-validator-1.0.1-1.el6.src.rpm

A scratch build has been done. 

http://koji.fedoraproject.org/koji/taskinfo?taskID=3563877

As upstream has now added a LICENSE file please include it as a %doc
in the package. 

The description appears to have got even shorter, can it not be made useful
to strangers. It's not a requirement but it's probably better if it was expanded.

Fails at runtime:

# glue-validator 
Traceback (most recent call last):
  File "/usr/bin/glue-validator", line 7, in <module>
    import validator.utils
ImportError: No module named validator.utils

on centos6 + cl + epel.

Steve.

I see there are some tests in 'tests/' can they be executed in a %check.

http://fedoraproject.org/wiki/Packaging:Guidelines#Test_Suites

Spec URL: http://lfield.web.cern.ch/lfield/fedora/glue-validator.spec
SRPM URL: http://lfield.web.cern.ch/lfield/fedora/glue-validator-1.0.2-1.fc16.src.rpm

Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3584319

A new upstream version has been provided. The following changes have been made.
The LICENSE has been included in the package.
The missing module has been included.
The description has been improved.
A man page has been added.

Very nearly there:

(1)
It's best to change 

%{_mandir}/man1/glue-validator.1.gz

%{_mandir}/man1/glue-validator.1* 

to allow for different (or no) compression mechanisms, the actual mechanism being defined outside
the .spec file. It may well change one day.

(2)
You still use both the new style '%{buildroot}' and the old style $RPM_BUILD_ROOT.
Be consistant but my tip is to just drop $RPM_BUILD_ROOT completly in favour
of %{buildroot} always.

http://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

(3) 
Readability, you have blank line between %prep, %build  and %install which is probably
good but not between %install and %clean.

Spec URL: http://lfield.web.cern.ch/lfield/fedora/glue-validator.spec
SRPM URL: http://cern.ch/lfield/fedora/glue-validator-1.0.2-2.fc16.src.rpm

I have updated the spec file to address the issues you found. 

Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3595039

APPROVED but something I missed and a comment.

Change:
install -m 0644 man/glue-validator.1 %{buildroot}/usr/share/man/man1
to 
install -p -m 0644 man/glue-validator.1 %{buildroot}/usr/share/man/man1

to preserve the timestamp of the file from last edit. This is written down somewhere in the guidelines
but I can't find it just now.

Also a comment about your comments, e.g.
- A couple of small spec file fixes
is basically obvious , they could have more details about what changed so as to be useful
to the end user.

Steve.

> 
> Change:
> install -m 0644 man/glue-validator.1 %{buildroot}/usr/share/man/man1
> to 
> install -p -m 0644 man/glue-validator.1 %{buildroot}/usr/share/man/man1
> 
In fact should be:

install -p -m 0644 man/glue-validator.1 %{buildroot}%{_mandir}/man1

to be consistant with the use of %{_mandir} in the files section below, same for the 'mkdir'
line as well.

Steve.

Spec URL: http://lfield.web.cern.ch/lfield/fedora/glue-validator.spec
SRPM URL: http://cern.ch/lfield/fedora/glue-validator-1.0.2-3.fc16.src.rpm

I have updated the spec file to address the issues you found. 

Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3595076

New Package SCM Request
=======================
Package Name: glue-validator
Short Description: A validation framework for GLUE 2.0 information
Branches: f16 el5 el6
InitialCC:

You forgot the owner field on that SCM request.

New Package SCM Request
=======================
Package Name: glue-validator
Short Description: A validation framework for GLUE 2.0 information
Owners: lfield
Branches: f16 el5 el6
InitialCC:

Git done (by process-git-requests).

glue-validator-1.0.2-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/glue-validator-1.0.2-3.fc16

glue-validator-1.0.2-3.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/glue-validator-1.0.2-3.el5

glue-validator-1.0.2-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/glue-validator-1.0.2-3.el6

Package glue-validator-1.0.2-3.el5:
* should fix your issue,
* was pushed to the Fedora EPEL 5 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=epel-testing glue-validator-1.0.2-3.el5'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-EPEL-2011-5280/glue-validator-1.0.2-3.el5
then log in and leave karma (feedback).

glue-validator-1.0.2-3.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

glue-validator-1.0.2-3.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

glue-validator-1.0.2-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

Package Change Request
======================
Package Name: glue-validator
New Branches: epel7

Git done (by process-git-requests).