https://bugzilla.redhat.com/show_bug.cgi?id=2035576
Bug ID: 2035576 Summary: Review Request: angband - Text-based roguelike RPG game Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: dherrera@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://download.copr.fedorainfracloud.org/results/dherrera/angband/fedora-3... SRPM URL: https://download.copr.fedorainfracloud.org/results/dherrera/angband/fedora-3... Description: A roguelike game where you explore a very deep dungeon, kill monsters, try to equip yourself with the best weapons and armor you can find, and finally face Morgoth - "The Dark Enemy". Fedora Account System Username: dherrera Blocks: FE-NEEDSPONSOR
This updates the work from wart https://bugzilla.redhat.com/show_bug.cgi?id=1739290 Depends on freetype 2.11.1, which is already updated on rawhide. This is my first package, so I need a sponsor.
https://bugzilla.redhat.com/show_bug.cgi?id=2035576
Carl George 🤠 carl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |carl@redhat.com Flags| |fedora-review? Doc Type|--- |If docs needed, set a value CC| |carl@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=2035576
--- Comment #1 from Carl George 🤠 carl@redhat.com --- Good work on this so far. Here are the things I've noticed so far that should be corrected.
================================================================================
git doesn't appear to be needed to build. I removed it and a test build works fine. I understand it's needed by the generate-tarball.sh script, but that isn't run during the build.
-BuildRequires: autoconf automake git +BuildRequires: autoconf automake
================================================================================
There are a few explicit requires that should be removed because they are already covered by the automatic library soname requirements (libSDL2-2.0.so.0, libSDL2_image-2.0.so.0, etc).
-Requires: SDL2 SDL2_image SDL2_ttf SDL2_mixer ncurses hicolor-icon-theme +Requires: hicolor-icon-theme
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_require...
================================================================================
There is a requirement for shadow-utils for %pre scriptlets, but there are no %pre scriptlets. This line should be removed.
-Requires(pre): shadow-utils
================================================================================
You can simplify the %prep section by using %autosetup.
-%setup -q -%patch0 -p1 -%patch1 -p1 -%patch2 -p1 +%autosetup -p1
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_autosetup
================================================================================
The spec file uses make, but there is no buildrequirement on make. It works right now because other buildrequirements pull in make as a dependency, but if that ever changes then the build will start failing. Best to specify it explicitly.
+BuildRequires: make
https://docs.fedoraproject.org/en-US/packaging-guidelines/#buildrequires
================================================================================
It's recommended to use the %make_build macro.
-make %{?_smp_mflags} +%make_build
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_parallel_make
================================================================================
Currently angband-data doesn't require angband, which could result in that package being installed without the necessary copying.rst file. angband-data should include the license file also or require angband.
+Requires: %{name} = %{version}-%{release}
https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline...
================================================================================
It appears that the only CC-BY licensed files are in /usr/share/angband/tiles/gervais/. Based on that, the top level license should be just GPLv2, and the -data subpackage should have a license of "GPLv2 and CC-BY".
(top level) -License: GPLv2 and CC-BY +License: GPLv2
(data subpackage) +License: GPLv2 and CC-BY
https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline...
================================================================================
There are 14 documentation files in /usr/share/doc/angband. You should consider creating a -doc subpackage to own them (not mandatory).
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
================================================================================
There are several rpmlint errors about the permissions structure.
angband.x86_64: E: setgid-binary /usr/bin/angband games 2755 angband.x86_64: E: non-standard-executable-perm /usr/bin/angband 2755 angband.x86_64: E: non-standard-dir-perm /var/games/angband 775 angband.x86_64: E: non-standard-dir-perm /var/games/angband/archive 2775 angband.x86_64: E: non-standard-dir-perm /var/games/angband/save 2775 angband.x86_64: E: non-standard-dir-perm /var/games/angband/scores 2775
The upstream default is to store savefiles and scorefiles in each users' home directory.
https://github.com/angband/angband/blob/4.2.3/configure.ac#L50-L54
Removing the --with-setgid flag will switch to this method, and then all the /var/games directories can be removed.
================================================================================
There is an rpmlint warning about a non-utf8 doc file. This can be fixed by running iconv on that file in %prep.
angband.x86_64: W: file-not-utf8 /usr/share/doc/angband/version.rst
https://fedoraproject.org/wiki/Common_Rpmlint_issues#file-not-utf8
https://bugzilla.redhat.com/show_bug.cgi?id=2035576
--- Comment #2 from Diego Herrera dherrera@redhat.com --- Spec URL: https://download.copr.fedorainfracloud.org/results/dherrera/angband/fedora-r... SRPM URL: https://download.copr.fedorainfracloud.org/results/dherrera/angband/fedora-r...
Thank you for the hard work! I applied all of the corrections and suggestions with the exception of the the permission structure. The purpose of the --with-setgid flag and the permission structure is so that game scores can be shared between different users on the same machine. This is implemented as described in the Fedora Games Packaging guidelines [1] and as in other roguelike games already packaged in Fedora [2][3].
[1] https://fedoraproject.org/wiki/SIGs/Games/Packaging [2] bug 165361 [3] bug 1676999
https://bugzilla.redhat.com/show_bug.cgi?id=2035576
Carl George 🤠 carl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |POST Flags|fedora-review? |fedora-review+
--- Comment #3 from Carl George 🤠 carl@redhat.com --- The justification for the permission structure makes sense. I didn't realize other games in Fedora already used that, or that there were games packaging guidelines in the wiki, separate from the rest of the packaging guidelines.
Package is approved.
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
===== MUST items =====
C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs.
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "FSF Unlimited License (with Retention) [generated file]", "GNU General Public License v2.0 or later [generated file]", "NTP License [generated file]", "GNU General Public License", "BSD 3-clause "New" or "Revised" License", "GNU General Public License v2.0 or later [obsolete FSF postal address (Mass Ave), obsolete FSF postal address (Mass Ave)]". 624 files have unknown license. Detailed output of licensecheck in /home/carl/packaging/reviews/angband/2035576-angband/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: %config files are marked noreplace or the reason is justified. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package contains desktop file if it is a GUI application. [x]: Package installs a %{name}.desktop using desktop-file-install or desktop-file-validate if there is such a file. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: No %config files under /usr. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [x]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [-]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in angband- data [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: SourceX tarball generation or download is documented. Note: Package contains tarball without URL, check comments [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Package should not use obsolete m4 macros [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: angband-4.2.3-4.fc36.x86_64.rpm angband-data-4.2.3-4.fc36.noarch.rpm angband-doc-4.2.3-4.fc36.x86_64.rpm angband-debuginfo-4.2.3-4.fc36.x86_64.rpm angband-debugsource-4.2.3-4.fc36.x86_64.rpm angband-4.2.3-4.fc36.src.rpm angband.x86_64: W: spelling-error Summary(en_US) roguelike -> rogue like, rogue-like, roguery angband.x86_64: W: spelling-error %description -l en_US roguelike -> rogue like, rogue-like, roguery angband.x86_64: E: no-binary angband.x86_64: E: setgid-binary /usr/bin/angband games 2755 angband.x86_64: E: non-standard-executable-perm /usr/bin/angband 2755 angband.x86_64: E: non-standard-dir-perm /var/games/angband 775 angband.x86_64: E: non-standard-dir-perm /var/games/angband/archive 2775 angband.x86_64: E: non-standard-dir-perm /var/games/angband/save 2775 angband.x86_64: E: non-standard-dir-perm /var/games/angband/scores 2775 angband.x86_64: W: desktopfile-without-binary /usr/share/applications/angband.desktop sh angband-data.noarch: W: no-documentation angband.src: W: spelling-error Summary(en_US) roguelike -> rogue like, rogue-like, roguery angband.src: W: spelling-error %description -l en_US roguelike -> rogue like, rogue-like, roguery angband.src: W: strange-permission generate-tarball.sh 775 angband.src: W: invalid-url Source0: angband-4.2.3-norestricted.tar.gz 6 packages and 0 specfiles checked; 7 errors, 8 warnings.
https://bugzilla.redhat.com/show_bug.cgi?id=2035576
--- Comment #4 from Carl George 🤠 carl@redhat.com --- I've sponsored you into the packagers group as well. Let me know if you have any questions about the next steps to request the dist-git repo and build the package.
https://bugzilla.redhat.com/show_bug.cgi?id=2035576
--- Comment #5 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/angband
https://bugzilla.redhat.com/show_bug.cgi?id=2035576
Carl George 🤠 carl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version| |angband-4.2.3-4.fc36 Resolution|--- |RAWHIDE Status|POST |CLOSED Last Closed| |2022-01-26 06:11:21
--- Comment #6 from Carl George 🤠 carl@redhat.com --- https://bodhi.fedoraproject.org/updates/FEDORA-2022-c540a0b61e
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com