Bug 2279208

Copr build:
https://copr.fedorainfracloud.org/coprs/build/7417530
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2279208-rust-libz-ng-sys/fedora-rawhide-x86_64/07417530-rust-libz-ng-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.

Created attachment 2031520 [details]
Configuration file for rust2rpm

The only manual change to the spec file produced by rust2rpm was removing

%license %{crate_instdir}/src/zlib-ng/LICENSE.md

since the bundled zlib-ng in src/zlib-ng/ is removed in %prep.

Created attachment 2031521 [details]
Configuration file for rust2rpm

Removed some inapplicable comment lines.

Updated using the same URLs.

[fedora-review-service-build]

Created attachment 2031523 [details]
The .spec file difference from Copr build 7417530 to 7418707

Copr build:
https://copr.fedorainfracloud.org/coprs/build/7418707
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2279208-rust-libz-ng-sys/fedora-rawhide-x86_64/07418707-rust-libz-ng-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.

It occurred to me that

#[allow(dead_code)]

in the downstream build.rs is not needed (although it is harmless). I’ll plan to remove that line on import.

It's a bit sad that there are literally *zero* tests in this crate.
Usually those are helpful in "-sys" crates because if there's at lease *one* test, the resulting binary is linked against the C library.

If there's zero tests, I don't think linking is even attempted once during the build. So I can't really verify that this works as expected ... but the patch (i.e. the replaced build script) looks sane to me.

I guess we will see if this works when building zip v1 - the tests built and run as part of its build should get linked to z-ng if this crate does its job.

===

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 (there are no tests)
✅ latest version of the crate is packaged
✅ license matches upstream specification and is acceptable for Fedora
✅ license files are included with %license in %files
✅ 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)

(In reply to Fabio Valentini from comment #8)
> It's a bit sad that there are literally *zero* tests in this crate.
> Usually those are helpful in "-sys" crates because if there's at lease *one*
> test, the resulting binary is linked against the C library.
> 
> If there's zero tests, I don't think linking is even attempted once during
> the build. So I can't really verify that this works as expected ... but the
> patch (i.e. the replaced build script) looks sane to me.
> 
> I guess we will see if this works when building zip v1 - the tests built and
> run as part of its build should get linked to z-ng if this crate does its
> job.

Hmm, this is a good point. I tested this with https://src.fedoraproject.org/rpms/rust-flate2/pull-request/2, but I suppose without enabling extra features it wouldn’t really be usefully tested there.

As an experiment, I just tried hacking up the flate2 rust2rpm.toml to explicitly enable the zlib-ng and libz-ng-sys features, resulting in e.g. “%cargo_test -f libz-ng-sys,zlib-ng”. That worked, but I’m not sure how to verify that this really caused the tests to link the zlib-ng C library.

As you said, I suppose if there are any problems we’ll just have to go back and fix them.

Thank you for the review.

The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libz-ng-sys

https://release-monitoring.org/project/372388/

FEDORA-2024-347e6de6e9 (rust-libz-ng-sys-1.1.15-1.fc41) has been submitted as an update to Fedora 41.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-347e6de6e9

Aha, I had started to test this locally with (at the time) rust-zip-1.1.2, but I hadn’t finished the zstd/zstd-safe/zstd-sys PR’s yet.

I just tried again and ran into issues with the tests wanting additional data files, like

error: couldn't read src/read/../../tests/data/invalid_offset.zip: No such file or directory (os error 2)
   --> src/read/stream.rs:248:46
    |
248 |           ZipStreamReader::new(io::Cursor::new(include_bytes!(
    |  ______________________________________________^
249 | |             "../../tests/data/invalid_offset.zip"
250 | |         )))
    | |_________^
    |
    = note: this error originates in the macro `include_bytes` (in Nightly builds, run with -Z macro-backtrace for more info)

so it looks like a little more work will be required on rust-zip before I can really double-check this.

FEDORA-2024-347e6de6e9 (rust-libz-ng-sys-1.1.15-1.fc41) has been pushed to the Fedora 41 stable repository.
If problem still persists, please make note of it in this bug report.