Bug 831878

Here's an informal review as I'm not a sponsor.

Key:
- = N/A
x = Pass
! = Fail

==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[N/A]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[!]: MUST Buildroot is not present
See https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: MUST Sources contain only permissible code or content.
[x]: MUST %config files are marked noreplace or the reason is justified.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
[N/A]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
See https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

[N/A]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: 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 is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
There are however no license notes in source code. I see you've been the
upstream so could you add them ?
See the APPENDIX in http://www.apache.org/licenses/LICENSE-2.0

[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST No %config files under /usr.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

> rpmlint ovirt-log-collector-3.1.0-0.fc18.src.rpm
> ovirt-log-collector.src: W: non-standard-group Virtualization/Management
> ovirt-log-collector.src: W: invalid-url Source0: http://ovirt.org/releases/stable/src/ovirt-log-collector-3.1.0.tar.gz HTTP Error 404: Not Found
> 1 packages and 0 specfiles checked; 0 errors, 2 warnings.

Source doesn't exist.

> rpmlint ovirt-log-collector-3.1.0-0.fc18.noarch.rpm
> ovirt-log-collector.noarch: W: non-standard-group Virtualization/Management
> ovirt-log-collector.noarch: W: incoherent-version-in-changelog 1.0.0-0 ['3.1.0-0.fc18', '3.1.0-0']

Start the release tag from 1.
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag

> ovirt-log-collector.noarch: W: manual-page-warning /usr/share/man/man8/engine-log-collector.8.gz 28: name expected (got a special character): treated as missing

Not sure what this means, but it's a warning anyway.

> ovirt-log-collector.noarch: E: non-readable /etc/ovirt-engine/logcollector.conf 0600L

Could you change it to 0644 ?

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

Either unset executable bits or add shebang.

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST File names are valid UTF-8.
[N/A]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD Dist tag is present.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).

Other notes:
> %doc  %{_mandir}/man8/engine-log-collector.8.gz
I can't say the %doc macro is wrong here, but I've never seen it
together with %{_mandir} so I'd remove it.

> %doc AUTHORS
> %doc LICENSE
You can put it on one line like: '%doc AUTHORS LICENSE'
and %doc is usually the first line in %files.

Ping, any news here?

Your spec files look fine and as you have currently 3 open tickets, this is fine to sponsor you.

Could you please resolve the issues from above?

Some comments to above:

(In reply to comment #1)
> > ovirt-log-collector.noarch: W: manual-page-warning /usr/share/man/man8/engine-log-collector.8.gz 28: name expected (got a special character): treated as missing
> 
> Not sure what this means, but it's a warning anyway.

$ rpmlint -I manual-page-warning
manual-page-warning:
This man page may contain problems that can cause it not to be formatted as
intended.

> > %doc  %{_mandir}/man8/engine-log-collector.8.gz
> I can't say the %doc macro is wrong here, but I've never seen it
> together with %{_mandir} so I'd remove it.

The man pages are marked as %doc automatically, so you don't need to add it on your own:
http://permalink.gmane.org/gmane.linux.redhat.fedora.extras.packaging/8236

(In reply to comment #2)
> > > %doc  %{_mandir}/man8/engine-log-collector.8.gz

Ah and while we are at it:
It would be best to include this as engine-log-collector.8.* so the format of the man page can change easily.

Pong, been on PTO.  I'll review the suggestions tomorrow and respond. Cheers

First, thanks for the review!

Here are my responses:

Problem 1:
>[!]: MUST Buildroot is not present
>See https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
I'm not sure what it is complaining about here.  I use %{buildroot} in the .spec. Further, the wiki says... "Fedora (as of F-10) does not require the presence of the BuildRoot tag in the spec...".

Problem 2:
>[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>     beginning of %install.
>See https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
Huh? The spec file does exactly this that.  Here are the relevant lines from my spec:
 %install
 rm -rf %{buildroot}/*  <--- See.
 make PREFIX=%{buildroot}/ install

Problem 3:
>[!]: MUST Rpmlint output is silent.
> ovirt-log-collector.src: W: non-standard-group Virtualization/Management
Fixed. Set to "Applications/System".
> ovirt-log-collector.src: W: invalid-url Source0: http://ovirt.org/releasesI
Fixed.

Problem 4: 
> rpmlint ovirt-log-collector-3.1.0-0.fc18.noarch.rpm
> ovirt-log-collector.noarch: W: non-standard-group Virtualization/Management
Fixed.
> ovirt-log-collector.noarch: W: incoherent-version-in-changelog 1.0.0-0 ['3.1.0-0.fc18', '3.1.0-0']
Fixed.

Problem 5:
>> ovirt-log-collector.noarch: W: manual-page-warning /usr/share/man/man8/engine-log-collector.8.gz 28: name expected (got a special character): treated as missing
> Not sure what this means, but it's a warning anyway.
Yeah, I have no idea how to fix this.

Problem 6: 
> It would be best to include this as engine-log-collector.8.*
I tried this and rpmlint complained with the following:
 "ovirt-log-collector.noarch: W: manual-page-warning /usr/share/man/man8/engine-log-collector.8.gz 28: name expected (got a special character): treated as missing"

Problem 7: 
>> ovirt-log-collector.noarch: E: non-readable /etc/ovirt-engine/logcollector.conf 0600L
>Could you change it to 0644 ?
Actually, 0600 is the right permission set.  The user *could* choose optionally to set the password for the oVirt RESTful API in there. 

Problem 8: 
> Either unset executable bits or add shebang.
Fixed.  Unset exec bits.

Problem 9:
> I can't say the %doc macro is wrong here, but I've never seen it
together with %{_mandir} so I'd remove it
Removed.

Problem 10:
> You can put it on one line like: '%doc AUTHORS LICENSE'
Done

(In reply to comment #5)
> Problem 1:
> >[!]: MUST Buildroot is not present
> >See https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
> I'm not sure what it is complaining about here.  I use %{buildroot} in the
> .spec. Further, the wiki says... "Fedora (as of F-10) does not require the
> presence of the BuildRoot tag in the spec...".

Some checklists still have this item in it. EPEL-5 still needs it, when you are not building for it, you can delete it again:
https://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#Distribution_specific_guidelines

> Problem 2:
> >[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
> >     beginning of %install.
> >See https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
> Huh? The spec file does exactly this that.  Here are the relevant lines from
> my spec:
>  %install
>  rm -rf %{buildroot}/*  <--- See.
>  make PREFIX=%{buildroot}/ install

Same here, can be omitted, but you are deleting %{buildroot}/* and not %{buildroot}, which makes a difference for .foo files:
$ mkdir a
$ touch a/.b
$ rm a/*
rm: cannot remove `a/*': No such file or directory
$ ls a/.b
a/.b

> Problem 5:
> >> ovirt-log-collector.noarch: W: manual-page-warning /usr/share/man/man8/engine-log-collector.8.gz 28: name expected (got a special character): treated as missing
> > Not sure what this means, but it's a warning anyway.
> Yeah, I have no idea how to fix this.

$ man --warning -l ./src/rhev/engine-log-collector.8 > /dev/null 
<standard input>:28: name expected (got a special character): treated as missing

And line 28 contains:
 28 .\' Describe engine\-slimmed

And this is a proper comment in the man page, which won't be renderen by man without the warning (If that was what you wanted?):
 28 .\" Describe engine\-slimmed

> Problem 6: 
> > It would be best to include this as engine-log-collector.8.*
> I tried this and rpmlint complained with the following:
>  "ovirt-log-collector.noarch: W: manual-page-warning
> /usr/share/man/man8/engine-log-collector.8.gz 28: name expected (got a
> special character): treated as missing"

Same warning as above, fine otherwise.

> Problem 7: 
> >> ovirt-log-collector.noarch: E: non-readable /etc/ovirt-engine/logcollector.conf 0600L
> >Could you change it to 0644 ?
> Actually, 0600 is the right permission set.  The user *could* choose
> optionally to set the password for the oVirt RESTful API in there. 

Could you add a note in the spec file, so it's findable? :)


* Please upload also the spec file and link to the spec/srpm on each update in the review requests.

* "make PREFIX=%{buildroot} install" would be nicer as %{buildroot}/ or you'll have two slashes in the final install command.

* It would be great to add a %check section as there is src/rhev/tests.py, if possible.

* I don't like this:
Source0: http://kojak.fedorapeople.org/ovirt-log-collector-%{version}.tar.gz
Wouldn't it be better to release proper tarballs over here:
http://www.ovirt.org/releases/stable/src/ ?

Uploading the source at $random_website to fullfil the review doesn't work. When there aren't tarballs released, you'd need to describe how to the the sources from revision controll systems instead:
https://fedoraproject.org/wiki/Packaging:SourceURL

I consider the Source0 problem as a blocker, the rest above are nitpicks :)

(In reply to comment #2)
> Your spec files look fine and as you have currently 3 open tickets, this is
> fine to sponsor you.

It seems you are now sponsored and this was handled somehow differently than described over here:
http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

- Unblocking FE-NEEDSPONSOR.
- Reassigning to nobody, someone else is free to take it and complete this review.

Hi,  

could the url to the latest spec and srpm posted ( so I can run fedora-review on the bug, as there is no working url for srpm right now )

Hi,

(In reply to comment #5)
> Problem 5:
> >> ovirt-log-collector.noarch: W: manual-page-warning /usr/share/man/man8/engine-log-collector.8.gz 28: name expected (got a special character): treated as missing
> > Not sure what this means, but it's a warning anyway.
> Yeah, I have no idea how to fix this.

Looking at the sources, this comes from the line 

25 .\' Describe engine\-slimmed

Lines starting with a . in troff are commands, and \' is not a valid command, it's a special character. However, lots of man pages seem to use .\\ or .
\' at the start of lines, I have no idea why (or what tool is generating them).

If this is supposed to be a comment, it should be 
 ." Describe engine\-slimmed

Is it intended to be something else?

Dave.

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

Closing this review request since it is very old and submitter's bugzilla account is no longer active.