Bug 847419 (mate-polkit) - Review request: mate-polkit - Integrates polkit with the MATE Desktop environment
Summary: Review request: mate-polkit - Integrates polkit with the MATE Desktop environ...
Keywords:
Status: CLOSED ERRATA
Alias: mate-polkit
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: MATE-DE-tracker
TreeView+ depends on / blocked
 
Reported: 2012-08-11 00:58 UTC by Dan Mashal
Modified: 2012-09-17 22:25 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-09-02 00:28:23 UTC
Type: Bug
Embargoed:
rdieter: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Dan Mashal 2012-08-11 00:58:26 UTC
Spec URL: http://vicodan.fedorapeople.org/matespec/mate-polkit.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/mate-polkit-1.4.0-1.fc17.src.rpm
Description: Integrates polkit with the MATE Desktop environment

Comment 1 Rex Dieter 2012-08-19 00:10:23 UTC
I  can review

Comment 2 Rex Dieter 2012-08-19 00:28:22 UTC
1.  SHOULD remove
BuildRequires: gtk+-devel
it's not needed

naming: ok

licensing: ok

sources: ok
md5sum *.xz
7eccae36af5f9ee0e38a569fcd34e395  mate-polkit-1.4.0.tar.xz

macros: ok

scriptlets: mostly ok

SHOULD: remove 
update-desktop-database &> /dev/null || :
scriptlets, not needed here.

2.  -devel MUST own %{_includedir}/polkit-gtk-mate-1/ (and polkitgtkmate subdir)

imo, replace
%{_includedir}/polkit-gtk-mate-1/polkitgtkmate/polkitgtkmate.h
%{_includedir}/polkit-gtk-mate-1/polkitgtkmate/polkitgtkmatetypes.h
%{_includedir}/polkit-gtk-mate-1/polkitgtkmate/polkitlockbutton.h
with just
%{_includedir}/polkit-gtk-mate-1/

3. SHOULD drop dup'd
%doc
statement in -devel subpkg

4. SHOULD add
Provides: PolicyKit-authentication-agent
to  match other policy kit providers (like gnome-polkit, polkit-kde)

Comment 4 Rex Dieter 2012-08-19 02:16:02 UTC
there's still an extra
update-desktop-database &> /dev/null || :
scriptlet, please remove that before doing any builds.


APPROVED.

Comment 5 Dan Mashal 2012-08-19 02:22:17 UTC
Fixed that. Thanks Rex.

Comment 6 Dan Mashal 2012-08-19 02:23:09 UTC
New Package SCM Request
=======================
Package Name: mate-polkit
Short Description: Integrates polkit with the MATE Desktop environment 
Owners: vicodan rdieter raveit65
Branches: f16 f17 18
InitialCC:

Comment 7 Wolfgang Ulbrich 2012-08-19 08:35:25 UTC
pls use this before you import.

Provides: polkit-mate-authentication-agent-1

Fedora switch to polkit a long time ago.

And for -devel subpackage
Provides: mate-polkit-demo%{?_isa} = %{version}-%{release}

Comment 8 Rex Dieter 2012-08-19 12:47:27 UTC
For the 2 provides you suggest adding, please explain why you think they are needed.

Comment 9 leigh scott 2012-08-19 13:12:53 UTC
There is still an outstanding issue, move this to -devel where it belongs

%{_datadir}/gir-1.0/PolkitGtkMate-1.0.gir

Comment 10 Wolfgang Ulbrich 2012-08-19 13:58:41 UTC
(In reply to comment #8)
> For the 2 provides you suggest adding, please explain why you think they are
> needed.

If i read this request , you are one wanted to add

4. SHOULD add
Provides: PolicyKit-authentication-agent
to  match other policy kit providers (like gnome-polkit, polkit-kde)

I use this also in my spec files, but i use polkit because current releases use polkit not policykit.
Maybe yum will nevertheless find polkit-mate-authentication-agent-1 with provide 'PolicyKit-authentication-agent', I can not estimate this.
If you say this will work.........


Forget the other provide in -devel, no need for it.

Comment 11 Gwyn Ciesla 2012-08-19 19:58:21 UTC
Git done (by process-git-requests).

Corrected branch name.

Comment 12 Fedora Update System 2012-08-21 09:35:49 UTC
mate-polkit-1.4.0-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/mate-polkit-1.4.0-2.fc16

Comment 13 Fedora Update System 2012-08-21 09:36:00 UTC
mate-polkit-1.4.0-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/mate-polkit-1.4.0-2.fc18

Comment 14 Fedora Update System 2012-08-21 09:36:10 UTC
mate-polkit-1.4.0-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/mate-polkit-1.4.0-2.fc17

Comment 15 Fedora Update System 2012-08-21 17:08:00 UTC
mate-polkit-1.4.0-2.fc18 has been pushed to the Fedora 18 testing repository.

Comment 16 Dan Mashal 2012-08-26 22:41:23 UTC
>leigh scott 2012-08-19 06:12:53 PDT
>There is still an outstanding issue, move this to -devel where it belongs
>%{_datadir}/gir-1.0/PolkitGtkMate-1.0.gir

Done.

Comment 17 Michael S. 2012-08-27 09:21:29 UTC
The review didn't catch some issues :

the same multiple license issues problem as others, I will not repeat what i said in others bug reports :
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

in fact, looking at it, there is no GPL licensed code in the tarball, except ltmain.sh, which is part of the build system and so do not count in distribution.

So the license tag is wrong, and the reviewer should have caught that.


There is also the fact that the desktop file is not checked explicitly :
https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage



The Buildrequires are also likely wrong, since there is no need for cairo there ( that's a polkit agent in gtk2 ), and there is surely a bug somewhere else. Removing it make build fails, and  while adding it is a work around, that's likely not the right fix since nothing in the source code requires cairo, and the real fix should be to report this to the maintainer of the gtk package so it requires what is needed instead.

Comment 18 Rex Dieter 2012-08-27 12:01:13 UTC
Fwiw, desktop file validation is only required for  items appearing in menus (ie, stuff under /usr/share/applications)

I'll look into the other items you raise now.

Comment 19 Rex Dieter 2012-08-27 12:12:35 UTC
Confirmed cairo is indeed needed somewhere for introspection.


%changelog
* Mon Aug 27 2012 Rex Dieter <rdieter> - 1.4.0-4
- drop extraneous update-desktop-database scriptlet
- don't mark .desktop file %%config
- License: LGPLv2+

should address the rest.

Comment 20 Fedora Update System 2012-09-02 00:28:23 UTC
mate-polkit-1.4.0-2.fc17 has been pushed to the Fedora 17 stable repository.

Comment 21 Fedora Update System 2012-09-17 22:25:23 UTC
mate-polkit-1.4.0-2.fc18 has been pushed to the Fedora 18 stable repository.


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