Bug 245015 - PolicyKit package review
PolicyKit package review
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Matthias Clasen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-20 11:35 EDT by David Zeuthen
Modified: 2013-03-05 22:51 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-31 15:54:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mclasen: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)
new spec file (4.36 KB, text/plain)
2007-07-27 12:29 EDT, David Zeuthen
no flags Details

  None (edit)
Comment 1 Jason Tibbitts 2007-06-20 12:34:50 EDT
Is mclasen reviewing this?  If so, please set fedora-review to '?'.  Thanks.
Comment 2 Matthias Clasen 2007-06-20 15:51:22 EDT
Preludes:

- Should use a full URL for the tarball

- Shouldn't have to BR both dbus-glib-devel and dbus-glib. Isn't one requiring
the other anyway ?

- -devel package needs to require pkgconfig 

- %post and %postun should just do -p /sbin/ldconfig if thats all they do

- file list: %doc %{_datadir}/doc/%{name}-%{version}/NEWS is somewhat unusual.
Normally, we just use those files out of the build tree.

- user creation: I tried to look at the packaging wiki docs for user creation,
but they are very confusing and don't give any clear guidance, but should you
nuke the user if the package is erased ?


Comment 3 Bill Nottingham 2007-06-20 15:58:33 EDT
Users should not be removed.
Comment 4 Matthias Clasen 2007-06-21 00:01:01 EDT
ok, here comes a more formal checklist

rpmlint output on PolicyKit-0.3-1.fc8.i386.rpm:
E: PolicyKit non-standard-uid /var/run/PolicyKit polkit
E: PolicyKit non-standard-gid /var/run/PolicyKit polkit
E: PolicyKit non-standard-dir-perm /var/run/PolicyKit 0775
E: PolicyKit non-standard-uid /var/lib/PolicyKit polkit
E: PolicyKit non-standard-gid /var/lib/PolicyKit polkit
E: PolicyKit non-standard-dir-perm /var/lib/PolicyKit 0775
E: PolicyKit non-standard-gid /usr/libexec/polkit-grant-helper polkit
E: PolicyKit setgid-binary /usr/libexec/polkit-grant-helper polkit 02755
E: PolicyKit non-standard-executable-perm /usr/libexec/polkit-grant-helper 02755

The errors about uid/gid should be covered by the bug asking for
a standard uid/gid

The errors about permissions should probably be handled
by adding a comment explaining why these permissions are necessary

W: PolicyKit incoherent-version-in-changelog 0.3.0-1 0.3-1.fc8

This should be corrected

W: PolicyKit invalid-license AFL/GPL

I believe rpmlint is just dumb here

W: PolicyKit conffile-without-noreplace-flag /etc/PolicyKit/PolicyKit.conf
W: PolicyKit conffile-without-noreplace-flag /etc/pam.d/polkit

What about these, David ? Is there any good reason not to make these
noreplace ?

rpmlint output on PolicyKit-docs-0.3-1.fc8.i386.rpm 
W: PolicyKit-docs invalid-license AFL/GPL

see above

W: PolicyKit one-line-command-in-%post /sbin/ldconfig
W: PolicyKit one-line-command-in-%postun /sbin/ldconfig

This has already been mentioned as something that should be changed

rpmlint output on PolicyKit-devel-0.3-1.fc8.i386.rpm
W: PolicyKit-devel no-documentation
W: PolicyKit-devel invalid-license AFL/GPL

the no-docs warning is ignorable, the other is already covered


package name: follows upstream tarball name, ok
spec file name: ok
packaging guidelines: see comment #2
license: ok. Small typo in  COPYING noticed in passing: 
    "[...] may be under the GPL only or under the LGPG."
license field: ok
license file: ok
American English: ok
legibility: pretty good
sources match upstream: ok
buildable: ok
excludearch: n/a
build requires: complete
locales: n/a
ldconfig: is run
relocatable: n/a
directory ownership: 
  - must require pam, for /etc/pam.d
  - must own /etc/PolicyKit, /usr/lib/PolicyKit, /usr/lib/PolicyKit/modules
file list duplicates: ok
file permissions: ok
%clean: ok
macro use: ok
content: permissable
doc subpackage: yes
%doc: ok
header files: ok
static libs: n/a
pc files: ok, see above for pkgconfig requirement
shared libs: ok
-devel requires: ok
libtool archives: ok
desktop files: n/a
directory ownership again: see above
%install cleans build root: yes
filenames utf8: ok 
Comment 5 Matthias Clasen 2007-07-26 12:51:56 EDT
David did a new release and new srpms here: 

http://people.freedesktop.org/~david/release-o-rama-july-2007/
Comment 6 Matthias Clasen 2007-07-26 13:24:26 EDT
Some of the earlier comments still apply:

- Shouldn't have to BR both dbus-glib-devel and dbus-glib. Isn't one requiring
  the other anyway ?

- -devel package needs to require pkgconfig 

- -docs package needs to require gtk-doc

- must require pam, for /etc/pam.d

- must own /etc/PolicyKit, /usr/lib/PolicyKit, /usr/lib/PolicyKit/modules

- %post and %postun should just do -p /sbin/ldconfig if thats all they do

- The errors about permissions should probably be handled
  by adding a comment explaining why these permissions are necessary
Comment 7 David Zeuthen 2007-07-27 12:26:59 EDT
(In reply to comment #6)
> Some of the earlier comments still apply:
> 
> - Shouldn't have to BR both dbus-glib-devel and dbus-glib. Isn't one requiring
>   the other anyway ?

Fixed.

> - -devel package needs to require pkgconfig 

Fixed.

> - -docs package needs to require gtk-doc

Fixed (with a vengeful comment).

> - must require pam, for /etc/pam.d

Fixed.

> - must own /etc/PolicyKit, /usr/lib/PolicyKit, /usr/lib/PolicyKit/modules

Fixed. The latter does not exist anymore FWIW.

> - %post and %postun should just do -p /sbin/ldconfig if thats all they do

Gee. Fixed, anyway.

> - The errors about permissions should probably be handled
>   by adding a comment explaining why these permissions are necessary

I've added a comment that this is explained in the upstream design docs.

Will attach new spec file.
Comment 8 David Zeuthen 2007-07-27 12:29:03 EDT
Created attachment 160123 [details]
new spec file
Comment 9 Matthias Clasen 2007-07-27 12:30:32 EDT
Looks fine now. APPROVED
Comment 10 David Zeuthen 2007-07-27 12:38:44 EDT
New Package CVS Request
=======================
Package Name: PolicyKit
Short Description: Fine-grained policy management
Owners: davidz@redhat.com
Branches:
InitialCC: 
Comment 11 David Zeuthen 2007-07-31 15:54:43 EDT
Built in pkg cvs.

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