Bug 502920 - Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop
Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Clasen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-27 15:15 EDT by David Zeuthen
Modified: 2014-01-02 09:19 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-07 12:56:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mclasen: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
add build requires (1.23 KB, application/octet-stream)
2009-05-28 15:10 EDT, Matthias Clasen
no flags Details
pendantic fixes (1.45 KB, text/plain)
2009-05-28 15:42 EDT, Matthias Clasen
no flags Details
nuked --vendor (1.44 KB, text/plain)
2009-05-29 13:19 EDT, David Zeuthen
no flags Details

  None (edit)
Description David Zeuthen 2009-05-27 15:15:27 EDT
Spec URL: http://people.freedesktop.org/~david/polkit-gnome.spec
SRPM URL: http://people.freedesktop.org/~david/polkit-gnome-0.92-0.git20090527.fc11.src.rpm
Description: PolicyKit integration for the GNOME desktop.

This depends on polkit, see bug 502919.
Comment 1 Matthias Clasen 2009-05-28 15:10:06 EDT
Created attachment 345821 [details]
add build requires

This spec adds enough build requires to make the package build.
Comment 2 Matthias Clasen 2009-05-28 15:14:56 EDT
rpmlint output

[mclasen@planemask rpmbuild]$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
polkit-gnome.src: W: non-standard-group Unspecified
polkit-gnome.src: W: no-url-tag
polkit-gnome.x86_64: W: non-standard-group Unspecified
polkit-gnome.x86_64: W: no-url-tag
polkit-gnome.x86_64: W: no-documentation
polkit-gnome-debuginfo.x86_64: W: no-url-tag
3 packages and 0 specfiles checked; 0 errors, 6 warnings.
Comment 3 Matthias Clasen 2009-05-28 15:42:49 EDT
Created attachment 345829 [details]
pendantic fixes

This spec file has some more pedantic fixes. With this, rpmlint reports:

3 packages and 0 specfiles checked; 0 errors, 0 warnings.

Basing the review on this.
Comment 4 Matthias Clasen 2009-05-28 16:08:35 EDT
checklist

rpmlint: see above
package name: matches tarball name, ok
spec file name: ok
packaging guidelines: ok
license: ok
license field: ok
license file: ok, but should probably updated to be straight LGPL, since the daemon etc are gone

spec file language: ok
spec file legible: ok
upstream sources: missing
buildable: ok
excludearch: n/a
build deps: ok
locales: ok
shared libs: n/a
relocatable: n/a
directory ownership: ok
duplicate files: ok
permissions: ok
%clean: ok
macro use: ok
content: ok
large docs: n/a
%doc content: ok
header files: n/a
static libs: n/a
pc files: n/a
shared libs: n/a
devel deps: ok
gui apps: ok
file ownership: ok
%install: ok
utf8 filenames: ok


Should drop the --vendor gnome in the desktop-file-install call
Comment 5 David Zeuthen 2009-05-29 13:19:06 EDT
Created attachment 345932 [details]
nuked --vendor

Nuked the --vendor for desktop-file install.

I've fixed the COPYING file in the upstream tree

http://git.gnome.org/cgit/PolicyKit-gnome/commit/?id=96e92020e8f98b5b290215d9f6c7e02861c7bfd5

and this will be in 0.92 (won't build it anyway until 0.92 releases are out).
Comment 6 Matthias Clasen 2009-05-29 13:24:05 EDT
ok, approved. 
I assume the 0.92 releases will find a permanent upstream location, too...
Comment 7 David Zeuthen 2009-05-29 13:29:15 EDT
New Package CVS Request
=======================
Package Name: polkit-gnome
Short Description: PolicyKit integration for the GNOME desktop
Owners: davidz
Branches: 
InitialCC:
Comment 8 Jason Tibbitts 2009-05-29 14:21:03 EDT
CVS done.
Comment 9 Christoph Wickert 2009-05-29 15:26:18 EDT
I really think this package should Require gnome-session, especially as it's just for directory ownership. In this case duplicate ownership is perfectly ok as none of the packages really requires the other.

If ths package required gnome-session we will have a *lot* of Gnome bits (GConf2, control-center, gnome-desktop, gnome-icon-theme, gnome-menus and their deps) on every livecd that contains a system-config tool or requies polkit-gnome somehow. Please reconsider this requirement.
Comment 10 Christoph Wickert 2009-05-29 19:09:05 EDT
(In reply to comment #9)
> I really think this package should Require gnome-session,
                                    ^
Sorry, the word NOT was missing here.

Some more comments:
- License tag is wrong, should be LGPLv2+ instead of LGPLv2
- Why does %configure check for GConf? 
- "BuildRequires: gnome-doc-utils" seems wrong to me, AFAIKS only gtkdoc-check from gtk-doc is needed.
- What is the use of " --enable-gtk-doc" if there are no docs?
- package does not use parallel make, see
  https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make
- Timestamps not preserved during make install, see
  https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
- This is the successor of PolicyKit-gnome, but the Provides and Obsoletes for it are missing.
- HACKING and TODO are missing from %doc, possibly also NEWS, but this one is very outdated
Comment 11 Matthias Clasen 2009-05-29 22:06:17 EDT
- Why does %configure check for GConf? 

Because it does. Remember, this is a git snapshot, so don't expect perfect polish.


- "BuildRequires: gnome-doc-utils" seems wrong to me, AFAIKS only gtkdoc-check
from gtk-doc is needed.

Well, thats what you think. Had you tried to build in mock, like I did, you'd see that gnome-doc-utils is needed for the build to succeed.

- This is the successor of PolicyKit-gnome, but the Provides and Obsoletes for
it are missing.

You want us to break rawhide until all the porting is done ? Really ?


- HACKING and TODO are missing from %doc, possibly also NEWS, but this one is
very outdated  

Both of these are not useful at all in a non-devel package, I'd say.
Comment 12 Christoph Wickert 2009-05-31 06:37:49 EDT
(In reply to comment #11)
> - "BuildRequires: gnome-doc-utils" seems wrong to me, AFAIKS only gtkdoc-check
> from gtk-doc is needed.
> 
> Well, thats what you think. Had you tried to build in mock, like I did, you'd
> see that gnome-doc-utils is needed for the build to succeed.
> 
> - This is the successor of PolicyKit-gnome, but the Provides and Obsoletes for
> it are missing.
> 
> You want us to break rawhide until all the porting is done ? Really ?

No, but I want this to be in the spec, even if it's commented out, so the reviewer could verify it is correct.

> - HACKING and TODO are missing from %doc, possibly also NEWS, but this one is
> very outdated  
> 
> Both of these are not useful at all in a non-devel package, I'd say.  

As long as we have no devel package they should be in the base package I think.

The main problem I see is that you rewrote the spec and based your review on the rewrite. This renders the review pretty useless because nobody will realize his own errors.
Comment 13 Matthias Clasen 2009-05-31 17:45:43 EDT
> The main problem I see is that you rewrote the spec and based your review on
> the rewrite. This renders the review pretty useless because nobody will realize
> his own errors.  

No, the main problem is that you have 'licked blood' now and go out of your way to find issues where there are none.
Comment 14 Christoph Wickert 2009-05-31 18:02:45 EDT
You call a wrong license tag and missing parallel build no problems?
Comment 15 Matthias Clasen 2009-05-31 18:41:43 EDT
The license tag was a minor oversight indeed. Thanks for finding that.
I don't see any problem with parallel build. Have you verified that there are no race conditions in the package if you turn it on ? See, me neither...
Comment 16 Christoph Wickert 2009-06-05 13:49:45 EDT
(In reply to comment #15)
> I don't see any problem with parallel build. Have you verified that there are
> no race conditions in the package if you turn it on ? See, me neither...  

Well, that's one of the things a reviewer should check. ;)
Comment 17 leigh scott 2013-12-25 13:44:53 EST
Package Change Request
======================
Package Name: polkit-gnome
New Branches: el7
Owners: leigh123linux
Comment 18 Gwyn Ciesla 2014-01-02 07:43:43 EST
No such branch as yet.
Comment 19 Jens Petersen 2014-01-02 07:54:08 EST
Perhaps you could try epel7 but I am not sure if it is completely ready yet.
Comment 20 leigh scott 2014-01-02 08:09:35 EST
(In reply to Jens Petersen from comment #19)
> Perhaps you could try epel7 but I am not sure if it is completely ready yet.

Thanks thats listed at koji as a tag

https://koji.fedoraproject.org/koji/buildtargetinfo?targetID=124


Package Change Request
======================
Package Name: polkit-gnome
New Branches: epel7
Owners: leigh123linux
Comment 21 Gwyn Ciesla 2014-01-02 09:19:24 EST
Git done (by process-git-requests).

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