Spec URL: https://music.fedorapeople.org/rust-lz4-sys.spec SRPM URL: https://music.fedorapeople.org/rust-lz4-sys-1.9.4-1.fc39.src.rpm Description: Rust LZ4 sys package. Fedora Account System Username: music This is a dependency for rust-lz4, which is a dependency for python-cramjam, which is a dependency for current versions of python-fastavro. This is my first attempt at this kind of unbundling patch in a Rust package, so feedback on anything I could do better or more easily in that area is especially welcome.
Copr build: https://copr.fedorainfracloud.org/coprs/build/6639465 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2249863-rust-lz4-sys/fedora-rawhide-x86_64/06639465-rust-lz4-sys/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
I took a quick peek at the package * There are no license files being installed here, probably because the crate is published without them. I normally submit a PR upstream that adds the license file. Looking at upstream here, a simple "cd lz4-sys && ln -s ../LICENSE" should do the trick here. To get this reviewed in the meantime, you can just add the license file manually and remove it later once your PR has been merged and released. * Your patch for using the system library looks good. Some comments: * Consider adding ".print_system_libs(false)" to the pkg-config patch to prevent linking errors for consumers * For Fedora you should force the usage of the system library, since this is the only way we want to link against "libz" Here is an example of how to do it: https://src.fedoraproject.org/rpms/rust-libz-sys/blob/rawhide/f/0001-unconditionally-use-pkg-config-to-link-with-system-z.patch
(In reply to blinxen from comment #2) > I took a quick peek at the package > > * There are no license files being installed here, probably because the > crate is published without them. > I normally submit a PR upstream that adds the license file. Looking at > upstream here, a simple "cd lz4-sys && ln -s ../LICENSE" should do the trick > here. > To get this reviewed in the meantime, you can just add the license file > manually and remove it later once your PR has been merged and released. Good catch, and good suggestion. I made a PR, https://github.com/10XGenomics/lz4-rs/pull/40, and I will add the appropriate LICENSE file as an additional source. > * Your patch for using the system library looks good. Some comments: > * Consider adding ".print_system_libs(false)" to the pkg-config patch to > prevent linking errors for consumers This seems reasonable, although I am not sure I understand how an extra “-L/usr/lib64” or similar would break anything. I’ll update my PR. If this is really a problem, it probably needs to be fixed in https://github.com/alexcrichton/bzip2-rs/blob/master/bzip2-sys/build.rs as well. > * For Fedora you should force the usage of the system library, since this > is the only way we want to link against "libz" > Here is an example of how to do it: > https://src.fedoraproject.org/rpms/rust-libz-sys/blob/rawhide/f/0001- > unconditionally-use-pkg-config-to-link-with-system-z.patch I’m trying to understand why additional patching would be needed; we can be certain the bundled liblz4 is not used because it is removed in %prep.
> I’m trying to understand why additional patching would be needed; we can be certain the bundled liblz4 is not used because it is removed in %prep. It's not strictly necessary in this case. Though I would not patch in a "static" feature that will never (and can never) be used. A case where an additional patch was helpful is here: https://src.fedoraproject.org/rpms/rust-pq-sys/blob/rawhide/f/0001-Unconditionally-use-pkg-config-to-link-against-syste.patch In that case, the pkgconfig code path is used unconditionally, regardless of feature flags passed to the build. That makes it unnecessary to patch dependent packages.
Ok, I added “.print_system_libs(false)” to the upstream PR https://github.com/10XGenomics/lz4-rs/pull/39 in https://github.com/10XGenomics/lz4-rs/pull/39/commits/59169aee7318bfba351bfd7ffeaa0d36c6b835cf, and I reverted the static features in https://github.com/10XGenomics/lz4-rs/pull/39/commits/93cbd4eb203f1159e341685ceb9bf04f8171a5ed. I didn’t do any additional pkgconfig-related downstream patching because the code path to use an external liblz4 should be already used unconditionally in practice. Unless I’m missing something, I think that addresses everything. New Spec File: https://music.fedorapeople.org/20231205/rust-lz4-sys.spec New SRPM File: https://music.fedorapeople.org/20231205/rust-lz4-sys-1.9.4-1.fc39.src.rpm
Created attachment 2002827 [details] The .spec file difference from Copr build 6639465 to 6726602
Copr build: https://copr.fedorainfracloud.org/coprs/build/6726602 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2249863-rust-lz4-sys/fedora-rawhide-x86_64/06726602-rust-lz4-sys/fedora-review/review.txt Found issues: - No gcc, gcc-c++ or clang found in BuildRequires Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Anyone see anything further that needs to be adjusted here? If not, would anyone care to do the official review? Thanks!
Thank you for submitting your patches to upstream! Package looks good to me. Two tips / hints for the future: 1. Please use "rust2rpm -p" for applying changes to Cargo.toml. Changes to Cargo.toml can affect the spec file that is generated by rust2rpm, so these changes need to happen at a stage where the changes *can* affect spec generation (which is what "rust2rpm -p" is for). 2. You can store a rust2rpm.toml config file in dist-git to preserve some changes for future updates: ```toml [requires] build = ["pkgconfig(liblz4)"] lib = ["pkgconfig(liblz4)"] ``` This will automatically add the "pkgconfig(liblz4)" BuildRequires and "-devel" package Requires when running rust2rpm again. === 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 and is acceptable for Fedora - license file is included with %license in %files (manually included from upstream project for now) - package complies with Rust Packaging Guidelines Package APPROVED. === Recommended post-import rust-sig tasks: - 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 - add @rust-sig with "commit" access as package co-maintainer (should happen automatically) - set bugzilla assignee overrides to @rust-sig (optional) - track package in koschei for all built branches (should happen automatically once rust-sig is co-maintainer)
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-lz4-sys
(In reply to Fabio Valentini from comment #9) > Thank you for submitting your patches to upstream! Package looks good to me. Thank you for the review! > Two tips / hints for the future: > > 1. Please use "rust2rpm -p" for applying changes to Cargo.toml. > Changes to Cargo.toml can affect the spec file that is generated by > rust2rpm, so these changes need to happen at a stage where the changes *can* > affect spec generation (which is what "rust2rpm -p" is for). https://src.fedoraproject.org/rpms/rust-lz4-sys/c/5f039fcda47c52f3234a889e3260aa555664b3ba?branch=rawhide > 2. You can store a rust2rpm.toml config file in dist-git to preserve some > changes for future updates: > […] https://src.fedoraproject.org/rpms/rust-lz4-sys/c/a31c09bee84d3c32c0115a3125def01d22294a95?branch=rawhide > Recommended post-import rust-sig tasks: > > - set up package on release-monitoring.org: Done, https://release-monitoring.org/project/370756/ > - add @rust-sig with "commit" access as package co-maintainer > (should happen automatically) Done, hadn’t happened automatically (yet?) > - set bugzilla assignee overrides to @rust-sig (optional) Not planned > - track package in koschei for all built branches > (should happen automatically once rust-sig is co-maintainer) Done
FEDORA-2024-2b3e782706 has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2024-2b3e782706
FEDORA-2024-2b3e782706 has been pushed to the Fedora 40 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2024-0407cd3e71 has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2024-0407cd3e71
FEDORA-2024-080062b0df has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2024-080062b0df
FEDORA-EPEL-2024-5446a7b08a has been submitted as an update to Fedora EPEL 9. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2024-5446a7b08a
FEDORA-EPEL-2024-5446a7b08a has been pushed to the Fedora EPEL 9 testing repository. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2024-5446a7b08a See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2024-080062b0df has been pushed to the Fedora 38 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-080062b0df \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-080062b0df See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2024-0407cd3e71 has been pushed to the Fedora 39 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-0407cd3e71 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-0407cd3e71 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2024-5446a7b08a has been pushed to the Fedora EPEL 9 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2024-080062b0df has been pushed to the Fedora 38 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2024-0407cd3e71 has been pushed to the Fedora 39 stable repository. If problem still persists, please make note of it in this bug report.