This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 807017 - Review Request: ovirt-engine - Management server for Open Virtualization
Review Request: ovirt-engine - Management server for Open Virtualization
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity medium
: ---
: ---
Assigned To: Richard W.M. Jones
Fedora Extras Quality Assurance
:
Depends On: 809395
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-26 15:00 EDT by Juan Hernández
Modified: 2012-05-04 18:57 EDT (History)
6 users (show)

See Also:
Fixed In Version: ovirt-engine-3.0.0.0001-12.fc17
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-04 18:57:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
rjones: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Fedora-Review output (95.03 KB, text/plain)
2012-03-28 11:43 EDT, Ofer Schreiber
no flags Details

  None (edit)
Description Juan Hernández 2012-03-26 15:00:01 EDT
Spec URL:

http://jhernand.fedorapeople.org/rpms/ovirt-engine/3.0.0_0001-6/ovirt-engine.spec

SRPM URL:

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

Description:

oVirt Engine is a feature-rich server virtualization management
system that provides advanced capabilities for managing the Open
virtualization infrastructure for Servers and Desktops.

Koji build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3934341
Comment 1 Juan Hernández 2012-03-26 15:13:15 EDT
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.
Comment 2 Ofer Schreiber 2012-03-27 11:47:44 EDT
I'm trying to do a review, but the mock build fails due to:
Error: No Package found for spring-ldap
Comment 3 Juan Hernández 2012-03-27 11:55:00 EDT
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!
Comment 4 Ofer Schreiber 2012-03-28 11:43:29 EDT
Created attachment 573382 [details]
Fedora-Review output
Comment 5 Ofer Schreiber 2012-03-28 11:49:12 EDT
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.
Comment 6 Juan Hernández 2012-03-28 11:51:12 EDT
Ofer, thank you very much for the review!

Quite a long list of things to take care of. I start inmediately to fix them.
Comment 7 Juan Hernández 2012-04-01 08:04:32 EDT
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.
Comment 8 Steven Dake 2012-04-09 20:27:14 EDT
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
Comment 9 Richard W.M. Jones 2012-04-12 06:19:39 EDT
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.
Comment 10 Juan Hernández 2012-04-12 06:28:42 EDT
(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?
Comment 11 Richard W.M. Jones 2012-04-12 06:39:35 EDT
(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.
Comment 12 Juan Hernández 2012-04-12 07:26:31 EDT
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
Comment 13 Richard W.M. Jones 2012-04-12 08:58:53 EDT
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.
Comment 14 Juan Hernández 2012-04-12 09:28:22 EDT
(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
Comment 15 Richard W.M. Jones 2012-04-12 12:18:48 EDT
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.
Comment 16 Juan Hernández 2012-04-12 12:55:38 EDT
Fixed. The updated spec and source package are available here:

http://jhernand.fedorapeople.org/rpms/ovirt-engine/3.0.0.0001-11
Comment 17 Richard W.M. Jones 2012-04-13 07:31:35 EDT
+ 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
----------------------------------
Comment 18 Juan Hernández 2012-04-13 07:34:33 EDT
Thank you very much Richard! Please let me know if you have any suggestion to improve the package.
Comment 19 Juan Hernández 2012-04-13 07:36:53 EDT
New Package SCM Request
=======================
Package Name: ovirt-engine
Short Description: Management server for Open Virtualization
Owners: jhernand
Branches: f17
InitialCC:
Comment 20 Jon Ciesla 2012-04-13 08:08:58 EDT
Git done (by process-git-requests).
Comment 21 Juan Hernández 2012-04-13 08:12:54 EDT
Thanks a lot Jon!
Comment 22 Fedora Update System 2012-04-13 10:13:14 EDT
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
Comment 23 Fedora Update System 2012-04-13 21:46:44 EDT
ovirt-engine-3.0.0.0001-1.fc17 has been pushed to the Fedora 17 testing repository.
Comment 24 Fedora Update System 2012-04-24 16:50:35 EDT
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
Comment 25 Fedora Update System 2012-05-04 18:57:11 EDT
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.

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