Spec URL: https://github.com/apptainer/apptainer/blob/main/dist/rpm/apptainer.spec.in SRPM URL: https://drive.google.com/file/d/1WtKqINsukl4kRzgO_tJBQZzAZroLLjPk/view Description: Application container platform Fedora Account System Username: dwd This is the new package name for singularity, after the project joined the Linux Foundation as announced at https://apptainer.org/news/community-announcement-20211130/ It is planned to maintain the command "singularity" as part of the package for compatibility, so the old package name should remain open to keep the name reserved.
What happens if nobody reviews this?
Here's a scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=81552090
I'm sorry that the google drive link doesn't work with fedora-review. Try instead: Spec URL: https://github.com/apptainer/apptainer/blob/main/dist/rpm/apptainer.spec.in SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/2208/81552208/apptainer-1.0.0~rc.1-1.el7.src.rpm Description: Application container platform Fedora Account System Username: dwd
Sorry, that fixed the SRPM URL but not the the Spec URL. Try again: Spec URL: https://github.com/apptainer/apptainer/releases/download/v1.0.0-rc.1/apptainer.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/2208/81552208/apptainer-1.0.0~rc.1-1.el7.src.rpm
Sorry for the late reply, I didn't see you email. Before posting a full review from fedora-review, I have some notices to do: - License: BSD-3-Clause-LBNL I see there's a "LICENSE-APACHE-2.0" which refers to code contained in one file. I think this license too should be listed. Also, I see a "COPYRIGHT.md" file which says "All rights reserved" (the same spec file contains such lines)... the "All rights reserved" is not acceptable. - Provides: singularity Obsoletes: singularity < 3.9 Conflicts: singularity Please list proper Provides/Obsoletes https://docs.fedoraproject.org/en-US/packaging-guidelines/#renaming-or-replacing-existing-packages The "Conflicts" tag is not required, in fact it is discouraged: https://docs.fedoraproject.org/en-US/packaging-guidelines/Conflicts/ - Source: %{name}-%{package_version}.tar.gz Source1: go1.16.12.src.tar.gz Where are these sources come from? Can't you provide a full URL for sources? Also, and that's my greatest doubt after the license, if I'm correct you're building the package using a bundled Go toolchain? I will ask on the Fedora mailing list if it is possible at all.
Mattia, I'll address those comments. Regarding the go tool chain, yes, that was my only choice for building singularity-3.8.5 a couple of months ago, because a golang security vulnerability required using at least 1.16.12 and epel7 was still stuck on go 1.15. It so happens that in the meanwhile I have become the provider for go on epel7 and I installed version 1.16.12 last weekend, so this could now be removed.
It's not clear to me what version to include with Provides =, since the apptainer version number has been re-set to 1.0.0. The packaging guidelines say it should usually be a macro that increases with new versions of the newly renamed package, but that would be difficult to do since you can't do math on a version number, for example to add 3.0.0 to whatever the current version is. I guess I will just set it to a constant 4.0.0-1, because that's what the number would have been if it hadn't been reset to 1.0.0.
Regarding the "All rights reserved" -- those are inherited and I'm not sure it's legal to remove them. The Apptainer project is not using the phrase itself. Where is the documentation on not including "All rights reserved"? Google of "fedora" "all rights reserved" only brings me to web pages where Fedora has used the phrase itself.
(In reply to Dave Dykstra from comment #7) > It's not clear to me what version to include with Provides =, since the > apptainer version number has been re-set to 1.0.0. The packaging guidelines > say it should usually be a macro that increases with new versions of the > newly renamed package, but that would be difficult to do since you can't do > math on a version number, for example to add 3.0.0 to whatever the current > version is. I guess I will just set it to a constant 4.0.0-1, because > that's what the number would have been if it hadn't been reset to 1.0.0. mmm that's an uncommon situation. In my opinion, you can/should use an Epoch bump like: Provides: singularity = 1:%{version}-%{release} Obsoletes: singularity <= 3.8.5-4 From the packaging guidelines: >If a package is being renamed without any functional changes, or is a compatible enough replacement to an existing package (where "enough" means that it includes only changes of magnitude that are commonly found in version upgrade changes), provide clean upgrade paths and compatibility with: > >Provides: oldpackagename = $provEVR >Obsoletes: oldpackagename < $obsEVR > >$provEVR refers to an (Epoch-)Version-Release tuple the original unchanged package would have had if it had been version or release bumped.
(In reply to Dave Dykstra from comment #8) > Regarding the "All rights reserved" -- those are inherited and I'm not sure > it's legal to remove them. The Apptainer project is not using the phrase > itself. Where is the documentation on not including "All rights reserved"? > Google of "fedora" "all rights reserved" only brings me to web pages where > Fedora has used the phrase itself. There has been a discussion on legal mailing list some day ago: https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/F62LSAKODK6D2UROUKUOFNZXPVHL4RLQ/ If I understang English right, it was decided that when there is "all rights reserved" + the license text, it is assumed that the rights granted by the license text are still granted... but I'm not 100% sure, I'd better ask this one too on that thread.
It looks to me like it is saying that using "All rights reserved" is not recommended, but if it's there, it can be ignored.
(In reply to Dave Dykstra from comment #11) > It looks to me like it is saying that using "All rights reserved" is not > recommended, but if it's there, it can be ignored. Yep, you can ignore my comments about that. So the points to fix are the Conflicts/Provides and the source URLs. It would be a plus if you can use Fedora Golang macros in every section.
I don't think I can depend on Fedora Golang macros, because they have not been backported to epel7. There they are a part of the golang package, which I maintain, so I could backport some of them if there was a call for it. Currently there are only two macros, %gopath and %go_arches.
Mattia, The updated package is now ready for further review: Spec URL: https://github.com/apptainer/apptainer/releases/download/v1.0.0-rc.2/apptainer.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/1635/82611635/apptainer-1.0.0~rc.2-1.el7.src.rpm A scratch build is at https://koji.fedoraproject.org/koji/taskinfo?taskID=82611539
Dave, it doesn't build on Rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=82677194
Oh, yes, that was something I noticed after the release when building the corresponding singularity new release for rawhide. It has been fixed in https://github.com/apptainer/apptainer/pull/245 and will get in the next apptainer pre-release. Please go ahead with other parts of the review to see what else needs to change.
Can you please backport the patch into the provided files, so that I can run fedora-review? Otherwise it will fail and doesn't produce any useful output. I think you have fixed all my points reported before, just some minor tweaks/cleanup: - macros in comments should have '%%' not just '%' - not sure if it's needed for Suse build, but ExclusiveOS is not something I have ever seen in Fedora - same for ExcludeArch ppc64, Fedora doesn't build for ppc64 anymore, so it should be no more needed - empty changelog? Everything else looks fine.
Ok I made a 1.0.0~rc.2-2 spec & src rpm including PR 245 and the additional changes you requested which are now in PR 251. Spec URL: https://github.com/DrDaveD/apptainer/releases/download/v1.0.0-rc.2-2/apptainer.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/7795/82817795/apptainer-1.0.0~rc.2-2.fc37.src.rpm Scratch build is at https://koji.fedoraproject.org/koji/taskinfo?taskID=82817793
I have some other comments about the specfile. Please let me know if you have any questions. - > License: BSD-3-Clause-LBNL and LICENSE-APACHE-2.0 You need to use Fedora license identifiers[1] here. This should be `License: BSD and ASL 2.0`. [1]: https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses - You should remove the SUSE conditionals, as per https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_legibility. - You should use `%autosetup -n %{name}-%{package_version} -p1` in `%prep` to prepare the archive. Remove all of the text that I quoted below and put the `./mconfig` line right below `%build. You can get rid of the rest of the `cd` commands and the manual patching; `%autosetup` will take care of this for you. ``` %if "%{?buildroot}" export RPM_BUILD_ROOT="%{buildroot}" %endif if [ -d %{name}-%{version} ]; then # Clean up old build root # First clean go's modcache because directories are unwritable GOPATH=$PWD/%{name}-%{version}/gopath go clean -modcache rm -rf %{name}-%{version} fi # Create our build root mkdir %{name}-%{version} %build cd %{name}-%{version} # Setup an empty GOPATH for the build export GOPATH=$PWD/gopath mkdir -p "$GOPATH" # Extract the source tar -xf "%SOURCE0" cd %{name}-%{package_version} patch -p1 <%PATCH0 %if "%{?SOURCE1}" != "" GOVERSION="$(echo %SOURCE1|sed 's,.*/,,;s/go//;s/\.src.*//')" if ! ./mlocal/scripts/check-min-go-version go $GOVERSION; then # build the go tool chain, the existing version is too old pushd .. tar -xf %SOURCE1 cd go/src ./make.bash cd ../.. export PATH=$PWD/go/bin:$PATH popd fi %endif ``` - > make -C builddir old_config= You should use the `%make_build` macro. - > make -C builddir DESTDIR=$RPM_BUILD_ROOT install You should use the `%make_install` macro. - You should install the docs using `%doc` and the licenses using `%license` in the `%files` list. For example: ``` # Define `%license` tag if not already defined. This is needed for EL 7 compatibility. %{!?_licensedir:%global license %doc} %files ... %doc CHANGELOG.md README.md %license LICENSE.md LICENSE-LBNL.md LICENSE-APACHE-2.0 NOTICE-APACHE-2.0 ``` - `%define package_version 1.0.0-rc.2` should use `%global` instead of `%define`.[2] [2]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_global_preferred_over_define
Thanks for the review, Maxwell. I have made an updated version: Spec URL: https://github.com/DrDaveD/apptainer/releases/download/v1.0.0-rc.2-3/apptainer.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/3275/82863275/apptainer-1.0.0~rc.2-3.fc37.src.rpm Koji build https://koji.fedoraproject.org/koji/taskinfo?taskID=82863274 The standard %prep macros weren't being used because of historical reasons which are no longer applicable, so that's a very good improvement. I'm not happy about having to remove SUSE macros because that means I can't directly use the upstream package's own .spec file. I think that's a bad rule, but I took it out anyway. I didn't want to remove the if-ed out code referencing %SOURCE because it might be needed someday if golang again lags behind a needed version. I don't think there's a rule against it.
Here it is the full review output. An automated issue is detected about packaging not using hardened build. I think this is a false positive, since nothing is undefining _hardened_build, but I'll check in the next days. I want to check especially if `%undefine _debugsource_packages` is causing this. In fact, I don't see -PIC or -PIE in the applied build flags. The license check lists many other licenses detected, but I think are all related to the "vendor" code bundled... maybe it's worth double checking this also. And check if a license breakdown in comments is feasible. There are errors in the %files definition: ``` %dir %{_sysconfdir}/%{name} %config(noreplace) %{_sysconfdir}/%{name}/*.conf %config(noreplace) %{_sysconfdir}/%{name}/*.toml %config(noreplace) %{_sysconfdir}/%{name}/*.json %config(noreplace) %{_sysconfdir}/%{name}/*.yaml %config(noreplace) %{_sysconfdir}/%{name}/global-pgp-public %config(noreplace) %{_sysconfdir}/%{name}/cgroups/* %config(noreplace) %{_sysconfdir}/%{name}/network/* %config(noreplace) %{_sysconfdir}/%{name}/seccomp-profiles/* ``` I think could be ``` %dir %{_sysconfdir}/%{name} %config(noreplace) %{_sysconfdir}/%{name}/* ``` so that it owns the subdirectories also. About ``` %dir %{_sysconfdir}/bash_completion.d ``` is wrong, it is owned by `filesystem`, so it doesn't need to be owned by apptainer. In the SHOULD section, the patch is missing an URL or a comment that explains what it is about. The source should point to an URL or have a comment why it doesn't (downstream modified sources?) Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Package uses hardened build flags if required to. Note: suid files: starter-suid and not %global _hardened_build See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_compiler_flags ===== 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]: 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. [?]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "*No copyright* Lawrence Berkeley National Labs BSD variant license", "*No copyright* Apache License 2.0", "BSD 3-Clause License", "Apache License 2.0", "Lawrence Berkeley National Labs BSD variant license", "ISC License", "Lawrence Berkeley National Labs BSD variant license BSD 3-Clause License BSD 2-Clause License", "MIT License", "*No copyright* BSD 3-Clause License", "*No copyright* MIT License", "*No copyright* Mozilla Public License 2.0", "Creative Commons Attribution 4.0 Apache License 2.0", "MIT License BSD 3-Clause License", "*No copyright* [generated file]", "BSD 3-Clause License Apache License 2.0", "MIT License Apache License 2.0", "BSD 2-Clause License", "GNU Lesser General Public License, Version 3 Apache License 2.0", "*No copyright* BSD 2-Clause License", "*No copyright* The Unlicense". 3917 files have unknown license. Detailed output of licensecheck in /home/rpmbuild/reviews/2034758-apptainer/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [!]: Package requires other packages for directories it uses. Note: No known owner of /etc/apptainer/network, /etc/apptainer/seccomp-profiles, /usr/libexec/apptainer/bin, /etc/apptainer/cgroups, /usr/libexec/apptainer/cni [!]: Package must own all directories that it creates. Note: Directories without known owners: /etc/apptainer/network, /etc/apptainer/seccomp-profiles, /usr/libexec/apptainer/bin, /etc/apptainer/cgroups, /usr/libexec/apptainer/cni [!]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /etc/bash_completion.d(freeipa-client, python3-manilaclient, filesystem, lttng-tools, pdc-client, phoronix-test-suite, python3-novaclient, clusterssh, singularity, perl-App-Cme, openscap- scanner, python3-heatclient, iprutils, stgit, git-extras, weldr- client, python3-glanceclient, quilt, RBTools, perl-Dist-Zilla) [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. [-]: Package contains desktop file if it is a GUI application. [-]: 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. [?]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [x]: 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. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 2 files. [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 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]: 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]: Package contains systemd file(s) if in need. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: 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). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. [!]: 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]: Fully versioned dependency in subpackages if applicable. [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: There are rpmlint messages (see attachment). [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]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Cannot parse rpmlint output: Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output: Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Requires -------- apptainer (rpmlib, GLIBC filtered): /bin/sh /usr/bin/sh config(apptainer) libc.so.6()(64bit) libseccomp.so.2()(64bit) rtld(GNU_HASH) squashfs-tools apptainer-debuginfo (rpmlib, GLIBC filtered): Provides -------- apptainer: apptainer apptainer(x86-64) config(apptainer) singularity singularity-runtime apptainer-debuginfo: apptainer-debuginfo apptainer-debuginfo(x86-64) debuginfo(build-id) Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -b 2034758 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, C/C++, Shell-api Disabled plugins: Ocaml, Perl, Haskell, Java, R, SugarActivity, PHP, Python, fonts Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
> An automated issue is detected about packaging not using hardened build. I think this is a false positive, since nothing is undefining _hardened_build, but I'll check in the next days. I want to check especially if `%undefine _debugsource_packages` is causing this. In fact, I don't see -PIC or -PIE in the applied build flags. Try adding `%set_build_flags` to the beginning of `%build` and see if that fixes it.
But the documentation https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/buildflags.md says that %set_build_flags is automatically called before the %build step, so how would that help? Anyway, I prepared another update attempting to address everything else. It so happened that we had just added a collection of the licenses from the bundled code, so that turned out to be easy. This time instead of including patch files I made a new pseudo-release of the source tarball. Spec URL: https://github.com/DrDaveD/apptainer/releases/download/v1.0.0-rc.2.1/apptainer.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/6126/83026126/apptainer-1.0.0~rc.2.1-1.fc37.src.rpm Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=83026068
(In reply to Dave Dykstra from comment #23) > But the documentation > https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/ > buildflags.md says that %set_build_flags is automatically called before the > %build step, so how would that help? That is only valid for F36+ and above, so you should still add it.
(In reply to Maxwell G from comment #24) > (In reply to Dave Dykstra from comment #23) > > But the documentation > > https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/ > > buildflags.md says that %set_build_flags is automatically called before the > > %build step, so how would that help? > > That is only valid for F36+ and above, so you should still add it. I don't think that would make any difference, since %set_build_flags just exports CFLAGS and other compiler flags without -fPIE... I'm not sure where the hardened builds flags are really injected.
I've spent some time today looking at the reported issue about hardening flags and I definitely think this is a false positive from the times when _hardened_build was not by default. Analyzing the built executables with `hardening-check` script shows they're being built with -fPIC. By explicitly adding `%global _hardened_build 1` into the specfile nothing changes from the report of hardening-check, while fedora-review doesn't list the issue anymore. Therefore, I think fedora-review expects an explicit `%global _hardened_build 1` into the specfile when any produced executable uses suid, not considering that now hardened_build is the default. That said, there are still a couple of things to tweak before finally approve the package: ``` %dir %{_libexecdir}/%{name} %{_libexecdir}/%{name}/bin/starter %{_libexecdir}/%{name}/cni/* ``` should be changed to just ``` %{_libexecdir}/%{name} ``` to fix issue about directories ownership. Finally, if you're going to use sources from a git snapshot and not from a release tarball, you'll have to follow https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots Otherwise the source URL is reported as unreachable. Also look at the version-release reported in the changelog, which doesn't correspond to the Version-Release tags (the Release tag is still set to 1, too). Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Package uses hardened build flags if required to. Note: suid files: starter-suid and not %global _hardened_build See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_compiler_flags ===== 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]: 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", "*No copyright* Lawrence Berkeley National Labs BSD variant license", "BSD 3-Clause License", "*No copyright* Apache License 2.0", "Apache License 2.0", "ISC License", "*No copyright* The Unlicense", "Lawrence Berkeley National Labs BSD variant license BSD 3-Clause License BSD 2-Clause License Apache License 2.0", "MIT License", "*No copyright* BSD 3-Clause License", "*No copyright* MIT License", "*No copyright* Mozilla Public License 2.0", "Creative Commons Attribution 4.0 Apache License 2.0", "MIT License BSD 3-Clause License", "*No copyright* [generated file]", "BSD 3-Clause License Apache License 2.0", "MIT License Apache License 2.0", "BSD 2-Clause License", "GNU Lesser General Public License, Version 3 Apache License 2.0", "*No copyright* BSD 2-Clause License". 3850 files have unknown license. Detailed output of licensecheck in /home/rpmbuild/reviews/2034758-apptainer/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: Package requires other packages for directories it uses. Note: No known owner of /usr/libexec/apptainer/bin, /usr/libexec/apptainer/cni [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/libexec/apptainer/bin, /usr/libexec/apptainer/cni [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [!]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: 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. [x]: 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. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 30720 bytes in 2 files. [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 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]: 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]: Package contains systemd file(s) if in need. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [!]: Sources can be downloaded from URI in Source: tag Note: Could not download Source0: https://github.com/apptainer/apptainer/releases/download/v1.0.0-rc.2.1/apptainer-1.0.0-rc.2.1.tar.gz See: https://docs.fedoraproject.org/en-US/packaging- guidelines/SourceURL/ [-]: 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). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [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]: Fully versioned dependency in subpackages if applicable. [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: There are rpmlint messages (see attachment). [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]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Cannot parse rpmlint output: Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output: Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Requires -------- apptainer (rpmlib, GLIBC filtered): /bin/sh /usr/bin/sh config(apptainer) libc.so.6()(64bit) libseccomp.so.2()(64bit) rtld(GNU_HASH) squashfs-tools apptainer-debuginfo (rpmlib, GLIBC filtered): Provides -------- apptainer: apptainer apptainer(x86-64) config(apptainer) singularity singularity-runtime apptainer-debuginfo: apptainer-debuginfo apptainer-debuginfo(x86-64) debuginfo(build-id) Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -b 2034758 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, C/C++, Shell-api Disabled plugins: R, Python, SugarActivity, Haskell, Java, PHP, Perl, fonts, Ocaml Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
The problem with the source URL wasn't that it needed a snapshot version but that I neglected to change the upstream URL to my personal org where I had the tag. It should work this time. I also updated the libexecdir and fixed the changelog entry. Spec URL: https://github.com/DrDaveD/apptainer/releases/download/v1.0.0-rc.2.2/apptainer.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/7453/83147453/apptainer-1.0.0~rc.2.2-1.fc37.src.rpm Koji scratch build https://koji.fedoraproject.org/koji/taskinfo?taskID=83147451
Thank you for all your work on this! I have a couple more minor comments. > %undefine _debugsource_packages Is this still necessary? > %{_sysconfdir}/bash_completion.d/* Bash completions should be installed in `%{_datadir}/bash-completion/completions`. > %posttrans > # clean out empty directories under /etc/singularity > rmdir %{_sysconfdir}/singularity/* %{_sysconfdir}/singularity 2>/dev/null || true This looks strange to me. What is it actually needed for?
(In reply to Maxwell G from comment #28) > > %undefine _debugsource_packages > > Is this still necessary? Yes. Without it, the error shown in the comment appears. Here's a scratch build with that: https://kojipkgs.fedoraproject.org//work/tasks/9892/83179892/build.log > > %{_sysconfdir}/bash_completion.d/* > > Bash completions should be installed in > `%{_datadir}/bash-completion/completions`. Oh, I hadn't heard about that other directory. Ok I'll update that after hearing from Mattia if there's anything else. > > %posttrans > > # clean out empty directories under /etc/singularity > > rmdir %{_sysconfdir}/singularity/* %{_sysconfdir}/singularity 2>/dev/null || true > > This looks strange to me. What is it actually needed for? It's because the previous package didn't clean up everything properly on deletion. This cleanup is needed so apptainer can figure out whether or not the system administrator has fully converted the configuration file to the new name: it warns if /etc/singularity still exists (more precisely, if "$(basename %{_sysconfdir})/singularity" still exists).
Actually that "more precisely" was wrong. I was trying to say "$(basename /etc/apptainer)/singularity" or whatever the configuration directory turns out to be inside the go program.
My comment in the previous review was wrong, now ``` %{_libexecdir}/%{name} ``` needs to be changed to ``` %dir %{_libexecdir}/%{name} %dir %{_libexecdir}/%{name}/bin %{_libexecdir}/%{name}/bin/starter %{_libexecdir}/%{name}/cni ``` to avoid starter-suid from being listed twice, while declaring all directories ownership. And bash completion should be installed in `%{_datadir}/bash-completion/completions` as reported by Maxwell. **Please make those two changes before importing the package**, I'm going to set the review APPROVED already. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Package does not contain duplicates in %files. Note: warning: File listed twice: /usr/lib/.build- id/2c/68e818e228ff80a99a7fda49c0a35a5cd3b048 See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_duplicate_files - Package uses hardened build flags if required to. Note: suid files: starter-suid and not %global _hardened_build See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_compiler_flags ===== 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]: 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", "*No copyright* Lawrence Berkeley National Labs BSD variant license", "BSD 3-Clause License", "*No copyright* Apache License 2.0", "Apache License 2.0", "ISC License", "*No copyright* The Unlicense", "Lawrence Berkeley National Labs BSD variant license BSD 3-Clause License BSD 2-Clause License Apache License 2.0", "MIT License", "*No copyright* BSD 3-Clause License", "*No copyright* MIT License", "*No copyright* Mozilla Public License 2.0", "Creative Commons Attribution 4.0 Apache License 2.0", "MIT License BSD 3-Clause License", "*No copyright* [generated file]", "BSD 3-Clause License Apache License 2.0", "MIT License Apache License 2.0", "BSD 2-Clause License", "GNU Lesser General Public License, Version 3 Apache License 2.0", "*No copyright* BSD 2-Clause License". 3851 files have unknown license. Detailed output of licensecheck in /home/rpmbuild/reviews/2034758-apptainer/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [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. [-]: Package contains desktop file if it is a GUI application. [-]: 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. [x]: 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. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 30720 bytes in 2 files. [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]: Dist tag is present. [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]: Package contains systemd file(s) if in need. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: 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). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: 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]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [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: There are rpmlint messages (see attachment). [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]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Cannot parse rpmlint output: Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output: Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://github.com/DrDaveD/apptainer/releases/download/v1.0.0-rc.2.2/apptainer-1.0.0-rc.2.2.tar.gz : CHECKSUM(SHA256) this package : fba03b8f4123ee0751a49b609e6c6ab9d69c39bc35d00977af8ceed61218d8ed CHECKSUM(SHA256) upstream package : fba03b8f4123ee0751a49b609e6c6ab9d69c39bc35d00977af8ceed61218d8ed Requires -------- apptainer (rpmlib, GLIBC filtered): /bin/sh /usr/bin/sh config(apptainer) libc.so.6()(64bit) libseccomp.so.2()(64bit) rtld(GNU_HASH) squashfs-tools apptainer-debuginfo (rpmlib, GLIBC filtered): Provides -------- apptainer: apptainer apptainer(x86-64) config(apptainer) singularity singularity-runtime apptainer-debuginfo: apptainer-debuginfo apptainer-debuginfo(x86-64) debuginfo(build-id) Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -b 2034758 Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Shell-api, Generic Disabled plugins: fonts, Haskell, SugarActivity, PHP, R, Java, Ocaml, Python, Perl Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Ok, thank you. Hopefully this one is final. Spec URL: https://github.com/DrDaveD/apptainer/releases/download/v1.0.0-rc.2.3/apptainer.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/9572/83249572/apptainer-1.0.0~rc.2.3-1.fc37.src.rpm Koji scratch build https://koji.fedoraproject.org/koji/taskinfo?taskID=83249571
Now a question based on the project review of this: instead of "BSD" can we use "LBNL BSD"? It is on the list of recognized Fedora licenses. The suggestion of "BSD" in place of "BSD-3-Clause-LBNL" came from Maxwell but I think "LBNL BSD" is more accurate.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/apptainer
(In reply to Dave Dykstra from comment #33) > Now a question based on the project review of this: instead of "BSD" can we > use "LBNL BSD"? It is on the list of recognized Fedora licenses. The > suggestion of "BSD" in place of "BSD-3-Clause-LBNL" came from Maxwell but I > think "LBNL BSD" is more accurate. I never understood the licensing tag. I once was told that sometimes a license can "inherit" from a more restrictive license, but in my packages I usually specify all licenses used in source files chaining them with an "AND".
(I've hit the send button too early) "BSD-3-Clause-LBNL" is "LBNL BSD" in Fedora: https://fedoraproject.org/wiki/Licensing:Main
This has now been built for rawhide, f36, f35, epel9, epel8, and epel7. Closing ticket.