Bug 1421047 (deepin-tool-kit) - Review Request: deepin-tool-kit - Base development tool of all C++/Qt Developer work on Deepin
Summary: Review Request: deepin-tool-kit - Base development tool of all C++/Qt Develop...
Keywords:
Status: CLOSED RAWHIDE
Alias: deepin-tool-kit
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: gsettings-qt dtksettings
Blocks: DeepinDEPackageReview deepin-menu deepin-shortcut-viewer deepin-qml-widgets
TreeView+ depends on / blocked
 
Reported: 2017-02-10 08:54 UTC by sensor.wen
Modified: 2018-01-01 01:45 UTC (History)
5 users (show)

Fixed In Version: deepin-tool-kit-0.3.1-1.fc27
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-08-04 14:28:56 UTC
Type: ---
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)

Comment 1 Zbigniew Jędrzejewski-Szmek 2017-07-19 23:15:10 UTC
+ package name is OK (a bit strange, but that's upstream's choice...)
+ license is acceptable (GPLv3)
+ license is specified correctly
+ BuildRequires/Provides/Requires look correct
+ devel subpackage is split out correctly
+ gcc flags look OK
- scriptlets are missing: https://fedoraproject.org/wiki/Packaging:Scriptlets#Shared_libraries

> Project MESSAGE: This project is using private headers and will therefore be tied to this specific Qt module build version.
> Project MESSAGE: Running this project against other versions of the Qt modules may crash at any arbitrary point.
> Project MESSAGE: This is not a bug, but a result of using Qt internals. You have been warned!

How specific is this requirement on qt version? Should there be a Requires added with a specific version?

Comment 2 Felix Yan 2017-07-21 06:45:32 UTC
It should be rebuilt with every minor Qt version change to avoid potential breakage.

Comment 3 Robin Lee 2017-07-22 14:44:34 UTC
You can add a line:
%{?_qt5:Requires: %{_qt5}%{?_isa} = %{_qt5_version}}

Comment 4 sensor.wen 2017-07-22 16:22:44 UTC
# rpm -qR deepin-tool-kit
libQt5Core.so.5()(64bit)
libQt5Core.so.5(Qt_5)(64bit)
libQt5Core.so.5(Qt_5.7)(64bit)

It already needs Qt 5.7. Of course, rebuild for each minor version, compatibility may be better.

I think use ">=" instead of "=" to avoid too much rebuild, because Fedora's Qt update is not frequent.

(In reply to Robin Lee from comment #3)
> You can add a line:
> %{?_qt5:Requires: %{_qt5}%{?_isa} = %{_qt5_version}}

Comment 5 Zbigniew Jędrzejewski-Szmek 2017-07-22 20:47:21 UTC
I don't know enough about qt for an informed decision. This is a nicety that we can figure out later. Bowen, can you post an updated spec file with the scriptlets? I'll rebuild the package once more, and if nothing pops up, approve it.

Comment 6 Rex Dieter 2017-07-22 22:07:18 UTC
The convention kde-sig recommends if a package uses private headers, is to add at least:

BuildRequires: qt5-qtbase-private-devel

and then one of:
%{?_qt5:Requires: %{_qt5}%{?_isa} >= %{_qt5_version}}

%{?_qt5:Requires: %{_qt5}%{?_isa} = %{_qt5_version}}


depending on your best judgement on how important the tight runtime dependency is (depends largely on *how* the private headers and interfaces are being used).

If you're not sure, go with the safer '=' variant.

Comment 7 sensor.wen 2017-07-23 15:22:10 UTC
SPEC:  https://raw.githubusercontent.com/FZUG/repo/0debd1ed03ecd29c5f01b98d44da1b6916c6555a/rpms/deepin_project/deepin-tool-kit.spec

It's build pass and added the `%{?_qt5:Requires: %{_qt5}%{?_isa} = %{_qt5_version}}` macro.   @Zbigniew Jędrzejewski-Szmek

Comment 8 Zbigniew Jędrzejewski-Szmek 2017-07-24 13:01:48 UTC
Looks all good now. Package is APPROVED.

Comment 9 Gwyn Ciesla 2017-07-30 02:17:52 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/deepin-tool-kit


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