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
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
(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
Will fix tomorrow.
I don't work for the Mate-Desktop project anymore, because i see no basic for working together with Dan Mashal.
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
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
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)
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
thanks, APPROVED.
New Package SCM Request ======================= Package Name: libmatekbd Short Description: A keyboard configuration library for MATE Desktop Owners: vicodan rdieter Branches: f16 f17 f18
Git done (by process-git-requests).
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
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
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.
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
%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.
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.
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 ).
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?)
And, that glosses over that even f18+ doesn't yet recognize "MATE" as a valid Category (only ShowIn entries currently recognize "MATE")
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 )
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.
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.
I guess I will for now focus on others features package review, since my help do not seems to be welcome here.
libmatekbd-1.4.0-3.fc18 has been pushed to the Fedora 18 testing repository.
libmatekbd-1.4.0-3.fc17 has been pushed to the Fedora 17 stable repository.
libmatekbd-1.4.0-3.fc18 has been pushed to the Fedora 18 stable repository.