Use feature for io-uring instead of cfg

[Sytten]

Motivation

This was to remove the lockfile problem described in #7546

Solution

Moving away from cfg and into a feature that is only available when unstable is enabled.
It is a PITA for the CI I will admit, but this solution could also be applied to backtrace

[mox692]

Regarding the error that io-uring crate is emitting, io-uring crate does not provide default bindings on architectures other than those explicitly listed here. The cross-tests that are currently failing in CI include 32-bit architectures that io-uring crate does not support, so some adjustments seem necessary.

I see two possible options:

1. Exclude the io-uring feature in cross-tests where the crate is not supported.
   • This is essentially the same approach as the current master branch. In cross-tests, the tokio_uring cfg is not used. To achieve the same effect with features, instead of --all-features, we would use --features "full,test-util" (excluding io-uring). The downside is that we cannot make the build fail with compile_error! on unsupported architectures.
2. Explicitly add supported architectures to the cfg.
   • Currently we do not, but it is possible to make io-uring available only under an explicit set of targets (perhaps this would be something like not(any(target_arch = "x86_64", target_arch = "aarch64", target_arch = "riscv64", target_arch = "loongarch64", target_arch = "powerpc64"))). This approach is more explicit and makes unsupported builds fail properly, but it has the cost of adding more complex cfg conditions in the codebase.

I'm somewhat inclined to option 1 because:

• Almost all major 64-bit architectures are supported, so practical issues should be minimal.
• Option 2 might become outdated once io-uring expands its arch support in the future
• Users can still use io_uring_use_own_sys as a workaround even if they're trying to use io-uring for unsupported arch.

@Darksonn @ADD-SP
Do you have any thoughts on this?

[ADD-SP]

@mox692 I prefer the option 1, especially because "Almost all major 64-bit architectures are supported".

The downside is that we cannot make the build fail with compile_error! on unsupported architectures.

I don't fully understand, let's say I'm trying to locally compile tokio with io-uring enabled on 32-bit architectures, shouldn't the io-uring crate emit the compilation error? Or did I mis-understand this sentence?

[mox692]

I don't fully understand let's say I'm trying to locally compile tokio with io-uring enabled on 32-bit architectures, shouldn't the io-uring crate emit the compilation error? Or did I mis-understand this sentence?

Ah you're right, I think io-uring will emit an error anyway.

[Darksonn]

Yes, let's exclude it in cross-tests for now.

[Sytten]

Trying another run, I expect we will still have some issues but should be better.

[Sytten]

I think we are good! @ADD-SP @Darksonn

[ADD-SP]

I'd like to express concern about the increasing complexity, this PR introduces a new feature flag under the --cfg tokio_unstable?

The discussion is about whether to test both with io-uring and without io-uring. If we do, shall we also test all possible combinations for the three (tracing, taskdump and io-uring) unstable feature flags? Or we just add combinations for io-uring?

See #7547 (comment) for more context.

[mox692]

A couple of nits.

[Sytten]

There something weird with cross-check-tier3, it should fail for the io-uring feature since I dont think it is supported on those targets but it doesnt?

[mox692]

There something weird with cross-check-tier3, it should fail for the io-uring feature since I dont think it is supported on those targets but it doesnt?

That sounds wiered. I'll try to figure out what's happening.

[mox692]

It looks like this is expected behavior. Tier 3 targets in ci are not linux, so io-uring crate isn't actually pulled in.

# This won't throw an error since this tier 3 target `armv7-sony-vita-newlibeabihf` isn't Linux, so the `io-uring` crate isn't even pulled in.
$ RUSTFLAGS="--cfg tokio_unstable" cargo build --features io-uring --target armv7-sony-vita-newlibeabihf

# This will throw an compile error from `io-uring` crate since it's linux but not supported arch, i686.
$ RUSTFLAGS="--cfg tokio_unstable" cargo build --features io-uring --target i686-unknown-linux-gnu

[Sytten]

Make sense @mox692 . I reckon it is ready now. Let me know if there is any change necessary.

[mox692]

Looks good to me. I'll leave this a little while in case other members still have opinions for this.

[ADD-SP]

Overall looks good to me.

[ADD-SP]

Can we also add a short comment to explain why did we use --features full,test-util instead of --all-features?

[ADD-SP]

Why did we need to add test-util here?

[ADD-SP]

Can we also add a short comment to explain why did we use --features full,test-util instead of --all-features?

[ADD-SP]

Can we add a short comment about --skip io-uring?

[ADD-SP]

Suggested change

	// if user enables `io-uring`. It is blocked by:
	// if user enables `io-uring` cargo feature. It is blocked by:

nit

[mox692]

@Sytten
Are you still working on this? I can push some commits to address the comments above, if you'd like.

This PR will be kind of important to define how users can opt in io_uring in the next release. We generally don't want to change the way to opt-in once we ship a new feature, even if it's unstable.

[Sytten]

@mox692 feel free to do so. I was too busy this week. I might have time tonight, but it is a bit complicated right now

[mox692]

It looks like I can’t edit this branch, so I opened another PR #7621.

[ADD-SP]

Superseded by #7621 (derived from this PR), thanks for your contribution!