Bug 1583798 (termy-qt) - Review Request: termy-qt - TermySequence terminal multiplexer client
Summary: Review Request: termy-qt - TermySequence terminal multiplexer client
Keywords:
Status: CLOSED ERRATA
Alias: termy-qt
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: termy-server
Blocks: qt-reviews
TreeView+ depends on / blocked
 
Reported: 2018-05-29 17:46 UTC by Eamon Walsh
Modified: 2018-10-02 19:30 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-10-01 01:08:37 UTC
Type: ---
Embargoed:
rdieter: fedora-review+


Attachments (Terms of Use)

Description Eamon Walsh 2018-05-29 17:46:53 UTC
Spec URL: https://termysequence.io/fedora/termy-qt.spec
SRPM URL: https://termysequence.io/fedora/termy-qt-1.0.2-1.fc28.src.rpm

Description: A Qt-based multiplexing terminal emulator client
implementing the TermySequence protocol. TermySequence is a terminal emulation system with a focus on connectivity, productivity, and collaboration.

The termy-server package should go before this one (see #1582983)

Available in COPR:
https://copr.fedorainfracloud.org/coprs/ewalsh/termysequence/

Koji scratch builds:
f28 https://koji.fedoraproject.org/koji/taskinfo?taskID=27283817
f29 https://koji.fedoraproject.org/koji/taskinfo?taskID=27284228

Fedora Account System Username: ewalsh

I am the upstream author and maintainer

Comment 1 Artur Frenszek-Iwicki 2018-05-29 22:02:22 UTC
The stuff inside /usr/share/qtermy is 6.2 MiB. It should go in termy-qt-data or some other noarch subpackage, IMHO.

Comment 2 Eamon Walsh 2018-05-31 17:25:38 UTC
Sounds reasonable. Will plan on doing this pending further review.

Comment 3 Eamon Walsh 2018-06-22 17:25:23 UTC
The libv8 package in Fedora is now deprecated [1]. To accommodate this, I'm working on bundling libv8 upstream and doing a static link instead. Because of this, the size of the termy-qt executable will increase. There will be a net savings in space over the current package+libv8. However, the executable is going to be larger than all of the data in /usr/share/qtermy combined, so I don't believe that a subpackage will be necessary.

[1] https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/DI4Q5526MVI5KS7OG4PH37QFK6KCDAY2/

Comment 4 Rex Dieter 2018-07-02 04:03:04 UTC
I'd prefer if:
Source:  https://termysequence.io/releases/termysequence-%{version}.tar.xz
Source1: https://termysequence.io/releases/termy-icon-theme-%{icons_version}.tar.xz
Source2: https://termysequence.io/releases/termy-emoji-%{emoji_version}.tar.xz

These 3 items were packaged separately.  I assume you're bundling them all here for convenience?

Comment 5 Eamon Walsh 2018-08-10 20:40:44 UTC
Okay, a new 1.1.0 release has been made upstream. Upstream is now distributing termy-qt as a separate tarball. Upstream is also no longer distributing termy-emoji and termy-icon-theme tarballs. These are now bundled into termy-qt. The V8 library is also now bundled as described in comment #3.

Spec URL: https://termysequence.io/fedora/termy-qt.spec
SRPM URL: https://termysequence.io/fedora/termy-qt-1.1.0-1.fc28.src.rpm

Available in COPR:
https://copr.fedorainfracloud.org/coprs/ewalsh/termysequence/

Comment 6 Rex Dieter 2018-09-18 16:13:51 UTC
$ rpmlint *.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

naming: ok

licensing: ok
but consider adding a comment near the License: tag as reference to which licenses apply to which parts of the software

sources: ok
$ md5sum *.xz
ac7581529766edb8f8c89f4515c94a15  termysequence-qt-1.1.0.tar.xz

1.  similar to termy-sever review, SHOULD remove
# Build type "None" disables Release/Debug CFLAGS and LDFLAGS set by CMake.
# Only the CFLAGS and LDFLAGS specified by rpmbuild will be used.
and consider using
-DCMAKE_BUILD_TYPE=Release
instead.

scriptlets: n/a

macros: ok

2.  since this bundles a copy of v8, MUST add something like:
Provides: bundled(v8) = <version>
per 
https://fedoraproject.org/wiki/Bundled_Libraries?rd=Packaging:Bundled_Libraries#Requirement_if_you_bundle

Comment 7 Eamon Walsh 2018-09-21 13:39:17 UTC
I have made all of the changes described in comment #6 and corrected the License which was missing BSD for v8. I have also updated to the upstream 1.1.1 release.

Spec URL: https://termysequence.io/fedora/termy-qt.spec
SRPM URL: https://termysequence.io/fedora/termy-qt-1.1.1-1.fc28.src.rpm

Available in COPR:
https://copr.fedorainfracloud.org/coprs/ewalsh/termysequence/

Koji scratch builds:
f28 https://koji.fedoraproject.org/koji/taskinfo?taskID=29790338
f29 https://koji.fedoraproject.org/koji/taskinfo?taskID=29790342
f30 https://koji.fedoraproject.org/koji/taskinfo?taskID=29790346

Comment 8 Rex Dieter 2018-09-21 16:11:59 UTC
Thanks looks good, approved.


Minor things to consider in the future, 

1. avoid using globs like:

desktop-file-validate %{buildroot}%{_datadir}/applications/*.desktop
appstream-util validate-relax --nonet %{buildroot}%{_datadir}/metainfo/*.appdata.xml

and 

%{_datadir}/applications/*.desktop
%{_datadir}/metainfo/*.appdata.xml

and reference the full files precisely (without globs).  These should generally never change, and if they do, you should be mindful of that.

2. consider using
%{_metainfodir}/...
instead of hard-coded
 %{_datadir}/metainfo/...

Comment 9 Gwyn Ciesla 2018-09-22 17:02:01 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/termy-qt

Comment 10 Fedora Update System 2018-09-23 02:38:32 UTC
termy-qt-1.1.1-1.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2018-41064bd1fa

Comment 11 Fedora Update System 2018-09-23 04:47:26 UTC
termy-qt-1.1.1-1.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-e6fc2fe6c7

Comment 12 Fedora Update System 2018-09-23 20:41:21 UTC
termy-qt-1.1.1-1.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-41064bd1fa

Comment 13 Fedora Update System 2018-09-23 21:39:36 UTC
termy-qt-1.1.1-1.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-e6fc2fe6c7

Comment 14 Eamon Walsh 2018-10-01 01:08:37 UTC
I have committed the changes described in comment #8 and they will be present in the next release. Thanks again for the sponsorship and the review. Feel free to give me a review or two to work on in return.

Comment 15 Fedora Update System 2018-10-02 16:01:37 UTC
termy-qt-1.1.1-1.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2018-10-02 19:30:49 UTC
termy-qt-1.1.1-1.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.


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