Bug 1421058 (deepin-metacity)

Summary: Review Request: deepin-metacity - 2D window manager for Deepin
Product: [Fedora] Fedora Reporter: sensor.wen
Component: Package ReviewAssignee: Zbigniew Jędrzejewski-Szmek <zbyszek>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: felixonmars, package-review, zbyszek
Target Milestone: ---Flags: zbyszek: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-07-22 13:30:46 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1476560    
Bug Blocks: 1465889, 1421066    

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.