Bug 2034758 - Review Request: apptainer - new name for singularity
Summary: Review Request: apptainer - new name for singularity
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mattia Verga
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-12-22 00:00 UTC by Dave Dykstra
Modified: 2022-03-03 23:24 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-03-03 23:24:16 UTC
Type: ---
Embargoed:
mattia.verga: fedora-review+


Attachments (Terms of Use)

Description Dave Dykstra 2021-12-22 00:00:30 UTC
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.

Comment 1 Dave Dykstra 2022-01-18 23:44:46 UTC
What happens if nobody reviews this?

Comment 2 Dave Dykstra 2022-01-26 22:44:34 UTC
Here's a scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=81552090

Comment 3 Dave Dykstra 2022-01-26 23:29:48 UTC
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

Comment 4 Dave Dykstra 2022-01-26 23:37:04 UTC
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

Comment 5 Mattia Verga 2022-02-01 18:00:05 UTC
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.

Comment 6 Dave Dykstra 2022-02-01 19:43:07 UTC
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.

Comment 7 Dave Dykstra 2022-02-02 16:52:21 UTC
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.

Comment 8 Dave Dykstra 2022-02-02 17:06:07 UTC
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.

Comment 9 Mattia Verga 2022-02-02 17:40:45 UTC
(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.

Comment 10 Mattia Verga 2022-02-02 17:47:31 UTC
(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.

Comment 11 Dave Dykstra 2022-02-02 18:55:36 UTC
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.

Comment 12 Mattia Verga 2022-02-03 17:06:09 UTC
(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.

Comment 13 Dave Dykstra 2022-02-03 17:50:19 UTC
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.

Comment 15 Mattia Verga 2022-02-11 12:06:06 UTC
Dave, it doesn't build on Rawhide:

https://koji.fedoraproject.org/koji/taskinfo?taskID=82677194

Comment 16 Dave Dykstra 2022-02-11 14:42:37 UTC
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.

Comment 17 Mattia Verga 2022-02-12 08:12:24 UTC
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.

Comment 18 Dave Dykstra 2022-02-14 17:52:00 UTC
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

Comment 19 Maxwell G 2022-02-15 03:01:00 UTC
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

Comment 20 Dave Dykstra 2022-02-15 21:33:55 UTC
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.

Comment 21 Mattia Verga 2022-02-16 18:49:52 UTC
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

Comment 22 Maxwell G 2022-02-18 03:31:50 UTC
> 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.

Comment 23 Dave Dykstra 2022-02-19 17:25:14 UTC
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

Comment 24 Maxwell G 2022-02-20 03:38:51 UTC
(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.

Comment 25 Mattia Verga 2022-02-20 10:46:29 UTC
(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.

Comment 26 Mattia Verga 2022-02-20 14:24:52 UTC
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

Comment 27 Dave Dykstra 2022-02-22 01:27:45 UTC
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

Comment 28 Maxwell G 2022-02-22 02:04:57 UTC
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?

Comment 29 Dave Dykstra 2022-02-22 17:03:54 UTC
(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).

Comment 30 Dave Dykstra 2022-02-22 17:16:26 UTC
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.

Comment 31 Mattia Verga 2022-02-23 08:28:00 UTC
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

Comment 33 Dave Dykstra 2022-02-24 21:55:28 UTC
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.

Comment 34 Gwyn Ciesla 2022-02-25 15:27:54 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/apptainer

Comment 35 Mattia Verga 2022-02-25 17:46:51 UTC
(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".

Comment 36 Mattia Verga 2022-02-25 17:47:55 UTC
(I've hit the send button too early)

"BSD-3-Clause-LBNL" is "LBNL BSD" in Fedora: https://fedoraproject.org/wiki/Licensing:Main

Comment 37 Dave Dykstra 2022-03-03 23:24:16 UTC
This has now been built for rawhide, f36, f35, epel9, epel8, and epel7.  Closing ticket.


Note You need to log in before you can comment on or make changes to this bug.