Bug 847604 (libmatekbd)

Summary: Review Request: libmatekbd - A keyboard configuration library for MATE Desktop
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   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-09-17 13:33:07 EDT Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 840149    

Description Dan Mashal 2012-08-13 02:12:29 EDT
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-20 20:38:01 EDT
Some suggestions:
1. Can you move

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.

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.

All done here.
Comment 2 Wolfgang Ulbrich 2012-08-20 20:44:51 EDT
(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=52493
Comment 3 Dan Mashal 2012-08-21 04:53:48 EDT
Will fix tomorrow.
Comment 4 Wolfgang Ulbrich 2012-08-21 08:07:19 EDT
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 08:20:11 EDT
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
Comment 6 Dan Mashal 2012-08-25 23:21:19 EDT

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 14:28:45 EDT
OK, I think you've misused the 

according to:

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

%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 18:23:22 EDT

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 19:49:12 EDT
thanks, APPROVED.
Comment 10 Dan Mashal 2012-08-26 19:53:35 EDT
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 Jon Ciesla 2012-08-26 21:46:42 EDT
Git done (by process-git-requests).
Comment 12 Fedora Update System 2012-08-26 22:50:11 EDT
libmatekbd-1.4.0-3.fc18 has been submitted as an update for Fedora 18.
Comment 13 Fedora Update System 2012-08-26 22:50:24 EDT
libmatekbd-1.4.0-3.fc17 has been submitted as an update for Fedora 17.
Comment 14 Michael Scherer 2012-08-27 04:37:51 EDT
Rex, the review have been rather hasty, you missed some points like :

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 ).


this directory end being unowned, this is a violation of


- the desktop file modification are likely wrong, since mate is a registered environment :

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 07:45:42 EDT
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 07:57:54 EDT
* Mon Aug 27 2012 Rex Dieter <rdieter@fedoraproject.org> 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 08:28:21 EDT
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 Scherer 2012-08-27 09:51:14 EDT
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 10:00:33 EDT
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 10:02:35 EDT
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 Scherer 2012-08-27 10:26:25 EDT
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 10:39:37 EDT
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.

Comment 23 Rex Dieter 2012-08-27 10:43:46 EDT
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 Scherer 2012-08-27 11:32:04 EDT
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 12:31:37 EDT
libmatekbd-1.4.0-3.fc18 has been pushed to the Fedora 18 testing repository.
Comment 26 Fedora Update System 2012-09-17 13:33:07 EDT
libmatekbd-1.4.0-3.fc17 has been pushed to the Fedora 17 stable repository.
Comment 27 Fedora Update System 2012-09-17 18:04:10 EDT
libmatekbd-1.4.0-3.fc18 has been pushed to the Fedora 18 stable repository.