Bug 752829
Summary: | Review Request: glue-validator - A validation framework for GLUE 2.0 information | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Laurence Field <Laurence.Field> |
Component: | Package Review | Assignee: | Steve Traylen <steve.traylen> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | notting, package-review, steve.traylen |
Target Milestone: | --- | Flags: | steve.traylen:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | glue-validator-1.0.2-3.el6 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2012-01-11 06:07:53 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Laurence Field
2011-11-10 14:31:18 UTC
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). |