Bug 442522
Summary: | Review Request: audit-viewer - Audit event viewer | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Miloslav Trmač <mitr> |
Component: | Package Review | Assignee: | Lubomir Rintel <lkundrak> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, notting |
Target Milestone: | --- | Flags: | lkundrak:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2008-05-03 23:50:42 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Miloslav Trmač
2008-04-15 10:55:25 UTC
1.) Release: 1 Please include the dist tag http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-beca3bf84972f19a384cc2e5091ed47c2b3cebc7 2.) You use update-desktop-database and update-mime-database Please depend on required packages: Requires(post): desktop-file-utils Requires(postun): desktop-file-utils SPEC file seems sane in all other respect. Will continue review once mock build finishes. 3.) mock build fails, probably due to lacking dependency on perl(XML::Parser): checking for XML::Parser... configure: error: XML::Parser perl module is required for intltool 4.) rpmlint: $ rpmlint -i audit-viewer-0.2-1.src.rpm audit-viewer-0.2-1.x86_64.rpm audit-viewer-debuginfo-0.2-1.x86_64.rpm audit-viewer.src: W: no-url-tag The URL tag is missing. audit-viewer.x86_64: W: symlink-should-be-relative /usr/libexec/audit-viewer-server /usr/bin/consolehelper Absolute symlinks are problematic eg. when working with chroot environments. audit-viewer.x86_64: W: no-url-tag The URL tag is missing. audit-viewer-debuginfo.x86_64: W: no-url-tag The URL tag is missing. The URL tag is not a problem for now given what the comment above URL tag says. However you should correct the absolute symlink. Have you considered using PolicyKit instread of consolehelper? (In reply to comment #1) > 1.) Release: 1 > Please include the dist tag The linked document states the dist tag is optional. Is it really necessary? > 2.) You use update-desktop-database and update-mime-database > > Please depend on required packages: > Requires(post): desktop-file-utils > Requires(postun): desktop-file-utils http://fedoraproject.org/wiki/Packaging/ScriptletSnippets recommends not to depend on these packages - to allow installing the package in non-GNOME environments without dragging these packages in. (In reply to comment #4) > (In reply to comment #1) > > 1.) Release: 1 > > Please include the dist tag > The linked document states the dist tag is optional. Is it really necessary? Common practice is to include it for packages that contains code that is linked to other packages in the distribution. It is usually ommited in content-only packages, such as clip art gallery or firmware packages. Moreover, it simplifies updating the branches with bugfixes, see the paragraph below. Given the guidelines do not require the dist tag, this won't block the package approval. But I encourage you to reconsider its use. > > 2.) You use update-desktop-database and update-mime-database > > > > Please depend on required packages: > > Requires(post): desktop-file-utils > > Requires(postun): desktop-file-utils > http://fedoraproject.org/wiki/Packaging/ScriptletSnippets recommends not to > depend on these packages - to allow installing the package in non-GNOME > environments without dragging these packages in. Oh, sorry. Ignore that then. 5.) One more thing: /usr/share/applications/audit-viewer.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6 Please consider setting vendor to Fedora (In reply to comment #3) > 4.) rpmlint: > audit-viewer.src: W: no-url-tag Added, now that audit-viewer is at fedorahosted.org > audit-viewer.x86_64: W: symlink-should-be-relative > /usr/libexec/audit-viewer-server /usr/bin/consolehelper > Absolute symlinks are problematic eg. when working with chroot environments. There's really no way to use that symlink in a chroot anyway: - There's no reason to use it to copy the consolehelper binary anywhere. - If you use the symlink to run audit-viewer-server, then consolehelper will access /etc/security/console.apps/audit-viewer-server, check users from /etc/passwd and run /usr/libexec/audit-viewer-server-real; this will all happen in the "top" root and it will happen the same whether the running consolehelper is /usr/bin/consolehelper or /chroot/usr/bin/consolehelper. OTOH using an absolute symlink somewhat protect us in case %{libexecdir} was moved. > Have you considered using PolicyKit instread of consolehelper? Briefly. Right now the privileged part is userhelper and roughly 300 lines of custom C code, and modifying the code to rely on libpolkit would probably not be a security improvement (although the PolicyKit features are somewhat compelling). (In reply to comment #2) > 3.) mock build fails, probably due to lacking dependency on perl(XML::Parser): > > checking for XML::Parser... > configure: error: XML::Parser perl module is required for intltool Fixed. Sorry about that - I really should have noticed this myself. (In reply to comment #6) > 5.) One more thing: > > /usr/share/applications/audit-viewer.desktop: warning: key "Encoding" in group > "Desktop Entry" is deprecated Removed. > http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6 > Please consider setting vendor to Fedora Fixed. New package at http://people.redhat.com/mitr/packaging/audit-viewer-0.2-2.src.rpm ; spec file updated at the original URL. (In reply to comment #7) > (In reply to comment #3) > > 4.) rpmlint: > > audit-viewer.src: W: no-url-tag > Added, now that audit-viewer is at fedorahosted.org Not really -- you just added a comment, not changed the URL tag. Given this is an upstream issue and relation between upstream and package is not relevant here, it can not block the review. I encourage you to upload the source to https://fedorahosted.org/releases/ and change the url tag respectively as described here: https://fedorahosted.org/web/faq > > > audit-viewer.x86_64: W: symlink-should-be-relative > > /usr/libexec/audit-viewer-server /usr/bin/consolehelper > > Absolute symlinks are problematic eg. when working with chroot environments. > There's really no way to use that symlink in a chroot anyway: > - There's no reason to use it to copy the consolehelper binary anywhere. > - If you use the symlink to run audit-viewer-server, then consolehelper > will access /etc/security/console.apps/audit-viewer-server, check users > from /etc/passwd and run /usr/libexec/audit-viewer-server-real; this will > all happen in the "top" root and it will happen the same whether the > running consolehelper is /usr/bin/consolehelper or > /chroot/usr/bin/consolehelper. > OTOH using an absolute symlink somewhat protect us in case %{libexecdir} was moved. I don't believe there's a guideline for these and was not able to find any. I see no more problems with the package. APPROVED (In reply to comment #9) > I encourage you to upload the source to https://fedorahosted.org/releases/ and > change the url tag respectively as described here: https://fedorahosted.org/web/faq Oh, i didn't know about that. Thanks for the pointer. New Package CVS Request ======================= Package Name: audit-viewer Short Description: A graphical utility for viewing and summarizing audit events. Owners: mitr Branches: F-9 InitialCC: Cvsextras Commits: yes cvs done. Package Change Request ====================== Package Name: audit-viewer New Branches: F-8 cvs done. |