Bug 1421058 (deepin-metacity) - Review Request: deepin-metacity - 2D window manager for Deepin
Summary: Review Request: deepin-metacity - 2D window manager for Deepin
Keywords:
Status: CLOSED RAWHIDE
Alias: deepin-metacity
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: deepin-desktop-schemas
Blocks: DeepinDEPackageReview deepin-wm-switcher
TreeView+ depends on / blocked
 
Reported: 2017-02-10 09:14 UTC by sensor.wen
Modified: 2018-07-22 13:30 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2018-07-22 13:30:46 UTC
Type: ---
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)

Comment 1 Zbigniew Jędrzejewski-Szmek 2017-08-04 16:45:33 UTC
License should be GPLv2+.

find %{buildroot} -name '*.la' -exec rm -f {} ';'
→ find %{buildroot} -name '*.la' -delete

rpmlint gives a bunch of warnings like:
deepin-metacity.x86_64: W: file-not-in-%lang /usr/share/locale/am/LC_MESSAGES/deepin-metacity.mo
deepin-metacity.x86_64: W: file-not-in-%lang /usr/share/locale/ar/LC_MESSAGES/deepin-metacity.mo
It seems you need to call %find_lang. See https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files.

deepin-metacity.x86_64: W: incoherent-version-in-changelog 3.22.10-1.gite9af397 ['3.22.10-1.fc27', '3.22.10-1']

I didn't test it, but it seems to be packaged OK, modulo the nits above.

Comment 3 Zamir SUN 2017-08-05 12:50:47 UTC
(In reply to sensor.wen from comment #2)
> SPEC: 
> https://copr-be.cloud.fedoraproject.org/results/mosquito/deepin/fedora-25-
> x86_64/00587015-deepin-metacity/deepin-metacity.spec
> SRPM: 
> https://copr-be.cloud.fedoraproject.org/results/mosquito/deepin/fedora-25-
> x86_64/00587015-deepin-metacity/deepin-metacity-3.22.10-1.fc25.src.rpm
> 
> Thanks. I fix it.

Please remember to use corresponding beginning of the line like "Spec URL". It will make review much easier.

Comment 4 Zbigniew Jędrzejewski-Szmek 2017-08-05 22:27:52 UTC
Issues:
=======
- Package does not install properly:
  "nothing provides deepin-desktop-schemas needed by deepin-metacity-3.22.10-1.fc27.x86_64"

That's actually correct, but we don't have that package yet in rawhide.
I marked that review as blocking this one.

- glib-compile-schemas is run in %postun and %posttrans if package has
  *.gschema.xml files.
  Note: gschema file(s) in deepin-metacity
  See:
  http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#GSettings_Schema

- Package installs a %{name}.desktop using desktop-file-install or desktop-
  file-validate if there is such a file.
[https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage]

- Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/GConf,
     /usr/share/GConf/gsettings, /usr/share/gnome-control-
     center/keybindings, /usr/share/gnome/wm-properties, /usr/share/gnome-
     control-center

Add to %files
%dir /usr/share/GConf
%dir /usr/share/GConf/gsettings
and
Requires: control-center-filesystem

Comment 6 Zbigniew Jędrzejewski-Szmek 2017-08-07 01:45:19 UTC
Oops, sorry. My comment about glib-compile-schemas was wrong. I trusted fedora-review without checking the guidelines, but /usr/bin/glib-compile-schemas should not be called since F24 (#1409315). So please remove those calls again.

Looks good otherwise, I'll re-review when #1476560 is done.

Comment 7 sensor.wen 2017-08-07 07:16:58 UTC
Diff:  https://github.com/FZUG/repo/commit/23c189b5fed5a5899dc0150a00e581722795a2ef

:) Thanks, it's not your not fault.

Comment 8 sensor.wen 2017-08-07 07:17:54 UTC
Diff:  https://github.com/FZUG/repo/commit/23c189b5fed5a5899dc0150a00e581722795a2ef

:) Thanks, it's not your fault.

Comment 9 sensor.wen 2017-10-08 17:40:48 UTC
#1476560 is done. Could you review it? @Zbigniew Jędrzejewski-Szmek

Comment 10 Zbigniew Jędrzejewski-Szmek 2017-10-08 19:14:27 UTC
I'm looking at https://github.com/FZUG/repo/blob/e7d45acd1874a9bb0934721f6d2163688e19714e/rpms/deepin_project/deepin-metacity.spec.

Hmm, fails to build here:
+ cd deepin-metacity-3.22.10
+ desktop-file-validate /builddir/build/BUILDROOT/deepin-metacity-3.22.10-1.fc28.x86_64/usr/share/applications/deepin-metacity.desktop /builddir/build/BUILDROOT/deepin-metacity-3.22.10-1.fc28.x86_64/usr/share/gnome/wm-properties/deepin-metacity-wm.desktop
/builddir/build/BUILDROOT/deepin-metacity-3.22.10-1.fc28.x86_64/usr/share/gnome/wm-properties/deepin-metacity-wm.desktop: error: file contains group "Window Manager", but groups extending the format should start with "X-"

??

--

BTW, I fell out out the loop with the deepin reviews. If there's something other ticket I should look at, I'd appreciate a reminder.

Comment 11 sensor.wen 2017-10-09 16:14:50 UTC
Thank you for your work. I fixed.  

https://koji.fedoraproject.org/koji/taskinfo?taskID=22354205

Currently, only two packages needs review (deepin-wm deepin-wm-switcher). If your have free time, maybe review them.

Comment 12 Zbigniew Jędrzejewski-Szmek 2017-10-10 09:22:42 UTC
+ package name is OK
+ license is acceptable for Fedora (GPLv2+)
+ license is specified correctly
+ latest version
+ builds and installs OK
+ fedora-review finds no issues (except obsolete comment about scriptlets for schemas which have been replaced by transfiletriggers)
+ BR/ Requires / Provides look correct
+ scriptlets are sane

Package is APPROVED.

Comment 13 Gwyn Ciesla 2017-10-10 13:27:51 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/deepin-metacity

Comment 14 Zamir SUN 2018-07-22 13:30:46 UTC
This is already in Rawhide. Closing on behalf of the Deepin Desktop packaging effort.


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