Bug 2233796
| Summary: | Review Request: rust-libheif-sys - Libheif bindings | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Kalev Lember <klember> |
| Component: | Package Review | Assignee: | Fabio Valentini <decathorpe> |
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | decathorpe, package-review |
| Target Milestone: | --- | Flags: | decathorpe:
fedora-review+
|
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| URL: | https://crates.io/crates/libheif-sys | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2023-08-24 21:07:42 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | |||
| Bug Blocks: | 2218117 | ||
|
Description
Kalev Lember
2023-08-23 13:10:29 UTC
Copr build: https://copr.fedorainfracloud.org/coprs/build/6337697 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2233796-rust-libheif-sys/fedora-rawhide-x86_64/06337697-rust-libheif-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. Since the crate already supported regenerating the Rust bindings at build-time, it was relatively straightforward to do that unconditionally (this is also preferred by the Packaging Guidelines). This also solves the test failure on i686: https://koji.fedoraproject.org/koji/taskinfo?taskID=105185550 For this, the bindgen build-dependency needs to be non-optional, and small changes to build.rs and src/lib.rs are necessary. I've posted an SRPM with a modified spec + patches online, if you want to take a look: https://decathorpe.fedorapeople.org/rust-libheif-sys-1.16.1-1.fc38.src.rpm Essentially, what I did was patching out code paths for non-bindgen use, and made those for bindgen use unconditional (both in build.rs and src/lib.rs, and src/bindings.rs - the pre-generated bindings and tests - could be removed entirely, since it's then unused). Oh nice, thanks! Makes a lot of sense for me to do it like that. The only thing I'd change there is that in the patch, the hunk that changes build.rs I'd keep the existing indentation for the bindgen code path - basically keep the {} that comes after #[cfg(feature = "use-bindgen")] intact, so that the indentation doesn't change. I think doing it like that makes it easier to see what's changed since this is, I assume, going to live as a downstream patch forever. Or do you think there's a way to upstream it?
Maybe we can create the repo, I push my initial packaging and then you can change it to use bindgen unconditionally?
You're right, that would make the patch cleaner. Package looks fine either way. I'm on my phone right now so I don't have access to my review template, so the package is just APPROVED for now - I already ran the checks earlier and nothing noteworthy stood out. The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libheif-sys OK, I pushed my initial packaging to the repo, but didn't do a build. Feel free to go ahead and push your changes on top of that :) (I also added you as an admin for the package, hope you don't mind). FEDORA-2023-3ad1cf0c25 has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2023-3ad1cf0c25 FEDORA-2023-3ad1cf0c25 has been pushed to the Fedora 40 stable repository. If problem still persists, please make note of it in this bug report. |