https://bugzilla.redhat.com/show_bug.cgi?id=2266544 Fabio Valentini <decathorpe@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: rust-trunk |Review Request: trunk - |- Build, bundle & ship your |Build, bundle & ship your |Rust WASM application to |Rust WASM application to |the web |the web --- Comment #13 from Fabio Valentini <decathorpe@xxxxxxxxx> --- Looks good to me! I'm glad that this seems to be working better for this case. You don't need to file a new request, the bug title can just be changed (which I will just do now). Spec URL: https://dentrassi.de/download/trunk/trunk.spec SRPM URL: https://dentrassi.de/download/trunk/trunk-0.19.0-1.fc41.src.rpm The review service should pick this up, hopefully. There's a few more small things, but those don't block the final review (which I will do after posting this comment): ============================== 1. These seem to be leftover from the previous version, and I don't think they are necessary anymore: > %global crate trunk > %global crate_version 0.19.0 The `%crate_version` macro is only necessary if it doesn't match the Version string (i.e. for SemVer prereleases). So you could just replace occurrences of `%{crate}` with `%{name}` and `%{crate_version}` with `%{version}` now. Side note: This only works by accident right now, because you would need to use `%{crate_version}` instead of `%{version}` in the %autosetup argument as well to make it work for cases where version != crate_version: > %autosetup -n %{crate}-%{version} -p1 -a1 This should be (if you want to keep the separate %crate_version macro): %autosetup -n %{crate}-%{crate_version} -p1 -a1 ============================== 2. I don't think you need to pass the `--bin trunk` argument to `%cargo_test`. It should not be necessary to specify running tests only for the "trunk" binary. `cargo test` should do this by default if that is the only target available that has tests. ============================== 3. It would be great if there were a few more comments in the spec file for some things, notably: > # Disable defaults (update check and rustls), enable native-tls/openssl > %global _trunk_features -n -f native-tls Maybe this would be better: # global feature flags: # * disable self-update check # * switch crypto backend from Rustls to OpenSSL %global _trunk_features -n -f native-tls > License: (...) AND Unicode-DFS-2016 Here it would be good to document why Unicode-DFS-2016 needs to be added (i.e. that some crates bundle Unicode data, but do not specify this in their license metadata), maybe like this: # Unicode-DFS-2016: the regex-syntax crate bundles Unicode data License: (...) AND Unicode-DFS-2016 I think the only crate affected by this issue (i.e. crate bundles Unicode data but does not declare this in the metadata) that is also getting statically linked into /usr/bin/trunk is the regex-syntax crate. But this might change in the future, so please keep an eye on it (other crates that are affected by this issue are currently: bstr, tinystr, databake). > BuildRequires: openssl-devel It would be great to document why this is necessary: # the bundled openssl-sys crate requires OpenSSL headers to be present BuildRequires: openssl-devel -- 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=2266544 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202266544%23c13 -- _______________________________________________ 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
