Bug 2277389 - Review Request: rust-bisection - Rust implementation of the Python bisect module
Summary: Review Request: rust-bisection - Rust implementation of the Python bisect module
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:
Whiteboard:
Depends On:
Blocks: 2277858
TreeView+ depends on / blocked
 
Reported: 2024-04-26 17:03 UTC by Ben Beasley
Modified: 2024-05-30 01:21 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2024-05-08 14:02:59 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)
Configuration file for rust2rpm (1.09 KB, text/plain)
2024-04-26 17:06 UTC, Ben Beasley
no flags Details
Crate to git diff (10.80 KB, patch)
2024-05-07 19:29 UTC, Ben Beasley
no flags Details | Diff

Description Ben Beasley 2024-04-26 17:03:46 UTC
Spec URL: https://music.fedorapeople.org/rust-bisection.spec
SRPM URL: https://music.fedorapeople.org/rust-bisection-0.1.0-1.fc39.src.rpm

Description:

Rust implementation of the Python bisect module.

Fedora Account System Username: music

This is a (hard/build) dependency for https://crates.io/crates/async_http_range_reader, which in turn is needed for https://github.com/astral-sh/uv.

Comment 1 Ben Beasley 2024-04-26 17:06:48 UTC
Created attachment 2029446 [details]
Configuration file for rust2rpm

The only hand-modification to the .spec file is to change the FIXME comment to:

%license %{crate_instdir}/LICENSE

to reflect the license file added by 4.patch.

Comment 2 Ben Beasley 2024-04-26 17:07:55 UTC
https://release-monitoring.org/project/372160/

Comment 3 Fabio Valentini 2024-05-07 19:18:12 UTC
I think I can solve the mystery about how something that's not in the git history ended up being published:

The commit that was published to crates.io (according to .cargo_vcs_info.json) is e5550fd4a136c60ab41c3be81209dd5539aa392d,
which is no longer part of the upstream git repo:

https://github.com/SteadBytes/bisection/commit/e5550fd4a136c60ab41c3be81209dd5539aa392d

It looks like the commit that's the current git HEAD was force-pushed *after* publishing to crates.io, so the commit that's associated with the 0.1.0 release on crates.io is no longer present in the commit history.

While this isn't necessary a cause for skepticism, I would still be good to compare whether there's any meaningful divergence between what's published and what's now at git HEAD.

Comment 4 Ben Beasley 2024-05-07 19:29:47 UTC
Created attachment 2032011 [details]
Crate to git diff

Diff from the contents of the published crate bisection-0.1.0.crate to the current contents of the master branch in the upstream git repository, https://github.com/SteadBytes/bisection/commit/14c95621a33842cdc01148d1d9e39ce16d2b9284.

Comment 5 Fabio Valentini 2024-05-07 19:32:24 UTC
So the fixed examples are indeed the only change. Why force-push for that *after* publishing .... :(

I'll continue the review then. Thank you for checking.

Comment 6 Ben Beasley 2024-05-07 19:34:43 UTC
Thanks for looking into it. Following .cargo_vcs_info.json was an excellent idea.

Comment 7 Fabio Valentini 2024-05-07 19:54:55 UTC
The project looks pretty dead :(

Thank you for filing the issue / PR upstream, but I'm not hopeful that it will do anything.

If uv / async_http_range_reader really needs this crate, it might be a good idea to fork it.

The upstream repo also has an open issue about a correctness bug (integer overflow in mid-point calculation) which would be good to fix.

Still, the package itself is now looking good (with the added LICENSE file being the best we can do for now unless upstream becomes active 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 (included manually from upstream Pull Request)
- 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)

===

If you have time, please review one of my pending packages in return.

Comment 8 Fabio Valentini 2024-05-07 20:06:20 UTC
I forgot: The comments for the patch files are double-"# " prefixed. You need to drop the "# " prefix from lines in rust2rpm.toml.

Comment 9 Ben Beasley 2024-05-07 21:01:50 UTC
(In reply to Fabio Valentini from comment #8)
> I forgot: The comments for the patch files are double-"# " prefixed. You
> need to drop the "# " prefix from lines in rust2rpm.toml.

There aren’t any; see https://bugzilla.redhat.com/show_bug.cgi?id=2277389#c1. The double "#"’s are coming from rust2rpm itself.

Comment 10 Fabio Valentini 2024-05-07 21:02:49 UTC
Wow, that's weird ... can you file a bug for that?

Comment 11 Ben Beasley 2024-05-07 21:05:50 UTC
(In reply to Fabio Valentini from comment #7)
> The project looks pretty dead :(
> 
> Thank you for filing the issue / PR upstream, but I'm not hopeful that it
> will do anything.
> 
> If uv / async_http_range_reader really needs this crate, it might be a good
> idea to fork it.
> 
> The upstream repo also has an open issue about a correctness bug (integer
> overflow in mid-point calculation) which would be good to fix.

Does this attempt at replacing the crate with standard-library functionality look correct to you?

https://github.com/musicinmybrain/async_http_range_reader/commit/dde1525a6fab737f74ba426a0ab4f4c1b30988af

The tests in async_http_range_reader pass with that change, but I don’t have too much confidence in its test coverage, and I would appreciate a second person looking at the documentation.

https://doc.rust-lang.org/std/vec/struct.Vec.html#method.partition_point

https://docs.rs/bisection/latest/bisection/fn.bisect_left.html
https://docs.rs/bisection/latest/bisection/fn.bisect_right.html

Comment 12 Ben Beasley 2024-05-07 21:11:17 UTC
(In reply to Fabio Valentini from comment #10)
> Wow, that's weird ... can you file a bug for that?

https://pagure.io/fedora-rust/rust2rpm/issue/273

Comment 13 Ben Beasley 2024-05-08 13:17:06 UTC
For now, I’ll go ahead and import this. I’ve opened https://github.com/SteadBytes/bisection/pull/5 and will apply it as a patch for the midpoint calculation bug. In the medium term, I’m hoping the approach in https://bugzilla.redhat.com/show_bug.cgi?id=2277389#c11 will allow async_http_range_reader to drop the dependency.

Comment 14 Fedora Admin user for bugzilla script actions 2024-05-08 13:18:06 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-bisection

Comment 15 Fedora Update System 2024-05-08 13:59:53 UTC
FEDORA-2024-831372236e (rust-bisection-0.1.0-2.fc41) has been submitted as an update to Fedora 41.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-831372236e

Comment 16 Fedora Update System 2024-05-08 14:02:59 UTC
FEDORA-2024-831372236e (rust-bisection-0.1.0-2.fc41) has been pushed to the Fedora 41 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 17 Fedora Update System 2024-05-16 18:15:27 UTC
FEDORA-2024-c2abca435e (rust-bisection-0.1.0-2.fc40, rust-http-content-range-0.1.2-1.fc40, and 4 more) has been submitted as an update to Fedora 40.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-c2abca435e

Comment 18 Fedora Update System 2024-05-16 18:15:39 UTC
FEDORA-2024-0bfd6dabc2 (rust-bisection-0.1.0-2.fc39, rust-http-content-range-0.1.2-1.fc39, and 4 more) has been submitted as an update to Fedora 39.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-0bfd6dabc2

Comment 19 Fedora Update System 2024-05-17 01:36:47 UTC
FEDORA-2024-0bfd6dabc2 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-0bfd6dabc2 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-0bfd6dabc2

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 20 Fedora Update System 2024-05-17 02:09:53 UTC
FEDORA-2024-c2abca435e has been pushed to the Fedora 40 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-c2abca435e \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-c2abca435e

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 21 Fedora Update System 2024-05-22 01:11:39 UTC
FEDORA-2024-c2abca435e has been pushed to the Fedora 40 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-c2abca435e \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-c2abca435e

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 22 Fedora Update System 2024-05-22 01:48:42 UTC
FEDORA-2024-0bfd6dabc2 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-0bfd6dabc2 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-0bfd6dabc2

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 23 Fedora Update System 2024-05-30 01:19:44 UTC
FEDORA-2024-0bfd6dabc2 (rust-async_http_range_reader-0.8.0-1.fc39, rust-bisection-0.1.0-2.fc39, and 6 more) has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 24 Fedora Update System 2024-05-30 01:21:11 UTC
FEDORA-2024-c2abca435e (rust-async_http_range_reader-0.8.0-1.fc40, rust-bisection-0.1.0-2.fc40, and 6 more) 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.