[Bug 502920] New: Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

https://bugzilla.redhat.com/show_bug.cgi?id=502920

Summary: Review Request: polkit-gnome - PolicyKit integration
                    for the GNOME desktop
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail], [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora
    Target Release: ---

Spec URL: http://people.freedesktop.org/~david/polkit-gnome.spec
SRPM URL:
http://people.freedesktop.org/~david/polkit-gnome-0.92-0.git20090527.fc11.sr...
Description: PolicyKit integration for the GNOME desktop.

This depends on polkit, see bug 502919.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #1 from Matthias Clasen   2009-05-28 15:10:06 EDT ---
Created an attachment (id=345821)
 --> (https://bugzilla.redhat.com/attachment.cgi?id=345821)
add build requires

This spec adds enough build requires to make the package build.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

Matthias Clasen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #2 from 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.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

Matthias Clasen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #345821|0                           |1
        is obsolete|                            |

--- Comment #3 from Matthias Clasen   2009-05-28 15:42:49 EDT ---
Created an attachment (id=345829)
 --> (https://bugzilla.redhat.com/attachment.cgi?id=345829)
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.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #4 from 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

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

David Zeuthen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #345829|application/octet-stream    |text/plain
          mime type|                            |

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

David Zeuthen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #345829|0                           |1
        is obsolete|                            |

--- Comment #5 from David Zeuthen   2009-05-29 13:19:06 EDT ---
Created an attachment (id=345932)
 --> (https://bugzilla.redhat.com/attachment.cgi?id=345932)
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=96e92020e8f98b5b290215d...

and this will be in 0.92 (won't build it anyway until 0.92 releases are out).

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

Matthias Clasen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review+

--- Comment #6 from Matthias Clasen   2009-05-29 13:24:05 EDT ---
ok, approved. 
I assume the 0.92 releases will find a permanent upstream location, too...

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

David Zeuthen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #7 from 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:

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

Jason Tibbitts  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-cvs?                 |fedora-cvs+

--- Comment #8 from Jason Tibbitts   2009-05-29 14:21:03 EDT ---
CVS done.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

Christoph Wickert  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
         AssignedTo|[e-mail]    |[e-mail]

--- Comment #9 from 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.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #10 from 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

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #11 from 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.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #12 from 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.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #13 from 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.

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #14 from Christoph Wickert   2009-05-31 18:02:45 EDT ---
You call a wrong license tag and missing parallel build no problems?

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #15 from 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...

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

--- Comment #16 from 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. ;)

[Bug 502920] Review Request: polkit-gnome - PolicyKit integration for the GNOME desktop

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=502920

Matthias Clasen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |RAWHIDE