Bug 1889387

Summary: Review Request: rust-alacritty - GPU-accelerated terminal emulator
Product: [Fedora] Fedora Reporter: Stefano Figura <stefano>
Component: Package ReviewAssignee: Fabio Valentini <decathorpe>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: decathorpe, package-review
Target Milestone: ---Flags: decathorpe: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2020-10-21 15:15:52 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Stefano Figura 2020-10-19 14:32:13 UTC
Spec URL: https://returntrip.fedorapeople.org/packages/rust-alacritty.spec

SRPM URL: https://returntrip.fedorapeople.org/packages/rust-alacritty-0.5.0-1.fc34.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=53762759

Description: GPU-accelerated terminal emulator

Fedora Account System Username: returntrip

Comment 1 Fabio Valentini 2020-10-19 15:50:11 UTC
Initial review:

1) The "without_terminfo" macro name is confusing because it mixes terminology with the built-in --with and --without support in RPM.
And you're rebuilding terminfo data on fedora < 33, but on fedora >= 33 it's supported by ncurses? You mean the files are already provided by ncurses on fedora 33+?

2) "Recommends: ncurses-base >= 6.2" has no effect since it's attached to the rust-alacritty *source* package, and there's no such thing as "BuildRecommends".

3) You could use a more descriptive file name for the GitHub tarball. I wrote very nice Guidelines for this :)

GitHub allows you to specify the file name that you want. You don't need to use "vX.Y.Z.tar.gz" anymore, e.g. instead of
Source1: https://github.com/alacritty/alacritty/archive/v%{version}.tar.gz
you could use
Source1: https://github.com/alacritty/alacritty/archive/v%{version}/alacritty-github-sources-%{version}.tar.gz

4) The indentation for the BuildRequires (lines 36-40) is inconsistent, I recommend to align them at column 17 (after four four-space tab-equivalents).

5) You need to calculate the "effective" license of the alacritty binary package (including all dependencies, since it's statically linked). The "easiest" way to do that is to run a mock build with "--without check", and then query all "rust-*-devel" packages in the buildroot for their "%{LICENSE}" tag, sort the results, and calcluate the license with "SPDX math".

6) Why to you manually install the alacritty binary? It should get installed by %cargo_install.

7) Your "install" lines in the %install scriptlet are very long. I recommend to break them onto two lines:

install -p -D -m644 %{crate}-%{version_no_tilde}/extra/linux/Alacritty.desktop \
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/logo/alacritty-term.svg \
install -p -D -m644 %{crate}-%{version_no_tilde}/alacritty.yml \
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/completions/alacritty.bash \
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/completions/_alacritty \
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/completions/alacritty.fish \
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/alacritty.man \

%if !%{without_terminfo}
tic -xe alacritty,alacritty-direct %{crate}-%{version_no_tilde}/extra/alacritty.info \
    -o %{buildroot}%{_datadir}/terminfo

This way, things are automatically aligned and the line lengths stay almost within 80 characters.

8) Some directories where you put files are not owned by any package:

     /usr/share/fish/vendor_completions.d, /usr/share/zsh/site-functions,
     /usr/share/alacritty, /usr/share/fish, /usr/share/zsh

Alacritty should at least own /usr/share/alacritty, but it should probably co-own /usr/share/fish/ and /usr/share/zsh, unless you want to add a hard dependendy ("Requires: zsh, fish") on those two shells from the alacritty binary package.

Comment 2 Stefano Figura 2020-10-20 14:00:13 UTC
Thanks for the review but especially for all the advice! Thanks to your guidance I have learned so much in the last couple of days.

I have updated the spec file and the source RPM. Hopefully I have addressed all the points above.

New koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=53835349

Comment 3 Fabio Valentini 2020-10-20 22:23:04 UTC
I have one question:

%if !%{build_terminfo}
tic -xe alacritty,alacritty-direct %{crate}-%{version_no_tilde}/extra/alacritty.info \
    -o %{buildroot}%{_datadir}/terminfo

This means that "if not build terminfo then build terminfo" ...
So I guess your 0 / 1 definitions in the .spec preamble are the wrong way round.

I think things should be:

%if 0%{?fedora} >= 33
%global build_terminfo 0
%global build_terminfo 1


%if !%{build_terminfo}
BuildRequires:  ncurses >= 6.2
Recommends:     ncurses-base >= 6.2


%if %{build_terminfo}


%if %{build_terminfo}
tic -xe alacritty,alacritty-direct %{crate}-%{version_no_tilde}/extra/alacritty.info \
    -o %{buildroot}%{_datadir}/terminfo

respectively, to make sense with the name of the macro. The way you did it now, the macro should probably be "do_not_build_terminfo" :)

You can fix that at import. I'm running fedora-review again now.

Comment 4 Fabio Valentini 2020-10-20 22:56:58 UTC
Okay, fedora-review is busted, because somebody broke rawhide.

Last question: why only "Recommends: ncurses-base >= 6.2" and not "Requires"?
Does the ncurses-base package only provide optional functionality for alacritty?

Comment 5 Fabio Valentini 2020-10-21 13:58:11 UTC
Package looks good now. All my concerns were addressed.
No fedora-review output because recent glibc change in rawhide borked some things.


Comment 6 Stefano Figura 2020-10-21 14:01:12 UTC
Many thanks for the review!

❯❯ fedpkg request-repo rust-alacritty 1889387

Comment 7 Gwyn Ciesla 2020-10-21 14:30:34 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-alacritty

Comment 8 Fabio Valentini 2020-10-21 14:33:11 UTC
Oh, I see one issue.

You have updated the License tag for the *source package* with the effective license instead of applying it to the "alacritty" *binary package*.

The source package has only the alacritty license (ASL 2.0), but there must be a separate License tag with the effective license for the binary package:

package     -n %{crate}
Summary:        %{summary}
License:        ASL 2.0 and BSD and CC0 and ISC and MIT and WTFPL and zlib

%if !%{build_terminfo}
BuildRequires:  ncurses >= 6.2

%description -n %{crate} %{_description}

Please fix this before importing the package.

Comment 9 Stefano Figura 2020-10-21 15:15:52 UTC
Cheers, issues addressed, imported and built!