This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 1069335 - Review Request: openstack-ironic - Management and provisioning of physical machines for Openstack
Review Request: openstack-ironic - Management and provisioning of physical ma...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On: 1067445
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-24 13:26 EST by Angus Thomas
Modified: 2016-04-26 09:24 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-02-02 05:55:30 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
sdake: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Angus Thomas 2014-02-24 13:26:45 EST
Spec URL: https://github.com/agroup/tripleo-rpm-spec-files/blob/master/ironic/openstack-ironic.spec
SRPM URL: http://athomas.fedorapeople.org/tuskarclient/python-tuskarclient-0.1.0-2.fc20.src.rpm
Description: Ironic provides an API for management and provisioning of physical machines
Fedora Account System Username: athomas
Comment 1 Jordan OMara 2014-02-25 15:38:31 EST
I believe I am allowed to sponsor athomas for the openstack fedora group. If not, please let me know and I'll find someone who can. 

Overall, review looks good - please check the ! items below and tidy those up.

Output from fedora-review:

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed, [j] = checked by me


===== MUST items =====

Generic:
[j]: Package is licensed with an open-source compatible license and meets
    other legal requirements as defined in the legal section of Packaging
     Guidelines.
[j]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)", "Unknown or generated", "*No copyright* Apache (v2.0)".
     2 files have unknown license. Detailed output of licensecheck in
     /home/jomara/dev/fedora-reviews/python-tuskarclient/licensecheck.txt

Project is ASL 2.0

[j]: Package contains no bundled libraries without FPC exception.
check
[j]: Changelog in prescribed format.
check
[j]: Sources contain only permissible code or content.
[j(NA)]: Package contains desktop file if it is a GUI application.
[j]: Development files must be in a -devel package
[j]: Package uses nothing in %doc for runtime.
[j]: Package consistently uses macros (instead of hard-coded directory names).
[j]: Package is named according to the Package Naming Guidelines.
[j]: Package does not generate any conflict.
[j]: Package obeys FHS, except libexecdir and /usr/target.
[j]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[j]: Requires correct, justified where necessary.
[j]: Spec file is legible and written in American English.
[j]: Package contains systemd file(s) if in need.
[j]: Package is not known to require an ExcludeArch tag.
[j]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 2 files.
[j]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 is included in %doc.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[j]: Python eggs must not download any dependencies during the build process.
[!]: A package which is used by another package via an egg interface should
     provide egg info.

Egg Info is missing from this package, might be worth adding . I believe this IS required based on python packaging guidelines

[!]: Package meets the Packaging Guidelines::Python

 - based on my read, you need to BuildRequires: python2-devel
 - egginfo


[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[j]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[j]: Final provides and requires are sane (see attachments).
[j]: Package functions as described.
[j]: Latest version is packaged.
[j]: Package does not include license text files separate from upstream.
[ ]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.

I believe, but am not positive, that translations for this package are surfaced in other packages

[?]: Package should compile and build into binary rpms on all supported
     architectures.

noarch

[!j]: %check is present and all tests pass.

I don't see a %check section, but I don't typically see one with packages like this. Also not in "MUST" section

[?]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.



Great work!
Comment 2 Matthias Runge 2014-02-26 10:29:48 EST
Jordan, you can not sponsor Angus into the packager group.

Angus, are you going to maintain this package?
Comment 3 Steven Dake 2014-02-26 12:07:35 EST
Angus,

I'll sponsor you.

Please have a quick read:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

Let me know when you have completed this step.
Comment 4 James Slagle 2014-02-27 08:44:35 EST
Angus, the link for the SRPM Url is for python-tuskarclient, not openstack-ironic :)
Comment 6 James Slagle 2014-02-27 09:53:22 EST
Hi, I'm doing an unofficial review.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Package contains BR: python2-devel or python3-devel
  Change the BR from python-devel to python2-devel
- Inconsistent alignment in spec file
- Looks like you got a request for more info on the fedora ticket request for
  the static uid/gid
- Other issues in the comments below


===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)", "Unknown or generated", "*No copyright* Apache (v2.0)".
     6 files have unknown license. Detailed output of licensecheck in
     /home/jslagle/rpmbuild/1069335-openstack-ironic/review-openstack-
     ironic/licensecheck.txt
[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/systemd/system,
     /usr/lib/systemd
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[!]: %config files are marked noreplace or the reason is justified.
     Note: No (noreplace) in %config %attr(-,root,ironic) /etc/ironic

     recommend to add noreplace to the above %config. We don't want local
     changes to these files overwritten by rpm do we?

[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[!]: Package consistently uses macros (instead of hard-coded directory names).

     Use %{_unitdir} for /usr/lib/systemd/system
     This requires adding BuildRequires: systemd per https://fedoraproject.org/wiki/Packaging:Systemd

[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[!]: Requires correct, justified where necessary.

     Why are there runtime requirements on the following?
     - swig
     - pyflakes
     These aren't in upstream ironic's requirements.txt, so they shouldn't be
     needed.

     Also need to remove the Requires on python-pbr. You'll have to provide a
     patch to remove the runtime requirement. See our other OpenStack packages
     for examples of this, (e.g. in glance there is a patch called
     0002-Remove-runtime-dep-on-python-pbr.patch)

[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 2 files.
[!]: Package complies to the Packaging Guidelines

      Outstanding issues mentioned elsewhere in this review.

[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 is included in %doc.
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[!]: Package meets the Packaging Guidelines::Python

     See other outstanding issues

[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[!]: Final provides and requires are sane (see attachments).

     Why is there a Provides on ironic? This shouldn't be necessary should it?

     Other Outstanding issues here, please see other comments

[ ]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[ ]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
     
     only tested on x86_64

[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: openstack-ironic-2014.1-b2.2.fc21.noarch.rpm
          openstack-ironic-2014.1-b2.2.fc21.src.rpm
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.conf
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/policy.json
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-deploy-helper.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-images.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/ironic.conf
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/dbsync.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/common/service.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.conf ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.conf 0640L
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/link.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/conductor.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/openstack/common/rpc/zmq_receiver.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/drivers/modules/deploy_utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/driver.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/policy.json ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/policy.json 0640L
openstack-ironic.noarch: W: non-standard-gid /etc/ironic ironic
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/state.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_utils.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-deploy-helper.filters ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-deploy-helper.filters 0640L
openstack-ironic.noarch: W: non-standard-uid /var/lib/ironic ironic
openstack-ironic.noarch: W: non-standard-gid /var/lib/ironic ironic
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/chassis.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/common/driver_factory.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_root.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-images.filters ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-images.filters 0640L
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters 0640L
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/collection.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_drivers.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/api.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/ironic.conf ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/ironic.conf 0640L
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-rootwrap
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-dbsync
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-conductor
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-api
openstack-ironic.src:16: W: unversioned-explicit-provides ironic
openstack-ironic.src:83: E: hardcoded-library-path in %{buildroot}/usr/lib/systemd/system/
openstack-ironic.src:84: E: hardcoded-library-path in %{_prefix}/lib/systemd/system/
openstack-ironic.src:85: E: hardcoded-library-path in %{_prefix}/lib/systemd/system/
openstack-ironic.src:98: E: hardcoded-library-path in %{_prefix}/lib/systemd/system/*
openstack-ironic.src:18: W: mixed-use-of-spaces-and-tabs (spaces: line 18, tab: line 8)
2 packages and 0 specfiles checked; 26 errors, 22 warnings.

Besides the other feedback above, the other warnings and errors look ok here.



Rpmlint (installed packages)
----------------------------
# rpmlint openstack-ironic
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.conf
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/policy.json
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-deploy-helper.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-images.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/ironic.conf
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/dbsync.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/common/service.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.conf ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.conf 0640L
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/link.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/conductor.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/openstack/common/rpc/zmq_receiver.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/drivers/modules/deploy_utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/driver.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/policy.json ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/policy.json 0640L
openstack-ironic.noarch: W: non-standard-gid /etc/ironic ironic
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/state.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_utils.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-deploy-helper.filters ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-deploy-helper.filters 0640L
openstack-ironic.noarch: W: non-standard-uid /var/lib/ironic ironic
openstack-ironic.noarch: W: non-standard-gid /var/lib/ironic ironic
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/chassis.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/common/driver_factory.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_root.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-images.filters ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-images.filters 0640L
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters 0640L
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/collection.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_drivers.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/api.py 0644L /usr/bin/env
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/ironic.conf ironic
openstack-ironic.noarch: E: non-readable /etc/ironic/ironic.conf 0640L
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-rootwrap
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-dbsync
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-conductor
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-api
1 packages and 0 specfiles checked; 22 errors, 20 warnings.
# echo 'rpmlint-done:'



Requires
--------
openstack-ironic (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python
    config(openstack-ironic)
    pycrypto
    pyflakes
    python(abi)
    python-anyjson
    python-eventlet
    python-fixtures
    python-glanceclient
    python-iso8601
    python-jinja2
    python-jsonpatch
    python-keystoneclient
    python-kombu
    python-migrate
    python-mock
    python-netaddr
    python-neutronclient
    python-paramiko
    python-pbr
    python-pecan
    python-sqlalchemy
    python-stevedore
    python-wsme
    shadow-utils
    swig



Provides
--------
openstack-ironic:
    config(openstack-ironic)
    ironic
    openstack-ironic



Source checksums
----------------
https://launchpad.net/ironic/icehouse/icehouse-2/+download/ironic-2014.1.b2.tar.gz :
  CHECKSUM(SHA256) this package     : b3a3bdf04df2e15519058fbe66d63b5156bcde8fc35c6cda787db3c34f2c637e
  CHECKSUM(SHA256) upstream package : b3a3bdf04df2e15519058fbe66d63b5156bcde8fc35c6cda787db3c34f2c637e


Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13
Command line :/usr/bin/fedora-review -n openstack-ironic
Buildroot used: fedora-rawhide-x86_64
Active plugins: Python, Generic, Shell-api
Disabled plugins: Java, C/C++, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
Comment 7 Steven Dake 2014-02-27 10:08:23 EST
James,

Have a read of:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets

Does anything pop out to you as a suggestion related to systemd?

Regards
-steve
Comment 8 Steven Dake 2014-02-27 10:12:56 EST
James,

It is typically the packagers responsibility to diagnose and fix the rpmlint warnings, but as the package reviewer, you have an opportunity to provide guidance about what you would recommend changing to fix the rpmlint problems.  Many of those errors and warnings cannot be ignored.  If you as the package reviewer think they can be ignored, it is your responsibility to actually verify that is the case.  It is the packagers responsibility to defend any decisions to ignore rpmlint warnings.  One cool thing about being a packager is two brains working together > 2 brains working separately.  Please have a more detailed look at the rpmlint warnings and errors and comment in detail.

Regards,
-steve
Comment 9 Steven Dake 2014-02-27 10:32:09 EST
Angus,

I'd recommend having a read through this review:

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

as well as:

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

then providing an unofficial review of:
https://bugzilla.redhat.com/show_bug.cgi?id=1067002

followed by providing an unofficial review of the other two bugs mentioned in this comment.

Then we should be all set :)
Comment 10 Steven Dake 2014-02-27 10:37:54 EST
Angus,

I just wanted to point out a couple pieces of documentation that you may or not be aware of.  I would recommend blocking off about 1-2 hours and reading through them.  The more experience you get reviewing, the less you will have to reference the documentation, but if its not documented, its probably not required.

The package review process:
https://fedoraproject.org/wiki/Package_Review_Process

The packaging guidelines:
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
This contain 32 separate wiki pages.  I would recommend reading through them so you know where to find information in future packaging efforts.

The python-specific guidelines:
https://fedoraproject.org/wiki/Packaging:Python

Regards,
-steve
Comment 11 James Slagle 2014-02-27 11:24:02 EST
(In reply to Steven Dake from comment #7)
> James,
> 
> Have a read of:
> https://fedoraproject.org/wiki/Packaging:ScriptletSnippets
> 
> Does anything pop out to you as a suggestion related to systemd?
> 
> Regards
> -steve

Ah, Yes :)

Per https://fedoraproject.org/wiki/Starting_services_by_default, openstack-ironic-api and openstack-ironic-conductor should not be started by default.

And, need to add the following to the spec file:
%post
%systemd_post openstack-ironic-api.service
%systemd_post openstack-ironic-conductor.service

%preun
%systemd_preun openstack-ironic-api.service
%systemd_preun openstack-ironic-conductor.service

%postun
%systemd_postun_with_restart openstack-ironic-api.service
%systemd_postun_with_restart openstack-ironic-conductor.service

The other OpenStack packages use the scriptlets themselves and not the %systemd_* macros though. Is there a reason for that?
Comment 12 James Slagle 2014-02-27 12:26:35 EST
(In reply to Steven Dake from comment #8)
> James,
> 
> It is typically the packagers responsibility to diagnose and fix the rpmlint
> warnings, but as the package reviewer, you have an opportunity to provide
> guidance about what you would recommend changing to fix the rpmlint
> problems.  Many of those errors and warnings cannot be ignored.  If you as
> the package reviewer think they can be ignored, it is your responsibility to
> actually verify that is the case.  It is the packagers responsibility to
> defend any decisions to ignore rpmlint warnings.  One cool thing about being
> a packager is two brains working together > 2 brains working separately. 
> Please have a more detailed look at the rpmlint warnings and errors and
> comment in detail.
> 
> Regards,
> -steve

I was using the other OpenStack packages already in rdo to determine what I thought was ok to allow here.

Here's my deeper feedback on the SRPM lint:

Rpmlint
-------
Checking: openstack-ironic-2014.1-b2.2.fc21.noarch.rpm
          openstack-ironic-2014.1-b2.2.fc21.src.rpm
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.conf
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/policy.json
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-deploy-helper.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-images.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/ironic.conf

Suggest to add %noreplace for above files, see earlier feedback in review.

openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/dbsync.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/common/service.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/link.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/conductor.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/openstack/common/rpc/zmq_receiver.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/drivers/modules/deploy_utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/driver.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/state.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/chassis.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/common/driver_factory.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_root.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/collection.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_drivers.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/api.py 0644L /usr/bin/env

Didn't see anything specific about what to do for these at https://fedoraproject.org/wiki/Packaging:Python
rpmlint is complaining I assume b/c there's a shebang in the files, but they aren't actually meant to be scripts and aren't executable.
Don't see anything about this at https://fedoraproject.org/wiki/Common_Rpmlint_issues
The files themselves aren't +x, given the output above.

What's the path forward here? It seems like the only thing that can be done in the spec itself is to make these files +x in %files. But, these things aren't actually scripts (from what I can tell), so a patch upstream to remove the #!/usr/bin/env lines from these files should probably be sumitted.  Then we can add that patch to the rpm until it is merged.

openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.conf ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/policy.json ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-deploy-helper.filters ironic
openstack-ironic.noarch: W: non-standard-uid /var/lib/ironic ironic
openstack-ironic.noarch: W: non-standard-gid /var/lib/ironic ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-images.filters ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/ironic.conf ironic

These can be explained by the fact that ironic is a non standard group, so rpm doesn't know about it. I think that's ok.

openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.conf 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/policy.json 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-deploy-helper.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-images.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/ironic.conf 0640L

These errors are b/c the files are not world readable. That is what is desired I believe. Do we need to flag them as exceptions to rpmlint somehow?

openstack-ironic.noarch: W: no-manual-page-for-binary ironic-rootwrap
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-dbsync
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-conductor
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-api

man pages don't exist for these commands. They don't for most OpenStack commands. Is that ok to ignore?

openstack-ironic.src:16: W: unversioned-explicit-provides ironic

This needs to be fixed. See earlier review comments.

openstack-ironic.src:83: E: hardcoded-library-path in %{buildroot}/usr/lib/systemd/system/
openstack-ironic.src:84: E: hardcoded-library-path in %{_prefix}/lib/systemd/system/
openstack-ironic.src:85: E: hardcoded-library-path in %{_prefix}/lib/systemd/system/
openstack-ironic.src:98: E: hardcoded-library-path in %{_prefix}/lib/systemd/system/*

This needs to be fixed. See earlier review comments.

openstack-ironic.src:18: W: mixed-use-of-spaces-and-tabs (spaces: line 18, tab: line 8)

This needs to be fixed. See earlier review comments.

2 packages and 0 specfiles checked; 26 errors, 22 warnings.
Comment 13 James Slagle 2014-02-27 12:31:14 EST
I don't see anything additional from the binary RPM lint that I haven't commented on above.

openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.conf
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/policy.json
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-deploy-helper.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-images.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters
openstack-ironic.noarch: W: conffile-without-noreplace-flag /etc/ironic/ironic.conf

openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/dbsync.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/common/service.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/link.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/conductor.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/openstack/common/rpc/zmq_receiver.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/drivers/modules/deploy_utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/driver.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/state.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/chassis.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/common/driver_factory.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_root.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/utils.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/api/controllers/v1/collection.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/tests/api/test_drivers.py 0644L /usr/bin/env
openstack-ironic.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/ironic/cmd/api.py 0644L /usr/bin/env

openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.conf ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/policy.json ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-deploy-helper.filters ironic
openstack-ironic.noarch: W: non-standard-uid /var/lib/ironic ironic
openstack-ironic.noarch: W: non-standard-gid /var/lib/ironic ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-images.filters ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters ironic
openstack-ironic.noarch: W: non-standard-gid /etc/ironic/ironic.conf ironic

openstack-ironic.noarch: E: non-readable /etc/ironic/policy.json 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.conf 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-deploy-helper.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-images.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/ironic.conf 0640L

openstack-ironic.noarch: W: no-manual-page-for-binary ironic-rootwrap
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-dbsync
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-conductor
openstack-ironic.noarch: W: no-manual-page-for-binary ironic-api
1 packages and 0 specfiles checked; 22 errors, 20 warnings.
# echo 'rpmlint-done:'
Comment 14 Steven Dake 2014-02-27 13:53:39 EST
Regarding Comment #11, there is also some Requires (for systemd) required.

I am pretty sure the rest of the openstack packages break out the api from the conductor.  This is not something a reviewer would typically comment on (its the packager's responsibility to decide how they want to package something) but for consistency it might make sense to make them subpackages.
Comment 15 Steven Dake 2014-02-27 14:11:30 EST
Re Comment #12:
What's the path forward here? It seems like the only thing that can be done in the spec itself is to make these files +x in %files. But, these things aren't actually scripts (from what I can tell), so a patch upstream to remove the #!/usr/bin/env lines from these files should probably be sumitted.  Then we can add that patch to the rpm until it is merged.

Nice work.  You identified the problem and what to do (submit a patch upstream).  The review doesn't need to gate on this, but ideally upstream should fix this oversight.  I don't think its necessary to carry a patch on top of the patch stream in the RPM to fix the rpmlint warnings as long as an upstream bug is filed by the submitter (Angus) and linked in the bugzilla for further reference.

Re:
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.conf 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/policy.json 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-deploy-helper.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-images.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters 0640L
openstack-ironic.noarch: E: non-readable /etc/ironic/ironic.conf 0640L

These errors are b/c the files are not world readable. That is what is desired I believe. Do we need to flag them as exceptions to rpmlint somehow?

Unfortunately rpmlint doesn't have said flagging feature.  It makes sense for the reviewer in this case to build the rpm and make sure /etc/ironic and /var/lib/ironic actually are installed with the uid/gid specified in the spec file before signing off on these specific rpmlint E messages.  Generally if rpmlint gives an error, and you want to fedora-review+ the package, the reviewer is responsible for verifying the E is a false-negative.

Re man pages, what I typically do is ask in the review for the submitter to request the upstream create manual pages for their binaries.  In the case of OpenStack, the upstream doesn't seem to care, but it doesn't hurt to create said bugs anyway.

Looking good James!
Comment 16 Angus Thomas 2014-02-27 16:11:37 EST
Many thanks for the detailed feedback. Based on all the recommended changes:

Spec URL: https://raw.github.com/agroup/tripleo-rpm-spec-files/master/ironic/openstack-ironic.spec

SRPM URL: http://athomas.fedorapeople.org/ironic/fedora20/openstack-ironic-2014.1-b2.3.fc20.src.rpm
Comment 17 James Slagle 2014-02-28 07:36:46 EST
Hi Angus, the latest iteration does not build in mock. Nothing gets installed because you also removed the BuildRequires on python-pbr. It's ok to have python-pbr as a BuildRequires (and is in fact needed), just not as a Requires.
Comment 18 Angus Thomas 2014-02-28 10:17:35 EST
Hi James.

Sorry about that. It fooled me by building fine locally. Fixed now.

Spec URL: https://raw.github.com/agroup/tripleo-rpm-spec-files/master/ironic/openstack-ironic.spec

SRPM URL: http://athomas.fedorapeople.org/ironic/fedora20/openstack-ironic-2014.1-b2.4.fc20.src.rpm
Comment 19 Steven Dake 2014-03-18 14:08:48 EDT
(In reply to Angus Thomas from comment #18)
> Hi James.
> 
> Sorry about that. It fooled me by building fine locally. Fixed now.
> 
Angus,

You can always use mock to build locally but without all your installed packages which might not pick out problems.

The fedora-review tool uses mock if you don't want to learn the details of mock, so that is always an option.
Comment 20 Steven Dake 2014-03-19 20:21:51 EDT
Angus,

I would recommend splitting up the api and conductor into separate packages.  All the OpenStack packaging behaves in this way.  For an example of how it is done take a look at the nova or heat repos in fedpkg.  (fedpkg clone openstack-heat).  If you think this is too big a task to take on, we can approve as is, but I suspect someone will have to go in and rework that part of the packaging, so we might as well get it out of the way now.

The reason for separate packages is the api may be installed separately from the engine.
Comment 21 Angus Thomas 2014-03-31 10:54:40 EDT
Hi.

New version which builds into -common, -api and -conductor packages

Spec URL: https://raw.github.com/agroup/tripleo-rpm-spec-files/master/ironic/openstack-ironic.spec

SRPM URL: http://athomas.fedorapeople.org/ironic/fedora20/openstack-ironic-2014.1-b2.5.fc20.src.rpm


Angus
Comment 23 Steven Dake 2014-04-07 16:02:11 EDT
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)", "Apache (v2.0) MIT/X11 (BSD like)", "Unknown or
     generated", "*No copyright* Apache (v2.0)". 8 files have unknown license.
     Detailed output of licensecheck in /home/sdake/fedora-review/1069335
     -openstack-ironic/licensecheck.txt
[!]: License file installed when any subpackage combination is installed.
[-]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 is included in %doc.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate file
    from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in openstack-
     ironic-common , openstack-ironic-api , openstack-ironic-conductor
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Scriptlets must be sane, if used.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: openstack-ironic-common-2014.1-rc1.1.fc20.noarch.rpm
          openstack-ironic-api-2014.1-rc1.1.fc20.noarch.rpm
          openstack-ironic-conductor-2014.1-rc1.1.fc20.noarch.rpm
          openstack-ironic-2014.1-rc1.1.fc20.src.rpm
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.conf ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/rootwrap.conf 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/policy.json ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/policy.json 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic ironic
openstack-ironic-common.noarch: W: non-standard-uid /var/lib/ironic ironic
openstack-ironic-common.noarch: W: non-standard-gid /var/lib/ironic ironic
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-images.filters ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-images.filters 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.d ironic
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-utils.filters ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-utils.filters 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/ironic.conf ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/ironic.conf 0640L
openstack-ironic-common.noarch: W: no-manual-page-for-binary ironic-rootwrap
openstack-ironic-common.noarch: W: no-manual-page-for-binary ironic-dbsync
openstack-ironic-api.noarch: W: only-non-binary-in-usr-lib
openstack-ironic-api.noarch: W: no-documentation
openstack-ironic-api.noarch: W: no-manual-page-for-binary ironic-api
openstack-ironic-conductor.noarch: W: only-non-binary-in-usr-lib
openstack-ironic-conductor.noarch: W: no-documentation
openstack-ironic-conductor.noarch: W: no-manual-page-for-binary ironic-conductor
openstack-ironic.src:20: W: macro-in-comment %{release_name}
openstack-ironic.src:20: W: macro-in-comment %{release_name}
openstack-ironic.src:20: W: macro-in-comment %{milestone}
openstack-ironic.src:20: W: macro-in-comment %{full_release}
openstack-ironic.src:194: W: mixed-use-of-spaces-and-tabs (spaces: line 194, tab: line 7)
4 packages and 0 specfiles checked; 6 errors, 23 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint openstack-ironic-common openstack-ironic-api openstack ^M-ironic-conductor
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.conf ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/rootwrap.conf 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/policy.json ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/policy.json 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic ironic
openstack-ironic-common.noarch: W: non-standard-uid /var/lib/ironic ironic
openstack-ironic-common.noarch: W: non-standard-gid /var/lib/ironic ironic
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-images.filters ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-images.filters 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.d ironic
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-manage-ipmi.filters 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/rootwrap.d/ironic-utils.filters ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/rootwrap.d/ironic-utils.filters 0640L
openstack-ironic-common.noarch: W: non-standard-gid /etc/ironic/ironic.conf ironic
openstack-ironic-common.noarch: E: non-readable /etc/ironic/ironic.conf 0640L
openstack-ironic-common.noarch: W: no-manual-page-for-binary ironic-rootwrap
openstack-ironic-common.noarch: W: no-manual-page-for-binary ironic-dbsync
openstack-ironic-api.noarch: W: only-non-binary-in-usr-lib
openstack-ironic-api.noarch: W: no-documentation
openstack-ironic-api.noarch: W: no-manual-page-for-binary ironic-api
openstack-ironic-conductor.noarch: W: only-non-binary-in-usr-lib
openstack-ironic-conductor.noarch: W: no-documentation
openstack-ironic-conductor.noarch: W: no-manual-page-for-binary ironic-conductor
3 packages and 0 specfiles checked; 6 errors, 18 warnings.
Requires
--------
openstack-ironic-common (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    config(openstack-ironic-common)
    pycrypto
    python(abi)
    python-alembic
    python-anyjson
    python-eventlet
    python-fixtures
    python-glanceclient
    python-iso8601
    python-jinja2
    python-jsonpatch
    python-keystoneclient
    python-kombu
    python-migrate
    python-mock
    python-netaddr
    python-neutronclient
    python-paramiko
    python-pecan
    python-pyghmi
    python-sqlalchemy
    python-stevedore
    python-wsme
    shadow-utils

openstack-ironic-api (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    openstack-ironic-common
    systemd

openstack-ironic-conductor (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    openstack-ironic-common
    systemd
Provides
--------
openstack-ironic-common:
    config(openstack-ironic-common)
    openstack-ironic-common

openstack-ironic-api:
    openstack-ironic-api

openstack-ironic-conductor:
    openstack-ironic-conductor



Source checksums
----------------
https://launchpad.net/ironic/icehouse/icehouse-rc1/+download/ironic-2014.1.rc1.tar.gz :
  CHECKSUM(SHA256) this package     : bd5981d3a9a870bfe3a853f87b6d6f636c586bd7ad1da31ebc87a13128ecfb35
  CHECKSUM(SHA256) upstream package : bd5981d3a9a870bfe3a853f87b6d6f636c586bd7ad1da31ebc87a13128ecfb35
Comment 24 Steven Dake 2014-04-07 16:05:00 EDT
Angus,

Strictly speaking this package meets the license guidelines by installing any package installs common which installs license and readme.  I don't like to leave things open to interpretation re licensing however in package reviews, so I think it would make sense to add the license (and possibly the readme) to each of the subpackages as %doc sections.

The rpmlint errors look fine, fedora-review gives a clean bill of health, and a manual review of the package since its complex shows it looks to be in good order.

I'll be happy to approve once the %doc is added to each subpackage.

Regards
-steve
Comment 25 Angus Thomas 2014-04-09 14:07:10 EDT
Thanks Steve.

I've updated the package so that that license is present in the subpackages.

Spec URL: https://raw.github.com/agroup/tripleo-rpm-spec-files/master/ironic/openstack-ironic.spec

SRPM URL: http://athomas.fedorapeople.org/ironic/fedora20/SRPMS/openstack-ironic-2014.1-rc1.2.fc20.src.rpm

Angus
Comment 26 Steven Dake 2014-04-09 15:33:13 EDT
Package in comment #25 APPROVED.
Comment 27 Steven Dake 2014-04-09 15:33:53 EDT
Please submit a fedora SCM request:
http://fedoraproject.org/wiki/Package_SCM_admin_requests
Comment 28 Angus Thomas 2014-04-09 16:04:02 EDT
New Package SCM Request
=======================
Package Name: openstack-ironic
Short Description: Ironic provides an API for management and provisioning of physical machines
Owners: athomas
Branches: f20
InitialCC:
Comment 29 Jon Ciesla 2014-04-10 09:28:03 EDT
Git done (by process-git-requests).
Comment 31 Matthias Runge 2015-02-02 05:55:30 EST
This is included in rawhide already.

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