Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1419317 (deepin-terminal)
Summary: | Review Request: deepin-terminal - terminal emulation application | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | sensor.wen |
Component: | Package Review | Assignee: | Zbigniew Jędrzejewski-Szmek <zbyszek> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | i, 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:33 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: | 1419330, 1419332 | ||
Bug Blocks: | 1465889 |
Description
sensor.wen
2017-02-05 11:17:00 UTC
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 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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/ 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_List. 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}. Errr, Requires: %{name}-data = %{version}-%{release} of course. SPEC: https://raw.githubusercontent.com/FZUG/repo/master/rpms/deepin_project/deepin-terminal.spec SRPM: https://copr-be.cloud.fedoraproject.org/results/mosquito/deepin/fedora-25-x86_64/00582191-deepin-terminal/deepin-terminal-2.5.1-2.git82c4a12.fc25.src.rpm Task: https://copr.fedorainfracloud.org/coprs/mosquito/deepin/build/582191/ When do i need to split the package? /usr/share/ is very large? 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. %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. Thank you. @Zbigniew Jędrzejewski-Szmek you are the best. Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/deepin-terminal This is already in Rawhide. Closing on behalf of the Deepin Desktop packaging effort. |