Bug 847604 (libmatekbd) - Review Request: libmatekbd - A keyboard configuration library for MATE Desktop
Summary: Review Request: libmatekbd - A keyboard configuration library for MATE Desktop
Keywords:
Status: CLOSED ERRATA
Alias: libmatekbd
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-13 06:12 UTC by Dan Mashal
Modified: 2012-09-17 22:04 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-09-17 17:33:07 UTC
Type: Bug
Embargoed:
rdieter: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Dan Mashal 2012-08-13 06:12:29 UTC
Spec URL: http://vicodan.fedorapeople.org/matespec/libmatekbd.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/libmatekbd-1.4.0-1.fc17.src.rpm
Description: A keyboard configuration library for MATE Desktop

Comment 1 Wolfgang Ulbrich 2012-08-21 00:38:01 UTC
Some suggestions:
1. Can you move
%{_bindir}/matekbd-indicator-plugins-capplet
%{_datadir}/applications/matekbd-indicator-plugins-capplet.desktop

to an subpackage called capplet?
Because capplet is the name of configuration elements in mate-control-center.
So everyone knows it belongs there.

2. mateconf schemas need a spriplet section.
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf

3. BR gtk+-devel means gtk-1.0, we don't need it. Adding libICE-devel is a good choice, see configure part.

4. Requires:	%{name}-libs%{?_isa} = %{version}-%{release}
under -devel is double call, because it's called under the main package.

5. no need for --add-only-show-in="X-MATE" in dektopfile installation for f18,
because MATE is a valid entry for f18 desktopfile version.
https://bugs.freedesktop.org/show_bug.cgi?id=52493

All done here.
http://raveit65.fedorapeople.org/Mate-Desktop/f19/SPECS/libmatekbd.spec

Comment 2 Wolfgang Ulbrich 2012-08-21 00:44:51 UTC
(In reply to comment #1)
> 5. no need for --add-only-show-in="X-MATE" in dektopfile installation for
> f18,
> because MATE is a valid entry for f18 desktopfile version.
https://bugs.freedesktop.org/show_bug.cgi?id=44353#c2
> https://bugs.freedesktop.org/show_bug.cgi?id=52493

Comment 3 Dan Mashal 2012-08-21 08:53:48 UTC
Will fix tomorrow.

Comment 4 Wolfgang Ulbrich 2012-08-21 12:07:19 UTC
I don't work for the Mate-Desktop project anymore, because i see no basic for working together with Dan Mashal.

Comment 5 Rex Dieter 2012-08-24 12:20:11 UTC
OK, initial review comments.

1.  SHOULD  move
NOCONFIGURE=1 ./autogen.sh
to %prep section

2.  SHOULD drop  -libs subpkg, this one's name already starts with  lib*,  and I see little risk for  multilib conflicts.  (while you're at it, seems  to  have  2 %files sections, remove the dup)

3.  MUST:  like item 2 from comment #2, MUST add mateconf-related scriptlets for
desktop_mate_peripherals_keyboard_xkb.schemas

Comment 6 Dan Mashal 2012-08-26 03:21:19 UTC
Fixed.

Spec URL: http://vicodan.fedorapeople.org/matespec/libmatekbd.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/libmatekbd-1.4.0-2.fc17.src.rpm
Description: A keyboard configuration library for MATE Deskto

Comment 7 Rex Dieter 2012-08-26 18:28:45 UTC
OK, I think you've misused the 
%..._scheme_obsolete

according to:
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf

%gconf_schema_obsolete is used for schemas that this package previously provided but no longer does.

So,
%pre
%mateconf_schema_prepare  .../desktop_mate_peripherals_keyboard_xkb.schemas
%mateconf_schema_obsolete .../desktop_mate_peripherals_keyboard_xkb.schemas

is definitely not right here.

I think the %mateconf_schema_obsolete call should simply be dropped (for now, until when/if we want to investigate if the 3rd-party mate repo has any schemas in need of obsoletion)


While we're at it, 

%mateconf_schema_prepare %{_sysconfdir}/mateconf/schemas/desktop_mate_peripherals_keyboard_xkb.schemas

should just be:
%mateconf_schema_prepare desktop_mate_peripherals_keyboard_xkb

(ie, each scriptlet should use the basename of the schema, not the full path)

Comment 8 Dan Mashal 2012-08-26 22:23:22 UTC
Fixed

Spec URL: http://vicodan.fedorapeople.org/matespec/libmatekbd.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/libmatekbd-1.4.0-3.fc17.src.rpm
Description: A keyboard configuration library for MATE Deskto

Comment 9 Rex Dieter 2012-08-26 23:49:12 UTC
thanks, APPROVED.

Comment 10 Dan Mashal 2012-08-26 23:53:35 UTC
New Package SCM Request
=======================
Package Name: libmatekbd
Short Description: A keyboard configuration library for MATE Desktop
Owners: vicodan rdieter
Branches: f16 f17 f18

Comment 11 Gwyn Ciesla 2012-08-27 01:46:42 UTC
Git done (by process-git-requests).

Comment 12 Fedora Update System 2012-08-27 02:50:11 UTC
libmatekbd-1.4.0-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/libmatekbd-1.4.0-3.fc18

Comment 13 Fedora Update System 2012-08-27 02:50:24 UTC
libmatekbd-1.4.0-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libmatekbd-1.4.0-3.fc17

Comment 14 Michael S. 2012-08-27 08:37:51 UTC
Rex, the review have been rather hasty, you missed some points like :
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

I see no license clarification in the spec file as committed in git. This should be fixed ASAP, given that Fedora try to be as clean as possible from a legal point of view ( and I guess Dan do not want to have Legal involved in the feature ).


%{_datadir}/libmatekbd/ui/show-layout.ui

this directory end being unowned, this is a violation of

https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_wholly_contained_in_your_package.2C_or_involves_core_functionality_of_your_package


- the desktop file modification are likely wrong, since mate is a registered environment :
http://standards.freedesktop.org/menu-spec/latest/apb.html

So if mate do not show "mate" category desktop file, a bug report should be filled, or at least, it should be explained and investigated.

Comment 15 Rex Dieter 2012-08-27 11:45:42 UTC
The .desktop file modification is required for some fedora releases, f18+ is ok afaik.  The rest of the mate stack is using X-Mate currently, thought it best to switch these over all at once instead of piece-meal.

I'll check on the dir ownership and licensing situation now, thanks

Comment 16 Rex Dieter 2012-08-27 11:57:54 UTC
%changelog
* Mon Aug 27 2012 Rex Dieter <rdieter> 1.4.0-4
- License: LGPLv2+, %%doc COPYING.LIB
- dir ownership
- don't use undefined %%{po_package} macro
- s|MATE|X-MATE| only on < f18

should take care of it here.

Comment 17 Rex Dieter 2012-08-27 12:28:21 UTC
icky, looks like we can't fully conditionalize the MATE/X-MATE thing.  so, f18+'s desktop-file-utils accepts MATE as valid ShowIn entry, but not as a valid Category yet.

Comment 18 Michael S. 2012-08-27 13:51:14 UTC
In fact, that's just desktop-file-validate that do not like using Mate category, the code is likely using the same category all over the place. 

If mate was pushed only on f18 as planned ( after all, that's the whole point of doing a fedora 18 feature ), it would not be a issue. 

And I think that using X-MATE will break some applets, I am looking at source code to check, but there is surely a issue there. So far, i only found it being used in the bundle copy of egglib in mate-panel, and not elsewhere. I guess upstream will likely know what is the specific version to us. 

But I doubt it does work with both, and I doubt upstream used a substring match ( since that would be prone so error, and slower ).

Comment 19 Rex Dieter 2012-08-27 14:00:33 UTC
Mate ought to recognize both with and without the X- prefix.  Either way, if something doesn't work, it's a bug worth fixing (and it's not something necessarily a review blocker, imo, unless you're arguing otherwise?)

Comment 20 Rex Dieter 2012-08-27 14:02:35 UTC
And, that glosses over that even f18+ doesn't yet recognize "MATE" as a valid Category (only ShowIn entries currently recognize "MATE")

Comment 21 Michael S. 2012-08-27 14:26:25 UTC
I would treat that as a serious bug, and I would have blocked the review if I was the reviewer, yes. But since the review was approved, I am just raising the issue so this can be fixed in time. 

Since mate desktop is not planned to be complete in F18 ( ie, there is lots of others packages part of mate, see  https://github.com/mate-desktop/, and Dan answered that for example, the screensaver is not part of the feature to have it in F18 ), I am not sure people will really report if a applet is missing until the day of the release, or will just think "this is a work in progress, that's normal if something is not here".

Another fix would be to backport desktop-file-utils so everything will be correct, and make sure the correct value is used right from the start. If we keep a mess, we are just increasing the technical debt, and the source code of mate is already full of stuff that should be cleaned ( like for example migration code that do not work since the word "gnome" have been changed for "mate" in constant, so nothing will be migrated at all , so the code is useless )

Comment 22 Dan Mashal 2012-08-27 14:39:37 UTC
Thanks Michael for your concerns. They will be taken in to consideration. If you have anything else to add in terms of actually fixing things instead of trying to block MATE please let is know.

Thanks,
Dan.

Comment 23 Rex Dieter 2012-08-27 14:43:46 UTC
Dan, his concerns are valid, even if we disagree on their severity or blocker-worthiness.   I know it can be frustrating, but invalidating those concerns with comments like "instead of trying to  block MATE" is inappropriate.  It's ok to *repectfully* disagree.

Comment 24 Michael S. 2012-08-27 15:32:04 UTC
I guess I will for now focus on others features package review, since my help do not seems to be welcome here.

Comment 25 Fedora Update System 2012-08-27 16:31:37 UTC
libmatekbd-1.4.0-3.fc18 has been pushed to the Fedora 18 testing repository.

Comment 26 Fedora Update System 2012-09-17 17:33:07 UTC
libmatekbd-1.4.0-3.fc17 has been pushed to the Fedora 17 stable repository.

Comment 27 Fedora Update System 2012-09-17 22:04:10 UTC
libmatekbd-1.4.0-3.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.