https://bugzilla.redhat.com/show_bug.cgi?id=1889387 Fabio Valentini <decathorpe@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |decathorpe@xxxxxxxxx Assignee|nobody@xxxxxxxxxxxxxxxxx |decathorpe@xxxxxxxxx Doc Type|--- |If docs needed, set a value Flags| |fedora-review? --- Comment #1 from Fabio Valentini <decathorpe@xxxxxxxxx> --- 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. -- 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 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
