https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Bug ID: 1419317 Summary: Review Request: deepin-terminal - terminal emulation application Product: Fedora Version: rawhide Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: sensor.wen@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://github.com/FZUG/repo/blob/master/rpms/deepin_project/deepin-terminal...
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/mosquito/deepin/fedora-25-x8...
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=17596833
Description: Default terminal emulation application for Deepin
Fedora Account System Username: mosquito
Snapshot: https://files.gitter.im/1dot75cm/h9Ah/____20170121203618.png
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
sensor.wen@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Zamir SUN sztsian@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1465889 | |(DeepinDEPackageReview)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1465889 [Bug 1465889] Tracking: Deepin Desktop related package review tracker
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Felix Yan felixonmars@archlinux.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |1419332
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1419332 [Bug 1419332] Review Request: deepin-shortcut-viewer - deepin shortcut viewer
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Felix Yan felixonmars@archlinux.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |1419330
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1419330 [Bug 1419330] Review Request: deepin-menu - deepin menu service
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |zbyszek@in.waw.pl Assignee|nobody@fedoraproject.org |zbyszek@in.waw.pl
--- Comment #1 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- The usual: - please link to the raw spec file for fedora-review's sake - underscores on macros are not necessary
It would be great to add an appdata file [https://fedoraproject.org/wiki/Packaging:AppData]. This will make it easier to discover and install deepin-terminal for users of other desktop environments. I'm assuming it's independently usable.
It doesn't build in rawhide and F26 currently: /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./lib/animation.vala:33.9-33.43: warning: the modifier `static' is not applicable to constants public static const int FAST = 250; /* Good for animations that convey duplicated information */ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./lib/animation.vala:34.9-34.46: warning: the modifier `static' is not applicable to constants public static const int INSTANT = 150; /* Good for animations that don't convey any information */ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./lib/animation.vala:35.9-35.45: warning: the modifier `static' is not applicable to constants public static const int NORMAL = 500; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./lib/animation.vala:36.9-36.44: warning: the modifier `static' is not applicable to constants public static const int SLOW = 1000; /* Good for animations that convey information that is only presented in the animation */ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ gee-0.8.vapi:278.3-278.31: error: overriding method `Gee.HashSet.foreach' is incompatible with base method `Gee.AbstractCollection.foreach': incompatible type of parameter 1. public override bool @foreach (Gee.ForallFunc f); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ gee-0.8.vapi:278.3-278.31: error: Gee.HashSet.foreach: no suitable method found to override public override bool @foreach (Gee.ForallFunc f); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ gee-0.8.vapi:368.3-368.31: error: overriding method `Gee.PriorityQueue.foreach' is incompatible with base method `Gee.AbstractCollection.foreach': incompatible type of parameter 1. public override bool @foreach (Gee.ForallFunc f); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ gee-0.8.vapi:368.3-368.31: error: Gee.PriorityQueue.foreach: no suitable method found to override public override bool @foreach (Gee.ForallFunc f);
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:43.27-43.54: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:43.56-43.79: warning: Gdk.Screen.get_active_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:45.13-45.39: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:293.27-293.54: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:293.56-293.79: warning: Gdk.Screen.get_active_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:295.13-295.39: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:177.34-177.61: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:177.63-177.86: warning: Gdk.Screen.get_active_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:178.34-178.61: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:181.13-181.39: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/window.vala:56.27-56.54: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/window.vala:56.56-56.79: warning: Gdk.Screen.get_active_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/window.vala:58.13-58.39: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/config_window.vala:486.43-486.70: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/config_window.vala:486.72-486.95: warning: Gdk.Screen.get_active_window has been deprecated since 3.22 /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/config_window.vala:488.29-488.55: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
--- Comment #2 from sensor.wen@gmail.com --- Hi Zbigniew Jędrzejewski-Szmek,
Thank your for you help. I searched this error in the github. The error seems to be related to vala. `rm vapi/gee-0.8.vapi` should fix it on fc26.
Some Requires may not be needed when using other desktop environments. This requires further testing in the virtual machine.
``` # right-click menu style Requires: deepin-menu # run command by create_from_commandline Requires: deepin-shortcut-viewer Requires: xdg-utils Recommends: deepin-manual ```
Build test: https://copr.fedorainfracloud.org/coprs/mosquito/deepin/build/580989/
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review?
--- Comment #3 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- For reviewer convenience, whenever you make another version of package under review, please add the "Spec:" and "SRPM:" lines again, so it's easy to go to the latest sources.
rpmlint:
deepin-terminal.src: W: invalid-license GPL3
The list of allowed license is at https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Software_License_.... In this case, it should be "GPLv3".
deepin-terminal.x86_64: E: incorrect-locale-subdir /usr/share/locale/es_419/LC_MESSAGES/deepin-terminal.mo deepin-terminal.x86_64: E: invalid-lc-messages-dir /usr/share/locale/es_419/LC_MESSAGES/deepin-terminal.mo
It's probably harmless, but indeed seems strange.
deepin-terminal.x86_64: E: invalid-desktopfile /usr/share/applications/deepin-terminal.desktop file contains group "NewWindow Shortcut Group", but groups extending the format should start with "X-" deepin-terminal.x86_64: E: invalid-desktopfile /usr/share/applications/deepin-terminal.desktop file contains group "Quake Shortcut Group", but groups extending the format should start with "X-"
Is this a Deepin thing? If so, you can ignore this warning.
3 packages and 0 specfiles checked; 5 errors, 8 warnings.
There's a bunch of other warnings, but I think they are all harmless.
It looks almost OK. We can revisit the appdata question later.
The package has a ~600kB binary, and ~5MB of data. Please split out the data into a noarch subpackage. Everything under /usr/share can go that that subpackage. And then the main package should have Requires = %{name}-data = %{version}-%{release}.
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
--- Comment #4 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- Errr, Requires: %{name}-data = %{version}-%{release} of course.
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841 (FE-NEEDSPONSOR) |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
--- Comment #5 from sensor.wen@gmail.com --- SPEC: https://raw.githubusercontent.com/FZUG/repo/master/rpms/deepin_project/deepi... SRPM: https://copr-be.cloud.fedoraproject.org/results/mosquito/deepin/fedora-25-x8... Task: https://copr.fedorainfracloud.org/coprs/mosquito/deepin/build/582191/
When do i need to split the package? /usr/share/ is very large?
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
--- Comment #6 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- There's no hard rule. For documentation, it's suggested to split it out if it's more than a megabyte or so. For shared data, if it's a few megabytes... It mostly saves some space on mirrors. Another minor advantage is that most likely the data doesn't change much, so drpm will save most of the download.
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #7 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- %description should end in ".".
You don't need %doc README.md and %license LICENSE in the main subpackage, because it's already included in %{name}-data anyway, and %{name}-data is required by the main subpackage.
+ package name is OK + license is acceptable for Fedora (GPLv3) + license is specified correctly + builds and installs OK + Provides/Requires/BuildRequires appear to be correct + scriptlets are OK (at least I think so, there's many different upgrade/downgrade/install/uninstall paths).
OK, I think it's all good, apart from some cosmetic issues. Package is APPROVED.
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
--- Comment #8 from sensor.wen@gmail.com --- Thank you. @Zbigniew Jędrzejewski-Szmek you are the best.
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
--- Comment #9 from Gwyn Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/deepin-terminal
https://bugzilla.redhat.com/show_bug.cgi?id=1419317 Bug 1419317 depends on bug 1419332, which changed state.
Bug 1419332 Summary: Review Request: deepin-shortcut-viewer - deepin shortcut viewer https://bugzilla.redhat.com/show_bug.cgi?id=1419332
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Zamir SUN sztsian@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Alias| |deepin-terminal
https://bugzilla.redhat.com/show_bug.cgi?id=1419317
Zamir SUN sztsian@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE Last Closed| |2018-07-22 09:30:33
--- Comment #10 from Zamir SUN sztsian@gmail.com --- This is already in Rawhide. Closing on behalf of the Deepin Desktop packaging effort.
https://bugzilla.redhat.com/show_bug.cgi?id=1419317 Bug 1419317 depends on bug 1419330, which changed state.
Bug 1419330 Summary: Review Request: deepin-menu - deepin menu service https://bugzilla.redhat.com/show_bug.cgi?id=1419330
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com