This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 847419 (mate-polkit)

Summary: Review request: mate-polkit - Integrates polkit with the MATE Desktop environment
Product: [Fedora] Fedora Reporter: Dan Mashal <dan.mashal>
Component: Package ReviewAssignee: Rex Dieter <rdieter>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: misc, notting, package-review, rdieter
Target Milestone: ---Flags: rdieter: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-09-01 20:28:23 EDT Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 840149    

Description Dan Mashal 2012-08-10 20:58:26 EDT
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-18 20:10:23 EDT
I  can review
Comment 2 Rex Dieter 2012-08-18 20:28:22 EDT
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-18 22:16:02 EDT
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-18 22:22:17 EDT
Fixed that. Thanks Rex.
Comment 6 Dan Mashal 2012-08-18 22:23:09 EDT
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 04:35:25 EDT
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 08:47:27 EDT
For the 2 provides you suggest adding, please explain why you think they are needed.
Comment 9 leigh scott 2012-08-19 09:12:53 EDT
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 09:58:41 EDT
(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 Jon Ciesla 2012-08-19 15:58:21 EDT
Git done (by process-git-requests).

Corrected branch name.
Comment 12 Fedora Update System 2012-08-21 05:35:49 EDT
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 05:36:00 EDT
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 05:36:10 EDT
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 13:08:00 EDT
mate-polkit-1.4.0-2.fc18 has been pushed to the Fedora 18 testing repository.
Comment 16 Dan Mashal 2012-08-26 18:41:23 EDT
>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 Scherer 2012-08-27 05:21:29 EDT
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 08:01:13 EDT
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 08:12:35 EDT
Confirmed cairo is indeed needed somewhere for introspection.


%changelog
* Mon Aug 27 2012 Rex Dieter <rdieter@fedoraproject.org> - 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-01 20:28:23 EDT
mate-polkit-1.4.0-2.fc17 has been pushed to the Fedora 17 stable repository.
Comment 21 Fedora Update System 2012-09-17 18:25:23 EDT
mate-polkit-1.4.0-2.fc18 has been pushed to the Fedora 18 stable repository.