https://bugzilla.redhat.com/show_bug.cgi?id=2449216
--- Comment #6 from Fabio Valentini <[email protected]> --- Sorry for the delay. First review pass included below. In general, I would strongly recommend to make as little modifications that cannot be automated by rust2rpm settings (i.e. a rust2rpm.toml config file) as possible. You need to regenerate the .spec file for every new version, so any manual modifications you make are just additional (and usually error-prone) work on every version update. It also looks as if you modified the generates spec file a lot instead of teaching rust2rpm how to generate the correct spec file for this type of package. I would recommend to patch the Cargo.toml [[lib]] target to have both "rlib" and "cdylib" crate-type, which will make the results of running rust2rpm much closer to what you need. I think all things you manually did (except for the two small Cargo.toml metadata changes) could be persisted across rust2rpm regenerations by using a rust2rpm.toml config file. Let me know if you want help with that, it should make long-term maintenance of this package much easier. Other comments: 1. You're disabling the debuginfo packages ("%global debug_package %{nil}"). This is wrong for packages that ship ELF executables and libraries, please remove this statement. rust2rpm should not generate it for you in the first place. If you need to *add* it manually to make the package build, then something else is wrong (likely something setting wrong compiler flags). 2. Defining "so_ver 0.2" isn't really necessary. The Rust library interface and the shared library appear to follow the same compatibility guarantees (i.e. SemVer). So any "breaking" update for the Rust crate is already automatically a "breaking" update for the shared library. 3. Do not include the License texts separately and manually. They are already included in the published ".crate" archives. Additionally, you used the wrong URLs. They point at the GitHub HTML UI for those files, not the raw / plain-text file contents themselves. 4. There's missing documentation for the Cargo.toml metadata patch you apply. Looks like this is to update a dependency to the version that is available in Fedora. If this is an update that is as easy as it looks here, then you should submit this upstream (or, if a PR or upstream commit already exists, add a link to that to the spec file). Keeping patches like this downstream is just accumulating tech debt. :) 5. What are the BuildRequires for lld, clang, and fdupes for? They all appear to be unused, please remove them. 6. The License tag for the libpathrs subpackage is incomplete. You copied only "MIT OR Apache-2.0" into the spec file (but didn't add that to the License tag). The full "%cargo_license_summary" output is actually: ### BEGIN LICENSE SUMMARY ### # Apache-2.0 # Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT # BSD-2-Clause OR Apache-2.0 OR MIT # MIT OR Apache-2.0 # MPL-2.0 OR LGPL-3.0-or-later # Unlicense OR MIT # Zlib OR Apache-2.0 OR MIT ### END LICENSE SUMMARY ### All these need to be represented in the License tag of the "libpathrs" subpackage. 7. Why did you add "Requires: glibc-devel" to the libpathrs-devel subpackage? I don't think this is necessary. 8. There is an unowned directory in the -devel subpackage: "%{_includedir}/pathrs//pathrs.h" has a "//" but still doesn't own the containing "%dir %{_includedir}/pathrs". 9. The way you run tests is broken due to what appears to be a copy-paste mistake. You appear to have copy-pasted an em-dash as a %cargo_test argument instead of two hyphens "--". This makes %cargo_test treat is as a test name filter, and since no test name contains an em-dash, no tests are run at all. In general, I would recommend to also run %cargo_test with the "-a" flag, since you already pass that to all other macros. Otherwise you will *build* and ship features of the library that are not turned on when building and running tests. -- 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=2449216 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202449216%23c6 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] 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/[email protected] Do not reply to spam, report it: https://forge.fedoraproject.org/infra/tickets/issues/new
