Bug 752829 - Review Request: glue-validator - A validation framework for GLUE 2.0 information
Summary: Review Request: glue-validator - A validation framework for GLUE 2.0 information
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Steve Traylen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-10 14:31 UTC by Laurence Field
Modified: 2014-10-13 23:04 UTC (History)
3 users (show)

Fixed In Version: glue-validator-1.0.2-3.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-01-11 06:07:53 UTC
steve.traylen: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Laurence Field 2011-11-10 14:31:18 UTC
Spec URL: http://lfield.web.cern.ch/lfield/glue-validator.spec
SRPM URL: http://lfield.web.cern.ch/lfield/glue-validator-0.0.3-2.el5.src.rpm
Description: A valiation framework for GLUE 2.0 information

Comment 1 Laurence Field 2011-11-10 14:41:42 UTC
This is my first package and I am seeking a sponsor.

Comment 2 Steve Traylen 2011-11-10 21:59:31 UTC
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.

Comment 3 Steve Traylen 2011-11-10 22:00:25 UTC
(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

Comment 4 Laurence Field 2011-11-14 14:04:37 UTC
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

Comment 5 Steve Traylen 2011-11-15 15:37:07 UTC
Another package from Laurence: bug #754137

Comment 6 Laurence Field 2011-11-16 11:02:48 UTC
First informal reviews of other packages:

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

Comment 7 Laurence Field 2011-11-16 11:23:37 UTC
More detailed review of the comoonics-base-py package

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

Comment 8 Laurence Field 2011-11-16 14:34:45 UTC
Another informal review, this time for abootimg

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

Comment 9 Steve Traylen 2011-11-22 21:43:24 UTC
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.

Comment 10 Laurence Field 2011-11-29 14:42:12 UTC
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

Comment 11 Steve Traylen 2011-11-29 18:53:01 UTC
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.

Comment 12 Steve Traylen 2011-11-29 18:55:11 UTC
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.

Comment 13 Laurence Field 2011-11-30 09:07:29 UTC
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

Comment 14 Laurence Field 2011-11-30 09:33:54 UTC
Here is a link to the mock build. 

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

Comment 15 Steve Traylen 2011-12-04 17:08:04 UTC
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.'

Comment 16 Laurence Field 2011-12-05 15:32:03 UTC
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

Comment 17 Steve Traylen 2011-12-07 18:22:13 UTC
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.

Comment 18 Steve Traylen 2011-12-07 18:48:12 UTC
I see there are some tests in 'tests/' can they be executed in a %check.

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

Comment 19 Laurence Field 2011-12-14 13:50:11 UTC
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.

Comment 20 Steve Traylen 2011-12-19 15:48:01 UTC
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.

Comment 21 Laurence Field 2011-12-20 10:41:13 UTC
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

Comment 22 Steve Traylen 2011-12-20 11:01:23 UTC
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.

Comment 23 Steve Traylen 2011-12-20 11:03:27 UTC
> 
> 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.

Comment 24 Laurence Field 2011-12-20 11:12:39 UTC
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

Comment 25 Laurence Field 2011-12-20 11:53:41 UTC
New Package SCM Request
=======================
Package Name: glue-validator
Short Description: A validation framework for GLUE 2.0 information
Branches: f16 el5 el6
InitialCC:

Comment 26 Steve Traylen 2011-12-20 12:01:52 UTC
You forgot the owner field on that SCM request.

Comment 27 Laurence Field 2011-12-20 12:19:26 UTC
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:

Comment 28 Gwyn Ciesla 2011-12-20 12:52:08 UTC
Git done (by process-git-requests).

Comment 29 Fedora Update System 2011-12-20 13:52:44 UTC
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

Comment 30 Fedora Update System 2011-12-20 13:54:10 UTC
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

Comment 31 Fedora Update System 2011-12-20 13:55:17 UTC
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

Comment 32 Fedora Update System 2011-12-20 20:03:12 UTC
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).

Comment 33 Fedora Update System 2012-01-11 06:07:53 UTC
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.

Comment 34 Fedora Update System 2012-01-11 07:59:29 UTC
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.

Comment 35 Fedora Update System 2012-01-11 07:59:56 UTC
glue-validator-1.0.2-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 36 Steve Traylen 2014-10-09 11:45:53 UTC
Package Change Request
======================
Package Name: glue-validator
New Branches: epel7

Comment 37 Kevin Fenzi 2014-10-13 23:04:41 UTC
Git done (by process-git-requests).


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