Bug 2281965

Created attachment 2034161 [details]
Configuration file for rust2rpm

Oops… this still has some arch-specific issues. Setting the status to MODIFIED until I figure out how to proceed.

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2281965-rust-tokio-tar/fedora-rawhide-x86_64/07467139-rust-tokio-tar/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.

I took a quick look at the test failures.

On i686, it looks like it's failing to allocate 1.6 GB in the doctest that adds the current directory (".") into a tar archive.
This likely includes the "target" directory, so any compiled artifacts generated during %cargo_build and %cargo_test.
This is likely working *most of the time*, but it's maybe not a good idea in general.

see https://github.com/vorot93/tokio-tar/blob/master/src/builder.rs#L400

On s390x, I can't really tell what's going wrong. The error message isn't that helpful because the test just "assert!(entries.next().await.is_none()" instead of using "assert_eq!(entries.next().await, None)", which would print the value that is "not none" in the error message instead of just saying "I wanted a true, but I got a false! FAIL"). Is the failure persistent? It might just have been a fluke.

see https://github.com/vorot93/tokio-tar/blob/master/tests/all.rs#L1103

(In reply to Fabio Valentini from comment #4)
> I took a quick look at the test failures.
> 
> On i686, it looks like it's failing to allocate 1.6 GB in the doctest that
> adds the current directory (".") into a tar archive.
> This likely includes the "target" directory, so any compiled artifacts
> generated during %cargo_build and %cargo_test.
> This is likely working *most of the time*, but it's maybe not a good idea in
> general.
> 
> see https://github.com/vorot93/tokio-tar/blob/master/src/builder.rs#L400

This is a logical explanation, although something still isn’t *entirely* adding up—when I tested this in an fedora-rawhide-i386 mock chroot, the target directory was only 303M, so this allocation still seems bizarrely large.

Would be nice if upstream had a bug tracker.

Still, an arch-dependent or even unconditional test skip is probably the way to go here regardless of what is going on. The only reason to build i686 is to avoid having to propagate ExcludeArch, not to actually *use* it.

> 
> On s390x, I can't really tell what's going wrong. The error message isn't
> that helpful because the test just "assert!(entries.next().await.is_none()"
> instead of using "assert_eq!(entries.next().await, None)", which would print
> the value that is "not none" in the error message instead of just saying "I
> wanted a true, but I got a false! FAIL"). Is the failure persistent? It
> might just have been a fluke.
> 
> see https://github.com/vorot93/tokio-tar/blob/master/tests/all.rs#L1103

Good question. I just kicked off five scratch builds:

https://koji.fedoraproject.org/koji/taskinfo?taskID=118005832
https://koji.fedoraproject.org/koji/taskinfo?taskID=118005833
https://koji.fedoraproject.org/koji/taskinfo?taskID=118005834
https://koji.fedoraproject.org/koji/taskinfo?taskID=118005842
https://koji.fedoraproject.org/koji/taskinfo?taskID=118005845

The test didn’t fail on s390x again, but it did fail twice on aarch64 with similar output:

failures:
---- insert_local_file_different_name stdout ----
thread 'insert_local_file_different_name' panicked at tests/all.rs:1103:5:
assertion failed: entries.next().await.is_none()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
    insert_local_file_different_name
test result: FAILED. 50 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.05s
error: test failed, to rerun pass `--test all`
     Running `/builddir/build/BUILD/tokio-tar-0.3.1/target/rpm/deps/entry-5ef2d6d85a16be6a`

…and one of those times, two other tests failed as well:

failures:
---- insert_local_file_different_name stdout ----
thread 'insert_local_file_different_name' panicked at tests/all.rs:1103:5:
assertion failed: entries.next().await.is_none()
---- large_filename stdout ----
thread 'large_filename' panicked at tests/all.rs:195:5:
assertion `left == right` failed
  left: 0
 right: 4
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- writing_files stdout ----
thread 'writing_files' panicked at tests/all.rs:160:5:
assertion `left == right` failed
  left: 0
 right: 4
failures:
    insert_local_file_different_name
    large_filename
    writing_files
test result: FAILED. 48 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.05s

As I just wrote in https://github.com/astral-sh/uv/issues/3423#issuecomment-2123668288,

It looks like the `insert_local_file_different_name` is flaky, roughly less than 20% of the time, on `s390x`, but it also occurs (more frequently) on `aarch64`. I have also observed flaky failures in other tests on `aarch64` – at least `large_filename` and `writing_files`, and my notes indicate that I saw `path_separators` panic in a previous attempt.

I think a reasonable conclusion here is that `tokio-tar` has significant race conditions, at least on some architectures, and should have engaged in much more fearful concurrency.

Now the question is what to do about the problem. I’ll see if the https://crates.io/crates/krata-tokio-tar crate mentioned in the uv issue is able to pass its own tests on all architectures or not. If so, maybe that would be a point in favor of upstream adopting it as at least a short-term band-aid.

(In reply to Ben Beasley from comment #6)
> Now the question is what to do about the problem. I’ll see if the
> https://crates.io/crates/krata-tokio-tar crate mentioned in the uv issue is
> able to pass its own tests on all architectures or not. If so, maybe that
> would be a point in favor of upstream adopting it as at least a short-term
> band-aid.

In ten scratch builds, I observed flaky test failures on x86_64, aarch64, ppc64le, and s390x. So it is also problematic.

I’m able to get this package to fail on every architecture if I do enough scratch builds, too. Furthermore, although it took 25 iterations, I was finally able to reproduce this with plain "cargo test --release" in a git checkout. I’m not sure yet if --release is required.

(In reply to Ben Beasley from comment #8)
> I’m able to get this package to fail on every architecture if I do enough
> scratch builds, too. Furthermore, although it took 25 iterations, I was
> finally able to reproduce this with plain "cargo test --release" in a git
> checkout. I’m not sure yet if --release is required.

I reproduced it in 82 iterations without --release.

I am not happy with this, but here is a submission that take sufficient measures to ignore the problems with this crate in order to almost always get a successful build.

Spec URL: https://music.fedorapeople.org/20240522/rust-tokio-tar.spec
SRPM URL: https://music.fedorapeople.org/20240522/rust-tokio-tar-0.3.1-1.fc39.src.rpm

Unless someone figures out how to straighten out the concurrency issues properly, or uv upstream figures out an alternative to this crate (and I hope they will), I don’t see what can be done other than paper over the problems and proceed with packaging this as-is.

Created attachment 2034602 [details]
Updated configuration for rust2rpm

Created attachment 2034603 [details]
The .spec file difference from Copr build 7467139 to 7476256

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2281965-rust-tokio-tar/fedora-rawhide-x86_64/07476256-rust-tokio-tar/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.

It's a bit unfortunate that the code here looks unreliable (looking at differences between the "tar" crate and "tokio-tar", it seems that wrong usage of Atomics might be to blame). But the package itself looks correct, and every change made on top of the spec file generated by rust2rpm is clearly documented.

===

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
  (tests appear to be unreliable, but the most-unreliable tests are skipped, so the package at least builds *most* of the time. probably the best one can do without skipping tests entirely, bar fixing the actual underlying issue that causes the unreliability)
- 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)

Thank you for the review!

The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-tokio-tar

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

FEDORA-2024-83ba0bb084 (rust-tokio-tar-0.3.1-1.fc41) has been submitted as an update to Fedora 41.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-83ba0bb084

FEDORA-2024-83ba0bb084 (rust-tokio-tar-0.3.1-1.fc41) has been pushed to the Fedora 41 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2024-eb65a23f70 (rust-tokio-tar-0.3.1-1.fc40) has been submitted as an update to Fedora 40.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-eb65a23f70

FEDORA-2024-1bae0600cb (rust-tokio-tar-0.3.1-1.fc39) has been submitted as an update to Fedora 39.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-1bae0600cb

FEDORA-2024-eb65a23f70 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-eb65a23f70 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-eb65a23f70

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

FEDORA-2024-1bae0600cb 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-1bae0600cb \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-1bae0600cb

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

FEDORA-2024-eb65a23f70 (rust-tokio-tar-0.3.1-1.fc40) has been pushed to the Fedora 40 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2024-1bae0600cb (rust-tokio-tar-0.3.1-1.fc39) has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.