[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878 Otto Urpelainen changed: What|Removed |Added Blocks||201449 (FE-DEADREVIEW) Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=201449 [Bug 201449] FE-DEADREVIEW -- Reviews stalled due to lack of submitter response should be blocking this bug. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878 Otto Urpelainen changed: What|Removed |Added Status|NEW |CLOSED CC||otu...@iki.fi Resolution|--- |NOTABUG Last Closed||2021-05-23 07:19:50 --- Comment #14 from Otto Urpelainen --- Closing this review request since it is very old and submitter's bugzilla account is no longer active. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
needinfo canceled: [Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
Product: Fedora Version: rawhide Component: Package Review Package Review has canceled Package Review 's request for Michael S. 's needinfo: Bug 831878: Review Request: ovirt-log-collector - Log collection tool for oVirt https://bugzilla.redhat.com/show_bug.cgi?id=831878 --- Comment #13 from Package Review --- 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. ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=831878 Dave Neary dne...@redhat.com changed: What|Removed |Added CC||dne...@redhat.com --- Comment #9 from Dave Neary dne...@redhat.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=cXOyWNgEJpa=cc_unsubscribe ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878 Michael Scherer m...@zarb.org changed: What|Removed |Added CC||m...@zarb.org Assignee|nob...@fedoraproject.org|m...@zarb.org Flags||fedora-review? --- Comment #8 from Michael Scherer m...@zarb.org --- 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 ) -- You are receiving this mail because: You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878 Thomas Spura toms...@fedoraproject.org changed: What|Removed |Added Blocks|177841 (FE-NEEDSPONSOR) | Assignee|toms...@fedoraproject.org |nob...@fedoraproject.org Flags|fedora-review? | --- Comment #7 from Thomas Spura toms...@fedoraproject.org --- (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. -- You are receiving this mail because: You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878
Keith Robertson krobe...@redhat.com changed:
What|Removed |Added
Flags||needinfo?(tomspur@fedorapro
||ject.org)
--- Comment #5 from Keith Robertson krobe...@redhat.com ---
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
--
You are receiving this mail because:
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878
Thomas Spura toms...@fedoraproject.org changed:
What|Removed |Added
Status|NEW |ASSIGNED
Assignee|nob...@fedoraproject.org|toms...@fedoraproject.org
Flags|needinfo?(tomspur@fedorapro |fedora-review?
|ject.org) |
--- Comment #6 from Thomas Spura toms...@fedoraproject.org ---
(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 :)
--
You are receiving this mail because:
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878
Thomas Spura toms...@fedoraproject.org changed:
What|Removed |Added
CC||toms...@fedoraproject.org
--- Comment #2 from Thomas Spura toms...@fedoraproject.org ---
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
--
You are receiving this mail because:
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878
--- Comment #3 from Thomas Spura toms...@fedoraproject.org ---
(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.
--
You are receiving this mail because:
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878 --- Comment #4 from Keith Robertson krobe...@redhat.com --- Pong, been on PTO. I'll review the suggestions tomorrow and respond. Cheers -- You are receiving this mail because: You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878
Jiri Popelka jpope...@redhat.com changed:
What|Removed |Added
CC||jpope...@redhat.com
--- Comment #1 from Jiri Popelka jpope...@redhat.com ---
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
[Bug 831878] Review Request: ovirt-log-collector - Log collection tool for oVirt
https://bugzilla.redhat.com/show_bug.cgi?id=831878 Keith Robertson krobe...@redhat.com changed: What|Removed |Added Blocks||177841 (FE-NEEDSPONSOR) -- You are receiving this mail because: You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
