https://bugzilla.redhat.com/show_bug.cgi?id=1889387
Fabio Valentini <decathorpe(a)gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED
CC| |decathorpe(a)gmail.com
Assignee|nobody(a)fedoraproject.org |decathorpe(a)gmail.com
Doc Type|--- |If docs needed, set a value
Flags| |fedora-review?
--- Comment #1 from Fabio Valentini <decathorpe(a)gmail.com> ---
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...
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-gith...
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component