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

ci: use --feature-powerset --depth 2 in features check #5007

Merged
merged 7 commits into from
Sep 13, 2022
Merged

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Sep 13, 2022

Motivation

As has been pointed out a few times in the past (e.g., #4036 (comment)), each-feature is not sufficient for features check.

Ideally, we'd like to check all combinations of features, but there are too many combinations.
So limit the max number of simultaneous feature flags to 2 by --depth option. I think this should be sufficient in most cases as @carllerche said in taiki-e/cargo-hack#58.

@taiki-e taiki-e requested a review from Darksonn September 13, 2022 12:18
```
info: running `cargo check -Z avoid-dev-deps --no-default-features --features net,rt` on tokio (38/225)
   Compiling tokio v1.21.1 (/home/runner/work/tokio/tokio/tokio)
error: associated function `shutdown` is never used
  --> tokio/src/runtime/driver.rs:65:23
   |
65 |         pub(crate) fn shutdown(&mut self) {
   |                       ^^^^^^^^
   |
   = note: `-D dead-code` implied by `-D warnings`
```
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Sep 13, 2022
@Darksonn Darksonn added the A-ci Area: The continuous integration setup label Sep 13, 2022
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot removed the R-loom Run loom tests on this PR label Sep 13, 2022
This warning happened in the following cases.

```
failed commands:
    tokio:
        `/home/runner/.rustup/toolchains/nightly-2022-07-26-x86_64-unknown-linux-gnu/bin/cargo check -Z avoid-dev-deps --manifest-path tokio/Cargo.toml --no-default-features --features net,rt`
        `/home/runner/.rustup/toolchains/nightly-2022-07-26-x86_64-unknown-linux-gnu/bin/cargo check -Z avoid-dev-deps --manifest-path tokio/Cargo.toml --no-default-features --features process,rt`
        `/home/runner/.rustup/toolchains/nightly-2022-07-26-x86_64-unknown-linux-gnu/bin/cargo check -Z avoid-dev-deps --manifest-path tokio/Cargo.toml --no-default-features --features rt,signal`
        `/home/runner/.rustup/toolchains/nightly-2022-07-26-x86_64-unknown-linux-gnu/bin/cargo check -Z avoid-dev-deps --manifest-path tokio/Cargo.toml --no-default-features --features rt-multi-thread,signal`
        `/home/runner/.rustup/toolchains/nightly-2022-07-26-x86_64-unknown-linux-gnu/bin/cargo check -Z avoid-dev-deps --manifest-path tokio/Cargo.toml --no-default-features --features signal,test-util`
    tokio-util:
        `/home/runner/.rustup/toolchains/nightly-2022-07-26-x86_64-unknown-linux-gnu/bin/cargo check -Z avoid-dev-deps --manifest-path tokio-util/Cargo.toml --no-default-features --features io-util,net`
        `/home/runner/.rustup/toolchains/nightly-2022-07-26-x86_64-unknown-linux-gnu/bin/cargo check -Z avoid-dev-deps --manifest-path tokio-util/Cargo.toml --no-default-features --features net,rt`
```
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Sep 13, 2022
@taiki-e
Copy link
Member Author

taiki-e commented Sep 13, 2022

hmm...

info: running `cargo check -Z avoid-dev-deps --no-default-features --features rt,signal` on tokio (56/225)
   Compiling tokio v1.21.1 (/home/runner/work/tokio/tokio/tokio)
error[E0308]: mismatched types
  --> tokio/src/runtime/driver.rs:46:58
   |
46 |                 IoStack::Enabled(v) => IoUnpark::Enabled(v.handle()),
   |                                        ----------------- ^^^^^^^^^^ expected struct `runtime::io::Handle`, found struct `signal::unix::driver::Handle`
   |                                        |
   |                                        arguments to this enum variant are incorrect
   |
note: tuple variant defined here
  --> tokio/src/runtime/driver.rs:20:9
   |
20 |         Enabled(crate::runtime::io::Handle),
   |         ^^^^^^^

For more information about this error, try `rustc --explain E0308`.
error: could not compile `tokio` due to previous error
error: process didn't exit successfully: `/home/runner/.rustup/toolchains/nightly-2022-07-26-x86_64-unknown-linux-gnu/bin/cargo check -Z avoid-dev-deps --manifest-path tokio/Cargo.toml --no-default-features --features rt,signal` (exit status: 101)

@taiki-e
Copy link
Member Author

taiki-e commented Sep 13, 2022

The problem seems to be that the type returned by the handle method of the SignalDriver (used as ProcessDriver when the process feature is disabled) is different from that of ProcessDriver.

// ===== process driver =====
cfg_process_driver! {
type ProcessDriver = crate::process::unix::driver::Driver;
fn create_process_driver(signal_driver: SignalDriver) -> ProcessDriver {
crate::process::unix::driver::Driver::new(signal_driver)
}
}
cfg_not_process_driver! {
cfg_io_driver! {
type ProcessDriver = SignalDriver;
fn create_process_driver(signal_driver: SignalDriver) -> ProcessDriver {
signal_driver
}
}
}

However, crate::signal::unix::driver::Handle that returned by SignalDriver::handle doesn't have unpark method, so the following simple patch doesn't seem to work.

diff --git a/tokio/src/runtime/driver.rs b/tokio/src/runtime/driver.rs
index 2a0deb80..6ef98825 100644
--- a/tokio/src/runtime/driver.rs
+++ b/tokio/src/runtime/driver.rs
@@ -17,7 +17,7 @@ pub(crate) enum IoStack {
     }
 
     pub(crate) enum IoUnpark {
-        Enabled(crate::runtime::io::Handle),
+        Enabled(ProcessHandle),
         Disabled(UnparkThread),
     }
 
@@ -127,6 +127,7 @@ fn create_signal_driver(io_driver: IoDriver) -> io::Result<(SignalDriver, Signal
 
 cfg_process_driver! {
     type ProcessDriver = crate::process::unix::driver::Driver;
+    type ProcessHandle = crate::runtime::io::Handle;
 
     fn create_process_driver(signal_driver: SignalDriver) -> ProcessDriver {
         crate::process::unix::driver::Driver::new(signal_driver)
@@ -136,6 +137,7 @@ fn create_process_driver(signal_driver: SignalDriver) -> ProcessDriver {
 cfg_not_process_driver! {
     cfg_io_driver! {
         type ProcessDriver = SignalDriver;
+        type ProcessHandle = crate::signal::unix::driver::Handle;
 
         fn create_process_driver(signal_driver: SignalDriver) -> ProcessDriver {
             signal_driver
$ cargo check -Z avoid-dev-deps --manifest-path tokio/Cargo.toml --no-default-features --features rt,signal
error[E0599]: no method named `unpark` found for reference `&signal::unix::driver::Handle` in the current scope
  --> tokio/src/runtime/driver.rs:77:43
   |
77 |                 IoUnpark::Enabled(v) => v.unpark(),
   |                                           ^^^^^^ method not found in `&signal::unix::driver::Handle`

For more information about this error, try `rustc --explain E0599`.

@taiki-e
Copy link
Member Author

taiki-e commented Sep 13, 2022

cc @ipetkov

@carllerche
Copy link
Member

Seems like we do need this!

@carllerche
Copy link
Member

@taiki-e I can work on fixing this build error since it is from my work.

@carllerche
Copy link
Member

This build error happened because I incorrectly combined "handle" and "unpark" when I got rid of the Park trait. I will keep them separate for now to fix the build.

@carllerche carllerche removed the R-loom Run loom tests on this PR label Sep 13, 2022
@carllerche carllerche merged commit b891714 into master Sep 13, 2022
@carllerche carllerche deleted the taiki-e/ci branch September 13, 2022 18:05
@carllerche
Copy link
Member

Thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ci Area: The continuous integration setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants