astral-sh / uv

An extremely fast Python package and project manager, written in Rust.
https://docs.astral.sh/uv
Apache License 2.0
21.98k stars 642 forks source link

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

Closed mgorny closed 3 weeks ago

mgorny commented 5 months ago

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

https://github.com/astral-sh/uv/blob/8e86cd0c7385643b9c017bb25adbd4f5ca895b37/Cargo.toml#L132

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 commented 4 months ago

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 commented 4 months ago

Seems reasonable to switch to a maintained repository.

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

mgorny commented 4 months ago

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 commented 4 months ago

I’m trying to build a Fedora package rust-tokio-tar using https://github.com/edera-dev/tokio-tar/pull/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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

musicinmybrain commented 4 months ago

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 https://github.com/astral-sh/uv/issues/3423#issuecomment-2124671700, and I haven’t managed to get it to fail so far.