Bug 2233796 - Review Request: rust-libheif-sys - Libheif bindings
Summary: Review Request: rust-libheif-sys - Libheif bindings
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL: https://crates.io/crates/libheif-sys
Whiteboard:
Depends On:
Blocks: MultimediaSIG
TreeView+ depends on / blocked
 
Reported: 2023-08-23 13:10 UTC by Kalev Lember
Modified: 2023-08-25 07:13 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-08-24 21:07:42 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Kalev Lember 2023-08-23 13:10:29 UTC
Spec URL: https://kalev.fedorapeople.org/rust-libheif-sys.spec
SRPM URL: https://kalev.fedorapeople.org/rust-libheif-sys-1.16.1-1.fc40.src.rpm
Description: Libheif bindings.
Fedora Account System Username: kalev

Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=105182446

Note that the package is likely broken on i686, but since it builds, it may be practical to keep it in the distro on i686 to avoid a fallout in dependencies. Another option would be to ExcludeArch i686 (and also recursively in everything that depends on rust-libheif-sys), but I don't think it is worth fixing the actual issues as i686 is on its way out anyway.

Comment 1 Fedora Review Service 2023-08-23 13:20:54 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.

Comment 2 Fabio Valentini 2023-08-23 14:07:27 UTC
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).

Comment 3 Kalev Lember 2023-08-23 15:02:08 UTC
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?

Comment 4 Fabio Valentini 2023-08-23 15:10:49 UTC
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.

Comment 5 Fedora Admin user for bugzilla script actions 2023-08-23 15:13:23 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libheif-sys

Comment 6 Kalev Lember 2023-08-23 15:20:08 UTC
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).

Comment 7 Fedora Update System 2023-08-24 21:05:07 UTC
FEDORA-2023-3ad1cf0c25 has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2023-3ad1cf0c25

Comment 8 Fedora Update System 2023-08-24 21:07:42 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.