Bug 2292522 - Review Request: rust-zune-jpeg - Fast, correct and safe jpeg decoder
Summary: Review Request: rust-zune-jpeg - Fast, correct and safe jpeg decoder
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: wojnilowicz
QA Contact: Fedora Extras Quality Assurance
URL: https://crates.io/crates/zune-jpeg
Whiteboard:
Depends On:
Blocks: 2268816
TreeView+ depends on / blocked
 
Reported: 2024-06-15 19:09 UTC by Fabio Valentini
Modified: 2024-07-07 18:01 UTC (History)
2 users (show)

Fixed In Version: rust-zune-jpeg-0.4.11-1.fc41
Clone Of:
Environment:
Last Closed: 2024-07-07 18:01:32 UTC
Type: ---
Embargoed:
lukasz.wojnilowicz: fedora-review+


Attachments (Terms of Use)

Description Fabio Valentini 2024-06-15 19:09:02 UTC
Spec URL: https://decathorpe.fedorapeople.org/rust-zune-jpeg.spec
SRPM URL: https://decathorpe.fedorapeople.org/rust-zune-jpeg-0.4.11-1.fc40.src.rpm

Description:
A fast, correct and safe jpeg decoder.

Fedora Account System Username: decathorpe

Comment 1 Fabio Valentini 2024-06-15 19:09:04 UTC
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=119071015

Comment 2 Fedora Review Service 2024-06-15 19:16:49 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7617617
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2292522-rust-zune-jpeg/fedora-rawhide-x86_64/07617617-rust-zune-jpeg/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 3 wojnilowicz 2024-06-28 20:09:02 UTC
1) Shouldn't the following subpackage:

%package     -n %{name}+x86-devel
Summary:        %{summary}
BuildArch:      noarch

be somehow restricted to x86_64, and not be noarch?
At https://github.com/etemesi254/zune-image/blob/dev/crates/zune-jpeg/README.md#crate-features it says that this "Enables x86 specific instructions"

2) x86 requires x86_64-v2, x86_64-v3. Wouldn't that be an issue for x86_64-v1 when used in an application using this as a dependency?

[fedora-review-service-build]

Comment 4 Fedora Review Service 2024-06-28 20:16:48 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7689515
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2292522-rust-zune-jpeg/fedora-rawhide-x86_64/07689515-rust-zune-jpeg/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 5 Fabio Valentini 2024-07-02 15:39:27 UTC
(In reply to wojnilowicz from comment #3)
> 1) Shouldn't the following subpackage:
> 
> %package     -n %{name}+x86-devel
> Summary:        %{summary}
> BuildArch:      noarch
> 
> be somehow restricted to x86_64, and not be noarch?

No, this is not possible.
The "-devel" package must be noarch and be available on all architectures, otherwise you get broken dependencies somewhere.

We even have special machinery in rust2rpm for crates that only compile on specific architectures (the "supported-arches" setting in rust2rpm.toml) because of this.

> At
> https://github.com/etemesi254/zune-image/blob/dev/crates/zune-jpeg/README.
> md#crate-features it says that this "Enables x86 specific instructions"

This should not be an issue according to the README:

https://github.com/etemesi254/zune-image/blob/dev/crates/zune-jpeg/README.md#crate-features

"""
Note that the x86 features are automatically disabled on platforms that aren't x86 during compile time hence there is no need to disable them explicitly if you are targeting such a platform.
"""


> 2) x86 requires x86_64-v2, x86_64-v3. Wouldn't that be an issue for
> x86_64-v1 when used in an application using this as a dependency?

That is a good question. It looks like there is code to guard against using instructions on CPUs where they are not available:

https://github.com/etemesi254/zune-image/blob/dev/crates/zune-core/src/options/decoder.rs#L451-L456

This should work under most circumstances. The only way this could break (as far as I can tell) would be if 1) zune-jpeg is built without "std" support 2) with "x86" feature enabled *and* 3) the feature in question is available in the environment where the code is compiled but *not* on the machine the code gets executed on.

Comment 6 wojnilowicz 2024-07-07 07:20:48 UTC
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 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)

===

Thanks for the explanations. They seem reasonable to me. What about this?

(In reply to Fabio Valentini from comment #5)
> (In reply to wojnilowicz from comment #3)
> > 1) Shouldn't the following subpackage:
> > 
> > %package     -n %{name}+x86-devel
> > Summary:        %{summary}
> > BuildArch:      noarch
> > 
> > be somehow restricted to x86_64, and not be noarch?
> 
> No, this is not possible.
> The "-devel" package must be noarch and be available on all architectures,
> otherwise you get broken dependencies somewhere.
> 
> We even have special machinery in rust2rpm for crates that only compile on
> specific architectures (the "supported-arches" setting in rust2rpm.toml)
> because of this.

How is it different from https://bugzilla.redhat.com/show_bug.cgi?id=2276290 ?
Is it because "supported-arches" there would be used for the whole crate, because the crate as a whole wouldn't work,
while here it would have to be used only for a subpackage of the crate, because only this subpackage makes no sense? I would assume that's impossible to filter it out from the rpm point of view, however it's still doable to package it friction less thanks to the following note:
 
> """
> Note that the x86 features are automatically disabled on platforms that
> aren't x86 during compile time hence there is no need to disable them
> explicitly if you are targeting such a platform.
> """

Comment 7 Fabio Valentini 2024-07-07 17:38:46 UTC
Thank you for the thorough review!

> Is it because "supported-arches" there would be used for the whole crate, because the crate as a whole wouldn't work,
> while here it would have to be used only for a subpackage of the crate, because only this subpackage makes no sense?
> I would assume that's impossible to filter it out from the rpm point of view, however it's still doable to package it friction less thanks to the following note (...)

Yeah. supported-arches only affects which architectures a crate is built on and where tests are run. It doesn't affect the package contents at all.
Packages that have "BuildArch: noarch" also aren't filtered out by architecture (they are supposed to be architecture-independent after all), so there's not much that can be done here.

Comment 8 Fedora Admin user for bugzilla script actions 2024-07-07 17:39:38 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-zune-jpeg

Comment 9 Fabio Valentini 2024-07-07 18:01:32 UTC
Imported and built:
https://bodhi.fedoraproject.org/updates/FEDORA-2024-70b523927a


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