Bug 2325933

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8255132
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2325933-kryoptic/fedora-rawhide-x86_64/08255132-kryoptic/fedora-review/review.txt

Found issues:

- Unversioned so-files directly in %_libdir.
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages
- Not a valid SPDX expression 'GPL-3.0'.
  Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

> - Unversioned so-files directly in %_libdir.
>    Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages

This is intended -- the pkcs11 modules are not for linking, but for dlopen-ing and have fixed ABI.

> - Not a valid SPDX expression 'GPL-3.0'.
    Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1

Fixed to GPL-3.0-or-later

Updated the links:

Spec URL: https://jjelen.fedorapeople.org/kryoptic.spec
SRPM URL: https://jjelen.fedorapeople.org/kryoptic-0.1.0%5e20241113.git14a7c8f9-1.fc42.src.rpm

Created attachment 2057673 [details]
The .spec file difference from Copr build 8255132 to 8257407

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8257407
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2325933-kryoptic/fedora-rawhide-x86_64/08257407-kryoptic/fedora-review/review.txt

Found issues:

- Unversioned so-files directly in %_libdir.
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Updated to current version

copr: https://copr.fedorainfracloud.org/coprs/jjelen/kryoptic/build/8414994/

Spec URL: https://jjelen.fedorapeople.org/kryoptic.spec
SRPM URL: https://jjelen.fedorapeople.org/kryoptic-0.1.0%5e20241219.git6166f28a-1.fc42.src.rpm

Created attachment 2063304 [details]
The .spec file difference from Copr build 8257407 to 8422625

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8422625
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2325933-kryoptic/fedora-rawhide-x86_64/08422625-kryoptic/fedora-review/review.txt

Found issues:

- Not a valid SPDX expression 'GPL-3.0'.
  Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Uploaded wrong spec/srpm files. Second try:

Spec URL: https://jjelen.fedorapeople.org/kryoptic.spec
SRPM URL: https://jjelen.fedorapeople.org/kryoptic-0.1.0%5e20241219.git6166f28a-1.fc42.src.rpm

Created attachment 2063328 [details]
The .spec file difference from Copr build 8422625 to 8426087

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8426087
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2325933-kryoptic/fedora-rawhide-x86_64/08426087-kryoptic/fedora-review/review.txt

Found issues:

- Unversioned so-files directly in %_libdir.
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Spec URL: https://jjelen.fedorapeople.org/kryoptic.spec
SRPM URL: https://jjelen.fedorapeople.org/kryoptic-0.1.0%5e20250225.gitb37f7ee1-1.fc43.src.rpm

Copr build:

https://copr.fedorainfracloud.org/coprs/jjelen/kryoptic/build/8698149/

Created attachment 2077751 [details]
The .spec file difference from Copr build 8426087 to 8698150

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8698150
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2325933-kryoptic/fedora-rawhide-x86_64/08698150-kryoptic/fedora-review/review.txt

Found issues:

- Unversioned so-files directly in %_libdir.
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

> - Unversioned so-files directly in %_libdir.
>   Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages

The PKCS#11 modules are not versioned and do not come in devel sub-package:

https://docs.fedoraproject.org/en-US/packaging-guidelines/Pkcs11Support/

Package was generated with rust2rpm, simplifying the review.

✅ package contains only permissible content
✅ package builds and installs without errors on rawhide
✅ test suite is run and all unit tests pass
✅ latest version of the crate is packaged
✅ license matches upstream specification and is acceptable for Fedora
✅ license file is included with %license in %files
✅ package complies with Rust Packaging Guidelines


Two midly annoying reports from rpmlint:
kryoptic.x86_64: E: unused-direct-shlib-dependency /usr/lib64/libkryoptic_pkcs11.so /lib64/libm.so.6
kryoptic.x86_64: W: no-soname /usr/lib64/libkryoptic_pkcs11.so


Package APPROVED.

===

Recommended post-import rust-sig tasks:

- set up package on release-monitoring.org:
  project: $crate
  homepage: https://crates.io/crates/$crate
  backend: crates.io
  version scheme: semantic
  version (*NOT* pre-release) filter: alpha;beta;rc;pre
  distro: Fedora
  Package: rust-$crate

- add @rust-sig with "commit" access as package co-maintainer
  (should happen automatically)

- set bugzilla assignee overrides to @rust-sig (optional)

- track package in koschei for all built branches
  (should happen automatically once rust-sig is co-maintainer)

The Pagure repository was created at https://src.fedoraproject.org/rpms/kryoptic

Thank you!

Repo request:
https://pagure.io/releng/fedora-scm-requests/issue/72680

Fedora 42 branch request:
https://pagure.io/releng/fedora-scm-requests/issue/72681

release-monitoring:
https://release-monitoring.org/project/376956/
(will likely have to adjust after the final release)

Built the pre-release now:
https://koji.fedoraproject.org/koji/taskinfo?taskID=129607188


We should get a final release soon.

There's several things that are strange and / or wrong in the spec file:

1. The SourceLicense is GPL-3.0-or-later, but this license doesn't show up in the License tag for the built package. That shouldn't be possible.

2. You're using the wrong GitHub archive (i.e. zip), the correct ones (.tar.gz) are documented here:
https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_commit_revision
i.e. it should be https://github.com/latchset/kryoptic/archive/%{revision}/kryoptic-%{revision}.tar.gz

2. What is "BuildRequires:  openssl-devel" for? This should not be necessary if this uses the Rust OpenSSL bindings.

3. I'm not sure if this actually works:

> CONFDIR=%{_sysconfdir} %cargo_build -f dynamic,nssdb,standard

since %cargo_build uses a subshell.

I would recommend to do this instead:

> export CONFDIR=%{_sysconfdir}
> %cargo_build -f dynamic,nssdb,standard

4. You can also just skip %cargo_install macro and the "rm .../conformance" calls entirely - building and installing the executable just to delete it again is pointless.

5. The unversioned .so in %{_libdir} is not a minor annoyance, it's wrong. Even the pkcs11 docs say to install it under %{_libdir}/pkcs11/.

> install -Dp target/rpm/libkryoptic_pkcs11.so $RPM_BUILD_ROOT/%{_libdir}/libkryoptic_pkcs11.so
> %{_libdir}/libkryoptic_pkcs11.so

Should be

> install -Dp target/rpm/libkryoptic_pkcs11.so $RPM_BUILD_ROOT/%{_libdir}/pkcs11/libkryoptic_pkcs11.so
> %{_libdir}/pkcs11/libkryoptic_pkcs11.so

see https://docs.fedoraproject.org/en-US/packaging-guidelines/Pkcs11Support/#_registering_the_modules_system_wide

Side note for point 4) in the previous comment:
If you skip %cargo_install, you can drop setting "%global cargo_install_lib 0" too, since that only affects %cargo_install.

> 1. The SourceLicense is GPL-3.0-or-later, but this license doesn't show up in the License tag for the built package. That shouldn't be possible.

This is the license of the project itself:

https://github.com/latchset/kryoptic/blob/main/LICENSE.txt

Seems like I did not manage to put it into the License field correctly. In generated LICENSE.dependencies I can see it is correctly. I will update it.

> 2. You're using the wrong GitHub archive (i.e. zip), the correct ones (.tar.gz) are documented here:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_commit_revision
> i.e. it should be https://github.com/latchset/kryoptic/archive/%{revision}/kryoptic-%{revision}.tar.gz

Thanks for the pointer! Will fix with the next update!

> 2. What is "BuildRequires:  openssl-devel" for? This should not be necessary if this uses the Rust OpenSSL bindings.

It does not use the Rust OpenSSL bindings. It is linking to the openssl directly for reasons.

> 3. I'm not sure if this actually works:
> 
> > CONFDIR=%{_sysconfdir} %cargo_build -f dynamic,nssdb,standard
> 
> since %cargo_build uses a subshell.
> 
> I would recommend to do this instead:
> 
> > export CONFDIR=%{_sysconfdir}
> > %cargo_build -f dynamic,nssdb,standard

Thank you! Will change!

> 4. You can also just skip %cargo_install macro and the "rm .../conformance" calls entirely - building and installing the executable just to delete it again is pointless.

We still install the %{_bindir}/softhsm_migrate which should land from this command.

> 5. The unversioned .so in %{_libdir} is not a minor annoyance, it's wrong. Even the pkcs11 docs say to install it under %{_libdir}/pkcs11/.
> 
> > install -Dp target/rpm/libkryoptic_pkcs11.so $RPM_BUILD_ROOT/%{_libdir}/libkryoptic_pkcs11.so
> > %{_libdir}/libkryoptic_pkcs11.so
> 
> Should be
> 
> > install -Dp target/rpm/libkryoptic_pkcs11.so $RPM_BUILD_ROOT/%{_libdir}/pkcs11/libkryoptic_pkcs11.so
> > %{_libdir}/pkcs11/libkryoptic_pkcs11.so
> 
> see https://docs.fedoraproject.org/en-US/packaging-guidelines/Pkcs11Support/#_registering_the_modules_system_wide

Thank you! Will fix.

Updated the spec file in 

https://src.fedoraproject.org/rpms/kryoptic/c/2e0b4c80c9b6bc5eb79420530394f11cc91ccea5

let me know if I missed some of your comments.

Apparently yes :D

You can drop these four lines:

> # prevent library files from being installed
> %global cargo_install_lib 0
> %cargo_install -f dynamic,nssdb,standard
> rm -f $RPM_BUILD_ROOT/%{_bindir}/conformance

If you don't call %cargo_install, the rest is unnecessary.
You're already installing the .so file manually, so %cargo_install does *only* things you need to manually prevent or un-do.
So ... just don't call %cargo_install xD

You also added a new issue into the License tag.

The "GPL-3.0" identifier is not a valid SPDX identifier in Fedora. It is deprecated in SPDX upstream, and should be either "GPL-3.0-or-later" or "GPL-3.0-only". Given that the SourceLicense is GPL-3.0-or-later, I assume it should be the former.

I don't know where you copied the deprecated identifier from, because whereever it came from, it should have matched where GPL-3.0-or-later for SourceLicense came from. So there appears to be either an inconsistency or a copy-paste-error happened.

(In reply to Fabio Valentini from comment #22)
> Apparently yes :D
> 
> You can drop these four lines:
> 
> > # prevent library files from being installed
> > %global cargo_install_lib 0
> > %cargo_install -f dynamic,nssdb,standard
> > rm -f $RPM_BUILD_ROOT/%{_bindir}/conformance
> 
> If you don't call %cargo_install, the rest is unnecessary.
> You're already installing the .so file manually, so %cargo_install does
> *only* things you need to manually prevent or un-do.
> So ... just don't call %cargo_install xD

What about the `%{_bindir}/softhsm_migrate`? It is still needed to be installed.

> You also added a new issue into the License tag.
> 
> The "GPL-3.0" identifier is not a valid SPDX identifier in Fedora. It is
> deprecated in SPDX upstream, and should be either "GPL-3.0-or-later" or
> "GPL-3.0-only". Given that the SourceLicense is GPL-3.0-or-later, I assume
> it should be the former.

Thanks for double-checking. Fixed.

> I don't know where you copied the deprecated identifier from, because
> whereever it came from, it should have matched where GPL-3.0-or-later for
> SourceLicense came from. So there appears to be either an inconsistency or a
> copy-paste-error happened.

Let me try to backtrace the issue. I think I took it from the output of `%cargo_license_summary`. Running `mockbuild results in the following output, which I used to update the licence from:

```
+ /usr/bin/env CARGO_HOME=.cargo RUSTC_BOOTSTRAP=1 'RUSTFLAGS=-Copt-level=3 -Cdebuginfo=2 -Ccodegen-units=1 -Cstrip=none -Cforce-frame-pointers=yes -Clink-arg=-specs=/usr/lib/rpm/redhat/redhat-package-notes --cap-lints=warn' /usr/bin/cargo tree -Z avoid-dev-deps --workspace --offline --edges no-build,no-dev,no-proc-macro --no-dedupe --target all --features dynamic,nssdb,standard --prefix none --format '# {l}'
+ sed -e 's: / :/:g' -e 's:/: OR :g'
+ sort -u
# Apache-2.0
# Apache-2.0 OR BSL-1.0
# Apache-2.0 OR MIT
# BSD-2-Clause OR Apache-2.0 OR MIT
# BSD-3-Clause
# GPL-3.0
# MIT
# MIT OR Apache-2.0
# MIT-0 OR Apache-2.0
# Unlicense OR MIT
```

> What about the `%{_bindir}/softhsm_migrate`? It is still needed to be installed.

Ah, true. I missed that one. You could install that in the same way manually as you install the .so file, but in that case %cargo_install is not *entirely* useless :)

> Let me try to backtrace the issue. I think I took it from the output of `%cargo_license_summary`. Running `mockbuild results in the following output, which I used to update the licence from: (...)

This means that some crate in the dependency tree as `package.license = "GPL-3.0"` in its Cargo.toml. The LICENSE.dependencies file should tell you which one.

And obviously, its our package:

# grep GPL-3 LICENSE.dependencies
GPL-3.0: kryoptic v1.0.0

https://github.com/latchset/kryoptic/blob/main/Cargo.toml#L8

Will get it fixed. Thanks for pointing this out!