Bug 245015 - PolicyKit package review
Summary: PolicyKit package review
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
low
low
Target Milestone: ---
Assignee: Matthias Clasen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-06-20 15:35 UTC by David Zeuthen
Modified: 2013-03-06 03:51 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-31 19:54:43 UTC
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 16:29 UTC, David Zeuthen
no flags Details

Comment 1 Jason Tibbitts 2007-06-20 16:34:50 UTC
Is mclasen reviewing this?  If so, please set fedora-review to '?'.  Thanks.

Comment 2 Matthias Clasen 2007-06-20 19:51:22 UTC
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 19:58:33 UTC
Users should not be removed.

Comment 4 Matthias Clasen 2007-06-21 04:01:01 UTC
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 16:51:56 UTC
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 17:24:26 UTC
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 16:26:59 UTC
(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 16:29:03 UTC
Created attachment 160123 [details]
new spec file

Comment 9 Matthias Clasen 2007-07-27 16:30:32 UTC
Looks fine now. APPROVED

Comment 10 David Zeuthen 2007-07-27 16:38:44 UTC
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 19:54:43 UTC
Built in pkg cvs.


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