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

tests: update nix dependency to 0.28 #6474

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

fkm3
Copy link

@fkm3 fkm3 commented Apr 9, 2024

nix::unistd::write now requies an trait AsFd, which RawFd does not implement, so, the struct FileDescriptor field has been switched to OwnedFd. An alternative solution is to use BorrowedFd::borrow_raw, but that requires unsafe code.

`nix::unistd::write` now requies an `trait AsFd`, which `RawFd` does not
implement, so, the `struct FileDescriptor` field has been switched to
`OwnedFd`. An alternative solution is to use `BorrowedFd::borrow_raw`,
but that requires unsafe code.
@fkm3
Copy link
Author

fkm3 commented Apr 10, 2024

Not sure how these changes could cause that FreeBSD build failure

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io M-net Module: tokio/net labels Apr 10, 2024
@Darksonn

This comment was marked as outdated.

@Darksonn
Copy link
Contributor

Perhaps we just need to enable the aio crate feature?

@Darksonn
Copy link
Contributor

After adding the feature, there are some new compilation failures:

error[E0599]: the method `register_raw` exists for struct `Source<AioFsync>`, but its trait bounds were not satisfied
   --> tokio/tests/io_poll_aio.rs:24:20
    |
24  |             self.0.register_raw(kq, token)
    |                    ^^^^^^^^^^^^ method cannot be called on `Source<AioFsync>` due to unsatisfied trait bounds
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-aio-0.8.0/src/aio.rs:80:1
    |
80  | pub struct Source<T>{inner: T}
    | -------------------- doesn't satisfy `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ------------------- doesn't satisfy `AioFsync: nix::sys::aio::Aio`
    |
    = note: the following trait bounds were not satisfied:
            `AioFsync: nix::sys::aio::Aio`
            which is required by `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
error[E0599]: the method `deregister_raw` exists for struct `Source<AioFsync>`, but its trait bounds were not satisfied
   --> tokio/tests/io_poll_aio.rs:27:20
    |
27  |             self.0.deregister_raw()
    |                    ^^^^^^^^^^^^^^ method cannot be called on `Source<AioFsync>` due to unsatisfied trait bounds
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-aio-0.8.0/src/aio.rs:80:1
    |
80  | pub struct Source<T>{inner: T}
    | -------------------- doesn't satisfy `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ------------------- doesn't satisfy `AioFsync: nix::sys::aio::Aio`
    |
    = note: the following trait bounds were not satisfied:
            `AioFsync: nix::sys::aio::Aio`
            which is required by `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
error[E0599]: the method `submit` exists for struct `Pin<&mut Source<AioFsync>>`, but its trait bounds were not satisfied
   --> tokio/tests/io_poll_aio.rs:37:21
    |
37  |             match p.submit() {
    |                     ^^^^^^ method cannot be called on `Pin<&mut Source<AioFsync>>` due to unsatisfied trait bounds
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-aio-0.8.0/src/aio.rs:80:1
    |
80  | pub struct Source<T>{inner: T}
    | -------------------- doesn't satisfy `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ------------------- doesn't satisfy `AioFsync: nix::sys::aio::Aio`
    |
    = note: the following trait bounds were not satisfied:
            `AioFsync: nix::sys::aio::Aio`
            which is required by `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
error[E0599]: the method `aio_return` exists for struct `Pin<&mut Source<AioFsync>>`, but its trait bounds were not satisfied
   --> tokio/tests/io_poll_aio.rs:56:36
    |
56  |                     let result = p.aio_return();
    |                                    ^^^^^^^^^^ method cannot be called on `Pin<&mut Source<AioFsync>>` due to unsatisfied trait bounds
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/mio-aio-0.8.0/src/aio.rs:80:1
    |
80  | pub struct Source<T>{inner: T}
    | -------------------- doesn't satisfy `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
    |
   ::: /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ------------------- doesn't satisfy `AioFsync: nix::sys::aio::Aio`
    |
    = note: the following trait bounds were not satisfied:
            `AioFsync: nix::sys::aio::Aio`
            which is required by `mio_aio::Source<AioFsync>: mio_aio::SourceApi`
error[E0308]: mismatched types
   --> tokio/tests/io_poll_aio.rs:139:34
    |
139 |         let source = TokioSource(mio_aio::Fsync::fsync(fd, mode, 0));
    |                      ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `AioFsync`, found `nix::sys::aio::AioFsync`
    |                      |
    |                      arguments to this struct are incorrect
    |
    = note: `nix::sys::aio::AioFsync` and `AioFsync` have similar names, but are actually distinct types
note: `nix::sys::aio::AioFsync` is defined in crate `nix`
   --> /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.27.1/src/sys/aio.rs:432:1
    |
432 | pub struct AioFsync {
    | ^^^^^^^^^^^^^^^^^^^
note: `AioFsync` is defined in crate `nix`
   --> /.cargo/registry/src/index.crates.io-6f17d22bba15001f/nix-0.28.0/src/sys/aio.rs:429:1
    |
429 | pub struct AioFsync {
    | ^^^^^^^^^^^^^^^^^^^
    = note: perhaps two different versions of crate `nix` are being used?
note: tuple struct defined here
   --> tokio/tests/io_poll_aio.rs:20:12
    |
20  |     struct TokioSource(mio_aio::Source<nix::sys::aio::AioFsync>);
    |            ^^^^^^^^^^^
error: unused import: `SourceApi`
 --> tokio/tests/io_poll_aio.rs:4:29
  |
4 | use mio_aio::{AioFsyncMode, SourceApi};
  |                             ^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
  = help: to override `-D warnings` add `#[allow(unused_imports)]`
Some errors have detailed explanations: E0308, E0599.
For more information about an error, try `rustc --explain E0308`.
error: could not compile `tokio` (test "io_poll_aio") due to 6 previous errors
warning: build failed, waiting for other jobs to finish...

@taiki-e
Copy link
Member

taiki-e commented Apr 10, 2024

We need to update nix used in mio-aio first. (it also enables aio feature of nix)

@asomers
Copy link
Contributor

asomers commented Apr 10, 2024

What's the motivation to update nix?

@fkm3
Copy link
Author

fkm3 commented Apr 10, 2024

What's the motivation to update nix?

I want to update the version of nix used in the Android Open Source Project and we try to keep only one version of each crate, so it would be convenient to update tokio to match versions.

@asomers
Copy link
Contributor

asomers commented Apr 10, 2024

What's the motivation to update nix?

I want to update the version of nix used in the Android Open Source Project and we try to keep only one version of each crate, so it would be convenient to update tokio to match versions.

The major difference between nix 0.27 and 0.28 is I/O safety. I've previously broached that topic with Tokio maintainers and they weren't interested, for good reason. So I think Tokio is best sticking with nix 0.27.

@Darksonn
Copy link
Contributor

Our MSRV is now 1.63, so I don't think I/O safety traits are an issue anymore.

But I'm not sure it matters so much for nix, since we only use it in tests.

@fkm3
Copy link
Author

fkm3 commented Apr 10, 2024

I wonder if it would make sense for the tokio tests to use the libc crate directly instead. There are also some API doc examples using nix though.

@asomers
Copy link
Contributor

asomers commented Apr 10, 2024

The big problem with I/O safety is lifetimes. Tokio programs need to create lots of futures with the lifetime of a file. And nontrivial Tokio programs usually need those lifetimes to be 'static. No problem if files are RawFd. But AsFd files aren't 'static. So it's hard to use them with Tokio.

@fkm3
Copy link
Author

fkm3 commented Apr 10, 2024

You can always just convert the RawFd to a BorrowedFd<'_> on the spot. It requires some unsafe, but it isn't really less safe than using the RawFds directly.

@asomers
Copy link
Contributor

asomers commented Apr 10, 2024

You can always just convert the RawFd to a BorrowedFd<'_> on the spot. It requires some unsafe, but it isn't really less safe than using the RawFds directly.

Yeah, but if you're going to do that, then what's the point of using I/O safety at all?

@Darksonn
Copy link
Contributor

I would generally prefer to not be stuck on an old version of a dependency forever, even if it's test-only. If avoiding that requires RawFd to BorrowedFd<'_> conversions, then so be it.

@asomers
Copy link
Contributor

asomers commented Apr 10, 2024

I would generally prefer to not be stuck on an old version of a dependency forever, even if it's test-only. If avoiding that requires RawFd to BorrowedFd<'_> conversions, then so be it.

In that case we can do it. But as @taiki-e said, we'll have to update mio-aio first. I'm probably the only one who can do that, and i'll need to ensure that it can still work with all downstream dependencies. BUT ATM I'm hobbled up with a broken arm so it might take me a few weeks.

@Darksonn
Copy link
Contributor

If you're willing to do the mio-aio upgrade, then I would definitely appreciate it! There's no rush from my side, and a few weeks is not a problem at all.

@fkm3 We can carry a patch in Android's vendored version of Tokio for now.

@fkm3
Copy link
Author

fkm3 commented Apr 10, 2024

Yeah, but if you're going to do that, then what's the point of using I/O safety at all?

It is the same argument for most other unsafe APIs: we want people to prefer the safe technique and make it obvious they are doing something "unsafe" that can result in bugs and security vulnerabilities. Making syscalls on a closed FD can be just as bad as a use-after-free bug, the FD number might have been reused already. I don't know how relevant that is to mio-aio's usage.

@fkm3 We can carry a patch in Android's vendored version of Tokio for now.

Sounds good. Thanks both of you

@Darksonn Darksonn marked this pull request as draft April 10, 2024 20:13
@Darksonn Darksonn added the S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. label Apr 10, 2024
@Darksonn
Copy link
Contributor

Marking this PR as blocked until mio-aio is upgraded.

jrguzman-ms pushed a commit to msft-mirror-aosp/platform.external.rust.crates.tokio that referenced this pull request Apr 12, 2024
Not possible to make this change upstream yet, see
tokio-rs/tokio#6474.

Bug: 333427576
Test: TreeHugger
Change-Id: I950e3e7efc41e0e8e23db373ff918d11a4362359
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-io Module: tokio/io M-net Module: tokio/net S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants