Bug 807017

The package contains currently the oVirt backend and the REST API.

In order to run it needs JBoss AS7, which is close to be submitted for review.

I'm trying to do a review, but the mock build fails due to:
Error: No Package found for spring-ldap

The spring-ldap was added just yesterday, so it is available currently only in rawhide and won't probably yet reached all mirrors. You can get it here:

http://koji.fedoraproject.org/koji/buildinfo?buildID=309704

Thanks for testing!

Created attachment 573382 [details]
Fedora-Review output

Juan,

^^^ attached the output of Fedora-Review, which containts the full output (w/ rpmlint errors)
Issues:
[!]: MUST Javadoc documentation files are generated and included in -javadoc
     subpackage
     Note: No javadoc subpackage present
See: https://fedoraproject.org/wiki/Packaging:Java#Javadoc_installation
[!]: MUST Javadoc subpackages have Requires: jpackage-utils
See: https://fedoraproject.org/wiki/Packaging:Java
[!]: MUST Javadocs are placed in %{_javadocdir}/%{name} (no -%{version}
     symlink)
     Note: No javadoc subpackage present
See: https://fedoraproject.org/wiki/Packaging:Java#Javadoc_installation
[!]: MUST Package does not contain duplicates in %files.
     Note: warning: File listed twice: /etc/ovirt-
     engine/notifier/notifier.conf
See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
[!]: MUST Rpmlint output is silent.

Ofer, thank you very much for the review!

Quite a long list of things to take care of. I start inmediately to fix them.

The spec has been updated to fix most of the errors detected during the review that Ofer did. The updated .src.rpm is available here:

http://jhernand.fedorapeople.org/rpms/ovirt-engine-3.0.0-9.0001.fc18.src.rpm

The updated package builds correctly in Koji:

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

There are still rpmlint warnings and also some errors that can't be avoided, but can be explained:

ovirt-engine.noarch: E: non-readable /etc/ovirt-engine/engine.conf 0640L
ovirt-engine-iso-uploader.noarch: E: non-readable /etc/ovirt-engine/isouploader.conf 0600L
ovirt-engine-log-collector.noarch: E: non-readable /etc/ovirt-engine/logcollector.conf 0600L
ovirt-engine-notification-service.noarch: E: non-readable /etc/ovirt-engine/notifier/notifier.conf 0640L

These file need restrictive read permissions because they may contain passwords.

ovirt-engine-log-collector.noarch: E: script-without-shebang /usr/lib/python2.7/site-packages/sos/plugins/postgresql.py
ovirt-engine-log-collector.noarch: E: script-without-shebang /usr/lib/python2.7/site-packages/sos/plugins/jboss.py
ovirt-engine-log-collector.noarch: E: script-without-shebang /usr/lib/python2.7/site-packages/sos/plugins/engine.py

These files have no shebang and execution permissions like all the other SOS plugins.

Ofer,
If your going to review this package, please
1. assign it to yourself (click take button)
2. place into the ASSIGNED state so Juan knows you are taking responsibility for reviewing his package.
3 Set "fedora-review" flag to ? to indicate you are currently reviewing the package.

Regards
-steve

Some comments on the spec file:

- Why does the spec file contain a huge license section at the
  top?  I think you should avoid this, unless there is some pressing
  reason for a specific license on the spec file itself.

- py_site_pkgs uses %define, should almost certainly be using %global.

- These seem to be unnecessary.  I would remove them and use the programs
  directly.
%global __getent /usr/bin/getent
%global __groupadd /usr/sbin/groupadd
%global __useradd /usr/sbin/useradd
%global __usermod /usr/sbin/usermod

- The whole business of splitting the spec file into different *.inc
  files ...  I can't see this getting past a Fedora review, so I suggest
  that you don't do it.

- There's some pretty funky stuff going on in scripts, such as backing
  up directories before they are removed by RPM (and thereby bypassing
  the whole purpose of RPM).  What is the purpose of this and how much
  of this can be avoided?  Note that scripts are (a) the thing most likely
  to fail during RPM installation and (b) the hardest thing to debug because
  it happens on someone else's computer, so it's in your interest to
  make scripts as simple/non-existent as possible.

(In reply to comment #9)
> Some comments on the spec file:

Thank you very much for the review.

> - Why does the spec file contain a huge license section at the
>   top?  I think you should avoid this, unless there is some pressing
>   reason for a specific license on the spec file itself.

No need for that, I will remove it.

> - py_site_pkgs uses %define, should almost certainly be using %global.

You are right, missed that one.

> - These seem to be unnecessary.  I would remove them and use the programs
>   directly.
> %global __getent /usr/bin/getent
> %global __groupadd /usr/sbin/groupadd
> %global __useradd /usr/sbin/useradd
> %global __usermod /usr/sbin/usermod

I will do that.

> - The whole business of splitting the spec file into different *.inc
>   files ...  I can't see this getting past a Fedora review, so I suggest
>   that you don't do it.

I thought that splitting the spec in several files could make it easier to maintain, but I agree with you that it is not common practice. I will revert that change.

> - There's some pretty funky stuff going on in scripts, such as backing
>   up directories before they are removed by RPM (and thereby bypassing
>   the whole purpose of RPM).  What is the purpose of this and how much
>   of this can be avoided?  Note that scripts are (a) the thing most likely
>   to fail during RPM installation and (b) the hardest thing to debug because
>   it happens on someone else's computer, so it's in your interest to
>   make scripts as simple/non-existent as possible.

I will review that and remove as much as possible.

Do you see any other important blocker for this package?

(In reply to comment #10)
> Do you see any other important blocker for this package?

It's complex, but it seems the complexity is largely
unavoidable.  Apart from that I don't see any blocker right
now, but you must first fix those things I pointed out, and
provide a new srpm.

I think that this version fixes all the issues you mention in comment #9. The updated spec and source package are available here:

http://jhernand.fedorapeople.org/rpms/ovirt-engine/3.0.0-10.0001

The updated package builds correctly in Koji:

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

Out of the rpmlint warnings, these looks suspicious:

ovirt-engine.noarch: E: non-executable-script /usr/share/ovirt-engine/scripts/vds_installer.py 0644L /usr/bin/python
ovirt-engine-log-collector.noarch: E: script-without-shebang /usr/lib/python2.7/site-packages/sos/plugins/postgresql.py
ovirt-engine-log-collector.noarch: E: script-without-shebang /usr/lib/python2.7/site-packages/sos/plugins/jboss.py

There are a lot of other warnings, but I can't see any of them
being problems.  eg. lots of complaints about "dangling symlinks"
but they all appear to be satisfied by Required packages, so they
wouldn't be a real problem (unless the dependent packages change ...)

We had a discussion on IRC about the version and release fields.
Currently they are:

Version: %{upstream_version}
Release: 10.%{upstream_release}%{?dist}

The usual rule is that "version belongs to upstream and release
belongs to Fedora", which would imply:

Version: %{upstream_version}.%{upstream_release}
Release: 10.%{?dist}

I don't think this is a blocker, but it would be interesting
to see what you think about making this change.

(In reply to comment #13)
> Out of the rpmlint warnings, these looks suspicious:

I can explain the reasons for those errors:

> ovirt-engine.noarch: E: non-executable-script
> /usr/share/ovirt-engine/scripts/vds_installer.py 0644L /usr/bin/python

This is script is not designed to run in the Fedora machine where it is installed: it is to be downloaded (via web) by other machines that will then execute it. So I think that it is better to have it without execution permissions.

> ovirt-engine-log-collector.noarch: E: script-without-shebang
> /usr/lib/python2.7/site-packages/sos/plugins/postgresql.py
> ovirt-engine-log-collector.noarch: E: script-without-shebang
> /usr/lib/python2.7/site-packages/sos/plugins/jboss.py

These are SOS plugins and they have execution permission and no shebang, as all the other SOS plugins.

> There are a lot of other warnings, but I can't see any of them
> being problems.  eg. lots of complaints about "dangling symlinks"
> but they all appear to be satisfied by Required packages, so they
> wouldn't be a real problem (unless the dependent packages change ...)

This is a general issue with rpmlint, I thoroughly checked that the symlinks are correct.

> We had a discussion on IRC about the version and release fields.
> Currently they are:
> 
> Version: %{upstream_version}
> Release: 10.%{upstream_release}%{?dist}
> 
> The usual rule is that "version belongs to upstream and release
> belongs to Fedora", which would imply:
> 
> Version: %{upstream_version}.%{upstream_release}
> Release: 10.%{?dist}

I also had this discussion (with myself). At the end I came to the conclusion that the "_0001" part of the upstream version number matches what in Fedora we call a post-release (see [1]). If the upstream project increases this correctly when they do new post-releases then it can go safely in the "Version" tag, as you suggest. However there is no history of upstream releases (this is the first one) so I can't be sure upstream is going to increase it correctly, so I decided to put it in the "Release" tag to be on the safe side.

That said, I am pretty sure next upstream release will be 3.1.x, so this won't be a problem.

I don't have anything against doing this change. Just let me know what you prefer.

> I don't think this is a blocker, but it would be interesting
> to see what you think about making this change.

Thanks again!

[1] http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Post-Release_packages

I'm pretty sure the right way is:

> Version: %{upstream_version}.%{upstream_release}
> Release: 10.%{?dist}

following the rule I quoted earlier.

I will do a formal review shortly.

Fixed. The updated spec and source package are available here:

http://jhernand.fedorapeople.org/rpms/ovirt-engine/3.0.0.0001-11

+ rpmlint output

Rpmlint output is extensive, but consists of false alarms or
things which the packager has assured me are not a problem.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license
+ %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
+ package successfully builds on at least one architecture

Built in Koji and on x86-64.

n/a ExcludeArch bugs filed
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
n/a binary RPM with shared library files must call ldconfig in %post and %postun
+ does not use Prefix: /usr
+ package owns all directories it creates
+ no duplicate files in %files
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
n/a header files should be in -devel
n/a static libraries should be in -static
n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
n/a libfoo.so must go in -devel
n/a -devel must require the fully versioned base
n/a packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ filenames must be valid UTF-8
+ use %global instead of %define

Optional:

n/a if there is no license file, packager should query upstream
n/a translations of description and summary for non-English languages, if available
+ reviewer should build the package in mock
n/a the package should build into binary RPMs on all supported architectures

It's a noarch package, so not applicable.

? review should test the package functions as described
+ scriptlets should be sane
n/a pkgconfig files should go in -devel
n/a shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or /usr/sbin

This package is APPROVED by rjones
----------------------------------

Thank you very much Richard! Please let me know if you have any suggestion to improve the package.

New Package SCM Request
=======================
Package Name: ovirt-engine
Short Description: Management server for Open Virtualization
Owners: jhernand
Branches: f17
InitialCC:

Git done (by process-git-requests).

Thanks a lot Jon!

ovirt-engine-3.0.0.0001-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/ovirt-engine-3.0.0.0001-1.fc17

ovirt-engine-3.0.0.0001-1.fc17 has been pushed to the Fedora 17 testing repository.

ovirt-engine-3.0.0.0001-12.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/ovirt-engine-3.0.0.0001-12.fc17

ovirt-engine-3.0.0.0001-12.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.