Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The tokio-tar dependency seems unmaintained, and it is broken on PowerPC #3423

Open
mgorny opened this issue May 7, 2024 · 10 comments
Open
Labels
upstream An upstream dependency is involved

Comments

@mgorny
Copy link
Contributor

mgorny commented May 7, 2024

The current version of uv depends on the tokio-tar crate:

uv/Cargo.toml

Line 132 in 8e86cd0

tokio-tar = { version = "0.3.1" }

However, the upstream repository has not seen any activity for 10 months now, and the project seems dead. It is currently broken on PowerPC, and I've filed a pull request to fix it. However, I don't hold my hopes high and in general, the repository has a feeling of a fork of async-tar that wasn't really meant to be maintained (note that it doesn't even have a bug tracker).

That said, krata-tokio-tar seems to be a fork of the fork that has had some recent activity, so I've filed my pull request there as well.

Perhaps a better option would be not to rely on tokio-tar (or async-tar) at all.

@musicinmybrain
Copy link
Contributor

Thanks for raising this issue. I’m working on packaging uv for Fedora, and tokio-tar is one of a handful of dependencies I hadn’t gotten around to investigating yet. Even if the dependency stays, your PR will be helpful as a downstream patch.

@zanieb zanieb added the upstream An upstream dependency is involved label May 17, 2024
@zanieb
Copy link
Member

zanieb commented May 17, 2024

Seems reasonable to switch to a maintained repository.

cc @BurntSushi who has more context on auditing Rust packages than me.

@mgorny
Copy link
Contributor Author

mgorny commented May 18, 2024

For the record, I've gotten an initial response on the krata-tokio-tar fork but they haven't found time to review the PR yet.

@musicinmybrain
Copy link
Contributor

I’m trying to build a Fedora package rust-tokio-tar using edera-dev/tokio-tar#1, and it’s working on x86_64, aarch64, and ppc64le, but still failing on i686:

test src/builder.rs - builder::Builder<W>::append_dir_all (line 390) ... FAILED
failures:
---- src/builder.rs - builder::Builder<W>::append_dir_all (line 390) stdout ----
Test executable failed (signal: 6 (SIGABRT) (core dumped)).
stderr:
memory allocation of 1677721600 bytes failed
failures:
    src/builder.rs - builder::Builder<W>::append_dir_all (line 390)
test result: FAILED. 9 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.01s

…and on s390x:

test header::set_metadata_deterministic ... ok
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

At a glance, the allocation on i686 is suspiciously large, and endianness issues are always prime suspects for s390x-specific problems, but I haven’t tried to investigate these in detail yet.

@musicinmybrain
Copy link
Contributor

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.

@musicinmybrain
Copy link
Contributor

That said, krata-tokio-tar seems to be a fork of the fork that has had some recent activity, so I've filed my pull request there as well.

I tested krata-tokio-tar, and in ten attempts I observed flaky test failures on at least x86_64, ppc64le, aarch64, and s390x, so it looks like it also suffers from similar problems.

@musicinmybrain
Copy link
Contributor

It turns out that flaky test failures, particularly insert_local_file_different_name, may occur on any architecture, including x86_64. I was able to reproduce this with just

$ gh repo clone vorot93/tokio-tar
$ cd tokio-tar
$ for i in $(seq 1 1000); do cargo test || (echo "FAILED ON ITERATION $i"; sleep 3600); done

This failed on iteration 82. When I tried cargo test --release, it failed on iteration 25.

@mgorny
Copy link
Contributor Author

mgorny commented May 22, 2024

Could you perhaps also try async-tar? Apparently tokio-tar was forked from that but it had a few changes since, and I don't know if they were forward-ported to tokio-tar.

@charliermarsh
Copy link
Member

We can't really use async-tar because it's built on async-std and we're using tokio.

@musicinmybrain
Copy link
Contributor

I haven’t attempted to build an RPM package for async-tar or test it on architectures other than x86_64 (especially in light of @charliermarsh’s comment), but I did try repeatedly running cargo test, with and without --release, as in #3423 (comment), and I haven’t managed to get it to fail so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream An upstream dependency is involved
Projects
None yet
Development

No branches or pull requests

4 participants