Bug 1991164 - Review Request: rust-libsodium-sys - FFI binding to libsodium
Summary: Review Request: rust-libsodium-sys - FFI binding to libsodium
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1976751 2121490 2045255
TreeView+ depends on / blocked
 
Reported: 2021-08-07 19:06 UTC by Stuart D Gathman
Modified: 2022-10-13 11:58 UTC (History)
3 users (show)

Fixed In Version: rust-libsodium-sys-0.2.7-1.fc38
Clone Of:
Environment:
Last Closed: 2022-10-13 11:55:28 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Stuart D Gathman 2021-08-07 19:06:33 UTC
Spec URL: https://gathman.org/linux/SPECS/rust-libsodium-sys.spec
SRPM URL: https://gathman.org/linux/f35/src/rust-libsodium-sys-0.2.7-1.fc35.src.rpm
Description: 
This package contains library source intended for building other packages
which use "libsodium-sys" crate.

Fedora Account System Username: sdgathman

Comment 1 Fabio Valentini 2022-04-30 09:48:57 UTC
Are you still interested in packaging this crate?
If yes, please adapt the packaging to drop the vendored copy of libsodium (i.e. "rm -rf libsodium" in %prep) and make it link against the shared library from the system (i.e. patch build.rs to enable code gated behind "use-pkg-config" feature enabled unconditionally).

Comment 2 Stuart D Gathman 2022-04-30 15:28:49 UTC
Thanks for the response!  Yes, ultimately I need to package sodiumoxide, which has a number of dependencies including this.   I am new to rust packaging, so your feedback is very welcome.  Let me try it ...

Comment 3 Fabio Valentini 2022-05-01 13:39:29 UTC
The changes that are required to make this work well as a distro package don't look too bad:

1) Patch the build.rs file according to these rules:
- remove all blocks that are gated behind #[cfg(not(feature = "use-pkg-config"))]
- remove #[cfg(feature = "use-pkg-config)] attributes to make those blocks compile unconditionally

2) Do "rm -r libsodium" between %prep and %cargo_prep.

This should ensure that the crate always dynamically links against the system copy of libsodium, whether during its own build or while it's being compiled as dependency for another crate.

Comment 4 Stuart D Gathman 2022-05-24 22:37:01 UTC
Haven't succeeded yet, but trying again carefully.

Comment 5 Stuart D Gathman 2022-08-11 21:21:09 UTC
https://gathman.org/linux/SPECS/rust-libsodium-sys.spec

Removing the libsodium breaks the build script.  I'm reposting for any help.

  thread 'main' panicked at 'Failed to find configure script! Did you clone the submodule at `libsodium-sys/libsodium`?: Os { code: 2, kind: NotFound, message: "No such file or directory" }', build.rs:232:89

Comment 6 Stuart D Gathman 2022-08-11 21:25:43 UTC
Added BR: libsodium-devel

But it doesn't help

Comment 8 Fabio Valentini 2022-08-18 09:48:43 UTC
Ok, starting the formal review process.

1) All new Rust packages should be using rpmautospec. Did you intentionally opt out?

2) You should use rust2rpm's "-p" flag when you want to patch Cargo.toml, it automates the "edit + create patch + add patch to generated spec file" steps.

3) Don't mix tabs and spaces (in the Patch0 line).

4) You need to add "Requires: libsodium-devel" to the "-devel" sub-package, in addition to the BuildRequires.
Otherwise dependent packages will fail to build, as libsodim-devel will not be pulled in as a transitive dependency.

5) Remove the commented-out "#license %{crate_instdir}/libsodium/LICENSE" line.
It doesn't provide any information, and creates warnings about expanded macros inside comments.

6) Add some comments why you're removing a bunch of stuff in %prep, for example:

# remove pre-built libsodium objects for MSVC / MinGW targets
rm -r mingw/ msvc/
# remove bundled libsodium sources
rm -r libsodium/

7) Either explain what the sed script does to src/gen.sh, or do it differently.

> sed -i -e '1 i#!/bin/sh' src/gen.sh

I have never seen this sed expression syntax before, and I wouldn't know what to do with it when I need to touch this package.
I assume you want to delete the shebang from the script, in order to prevent a generated dependency on /bin/sh?

8) The bindings are machine-generated code, and we *SHOULD* be re-generating them during the build process.
We do have all the tools that are necessary to do this packaged for Fedora, and it would be pretty easy to adapt the build.rs script to do automatically (by using the bindgen library interface) what that "src/gen.sh" script does manually (by using the bindgen CLI interface).

9) Remove "export RUST_BACKTRACE=1" from %build.
I don't know why it's there, but it should never be needed in %build (though sometimes it's needed in %check to make some tests work as expected).

10) Rather than patch Cargo.toml to make the "pkg-config" feature a default feature, we should patch the build.rs to use pkg-config unconditionally. This is much safer, especially since dependent crates can work around the current patch by using "no-default-features = true" for their libsodium dependency, in which case, they will break.

Comment 9 Stuart D Gathman 2022-10-11 01:13:43 UTC
Spec URL: https://gathman.org/linux/SPECS/rust-libsodium-sys.spec
SRPM URL: https://gathman.org/linux/f37/src/rust-libsodium-sys-0.2.7-5.fc37.src.rpm

I switched to rpmautospec (hopefully ok).  You prefer patching build.rs, so I didn't address 2.

Added FIXME comments for 8 and 10

Comment 10 Fabio Valentini 2022-10-11 09:09:13 UTC
ad 2)

This is independent of the other point. If you patch Cargo.toml, then using "rust2rpm -p" is the standard way to do that - it opens an Cargo.toml in an editor for you, generates the patch file once you save changes, and includes that patch file in the generated spec file automatically.

It's not a blocker though. I can clean this up with a follow-up commit.

ad 7)

> # Add shebang to top of gen.sh as build tries to run it directly
> # FIXME: patch build.rs to use the bindgen library interface instead
> sed -i -e '1 i#!/bin/sh' src/gen.sh

I don't understand this. "src/gen.sh" is not run during the build at all.

===

Either way, these are now small problems (points 2, 7, 8, 10) and the package looks good (other than a missing %changelog tag before %autochangelog). If you convert package to rpmautospec, you can use "rpmautospec convert" command, which automates this. Please fix this before importing the package.

I'll then push a follow-up commit to address the remaining non-blocking issues, so please hold off launching a koji build after importing the package.

Package APPROVED.

===

Package was generated with rust2rpm, simplifying the review.

- 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 (MIT OR Apache-2.0) and is acceptable for Fedora
- license file is included with %license in %files
- package complies with Rust Packaging Guidelines

Package APPROVED.

===

Recommended post-import rust-sig tasks:

- add @rust-sig with "commit" access as package co-maintainer

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

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

- track package in koschei for all built branches

===

You can proceed with requesting the dist-git repo (and branches) now:

$ fedpkg request-repo rust-libsodium-sys 1991164
$ fedpkg request-branch --repo rust-libsodium-sys f37
(same command for f36 and f35, if you need this package there)

Comment 11 Gwyn Ciesla 2022-10-12 14:01:03 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libsodium-sys

Comment 12 Fabio Valentini 2022-10-13 10:30:45 UTC
Looks like you ignored my request not to launch a koji build yet?
https://koji.fedoraproject.org/koji/buildinfo?buildID=2074817

Either way, I set up release-monitoring and koschei for the package, since that wasn't done yet, either ...
So this bug can now probably be closed?

Comment 13 Stuart D Gathman 2022-10-13 11:55:28 UTC
I ended up manually copying files for import - did not know the trick with rpmbuild -bs.  I did a build in rawhide, thinking you were concerned about f37.  In hindsight, I should have done a scratch build.  I apologize.  My only excuse is working too late on the project.


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