Bug 1889387
Summary: | Review Request: rust-alacritty - GPU-accelerated terminal emulator | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Stefano Figura <stefano> |
Component: | Package Review | Assignee: | Fabio Valentini <decathorpe> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | decathorpe, package-review |
Target Milestone: | --- | Flags: | decathorpe:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
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: | |
Embargoed: |
Description
Stefano Figura
2020-10-19 14:32:13 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 :) 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! |