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 :)
https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags

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 \
    %{buildroot}%{_datadir}/applications/Alacritty.desktop
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/logo/alacritty-term.svg \
    %{buildroot}%{_datadir}/pixmaps/Alacritty.svg
install -p -D -m644 %{crate}-%{version_no_tilde}/alacritty.yml \
    %{buildroot}%{_datadir}/alacritty/alacritty.yml
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/completions/alacritty.bash \
    %{buildroot}%{_datadir}/bash-completion/completions/alacritty
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/completions/_alacritty \
    %{buildroot}%{_datadir}/zsh/site-functions/_alacritty
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/completions/alacritty.fish \
    %{buildroot}%{_datadir}/fish/vendor_completions.d/alacritty.fish
install -p -D -m644 %{crate}-%{version_no_tilde}/extra/alacritty.man \
    %{buildroot}%{_mandir}/man1/alacritty.1

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

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.

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

I have one question:

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

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
%else
%global build_terminfo 1
%endif

and

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

and

%if %{build_terminfo}
%{_datadir}/terminfo/a/alacritty*
%endif

and

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

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.

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?

Package looks good now. All my concerns were addressed.
No fedora-review output because recent glibc change in rawhide borked some things.

APPROVED

Many thanks for the review!

❯❯ fedpkg request-repo rust-alacritty 1889387
https://pagure.io/releng/fedora-scm-requests/issue/29867

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-alacritty

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
%endif

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

Please fix this before importing the package.

Cheers, issues addressed, imported and built!