https://bugzilla.redhat.com/show_bug.cgi?id=1991164
Bug ID: 1991164 Summary: Review Request: rust-libsodium-sys - FFI binding to libsodium Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: stuart@gathman.org QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
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
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |decathorpe@gmail.com Doc Type|--- |If docs needed, set a value
--- Comment #1 from Fabio Valentini decathorpe@gmail.com --- 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).
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
--- Comment #2 from Stuart D Gathman stuart@gathman.org --- 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 ...
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
--- Comment #3 from Fabio Valentini decathorpe@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
--- Comment #5 from Stuart D Gathman stuart@gathman.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
--- Comment #6 from Stuart D Gathman stuart@gathman.org --- Added BR: libsodium-devel
But it doesn't help
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
--- Comment #7 from Stuart D Gathman stuart@gathman.org --- 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-1.fc37.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |decathorpe@gmail.com Flags| |fedora-review? Status|NEW |ASSIGNED
--- Comment #8 from Fabio Valentini decathorpe@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
Stuart D Gathman stuart@gathman.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2121490
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2121490 [Bug 2121490] Review Request: rust-sodiumoxide - Fast cryptographic library for Rust
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
Stuart D Gathman stuart@gathman.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2045255
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2045255 [Bug 2045255] cjdns: FTBFS in Fedora rawhide/f36
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
--- Comment #9 from Stuart D Gathman stuart@gathman.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #10 from Fabio Valentini decathorpe@gmail.com --- 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)
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
--- Comment #11 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libsodium-sys
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
--- Comment #12 from Fabio Valentini decathorpe@gmail.com --- 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?
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
Stuart D Gathman stuart@gathman.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |NOTABUG Status|POST |CLOSED Last Closed| |2022-10-13 11:55:28
--- Comment #13 from Stuart D Gathman stuart@gathman.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1991164
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mhroncok@redhat.com Resolution|NOTABUG |RAWHIDE Fixed In Version| |rust-libsodium-sys-0.2.7-1. | |fc38
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com