https://bugzilla.redhat.com/show_bug.cgi?id=1991164 Fabio Valentini <decathorpe@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@xxxxxxxxxxxxxxxxx |decathorpe@xxxxxxxxx Flags| |fedora-review? Status|NEW |ASSIGNED --- Comment #8 from Fabio Valentini <decathorpe@xxxxxxxxx> --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=1991164 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
