chore: check each feature works properly #1695

taiki-e · 2019-10-26T15:52:46Z

The current tests for each feature gate do not work properly because the default feature is enabled by dev-dependencies.

The cause of this is rust-lang/cargo#4866, but it seems difficult to fix on the cargo side.
~~So remove all dev-dependencies in CI to avoid this issue. This is basically the same as rust-lang/futures-rs#1456 (comment).~~ EDIT2 see #1695 (comment)

~~Also, this change test to run only once with all-features instead of running tests for each feature.~~ EDIT: see #1695 (comment)

Closes #1693

Refs:

taiki-e · 2019-10-26T15:59:18Z

Also, this change test to run only once with all-features instead of running tests for each feature.

Oh, this is not good. It needs to keep the existing one for tests-build.

EDIT: fixed in 3eebedc

taiki-e · 2019-10-26T16:56:44Z

hmm...

error[E0432]: unresolved import `tokio_sync`
 --> tokio/src/net/driver/reactor.rs:4:5
  |
4 | use tokio_sync::AtomicWaker;
  |     ^^^^^^^^^^ use of undeclared type or module `tokio_sync`

error[E0432]: unresolved import `slab`
 --> tokio/src/net/driver/reactor.rs:7:5
  |
7 | use slab::Slab;
  |     ^^^^ help: a similar path exists: `mio::slab`

error[E0432]: unresolved import `crate::io`
 --> tokio/src/net/util/poll_evented.rs:1:12
  |
1 | use crate::io::{AsyncRead, AsyncWrite};
  |            ^^
  |            |
  |            unresolved import
  |            help: a similar path exists: `std::io`

carllerche · 2019-10-26T19:20:58Z

Thanks. I haven't reviewed yet. However, I want to quickly point out that tests-build exists to work around the cargo limitations and test the various cargo feature combos. These tests have gone out of date w/ the recent churn though.

taiki-e · 2019-10-26T19:54:26Z

However, I want to quickly point out that tests-build exists to work around the cargo limitations and test the various cargo feature combos. These tests have gone out of date w/ the recent churn though.

tests-build's approach requires all tests to be written manually. Also, we need to write tests for each platform for some crates (see 7aee1f8).

But I'd like to try tests-build as another pr because it seems help reduce CI time.

carllerche · 2019-10-26T20:02:21Z

@taiki-e I wasn't making a comment on your PR yet, just providing context :) I will probably review on monday.

taiki-e · 2019-10-27T16:59:48Z

UPDATE: It is very hard to maintain these tests manually, so I'm writing a tool to make these checks run in one line :)

carllerche · 2019-10-28T16:55:51Z

👍 Should I merge this then or wait?

carllerche · 2019-10-28T17:00:43Z

Also, is the plan to get rid of tests-build or have something else with it?

It is hard to maintain features list manually, so use cargo-hack's `--each-feature` flag. And cargo-hack provides a workaround for an issue that dev-dependencies leaking into normal build (`--no-dev-deps` flag), so removed own ci tool. Also, compared to running tests on all features, there is not much advantage in running tests on each feature, so only the default features and all features are tested. If the behavior changes depending on the feature, we need to test it as another job in CI.

this is no longer necessary

It seems rustfmt does not properly format the module in the macro.

taiki-e · 2019-10-30T12:24:16Z

I wrote https://github.com/taiki-e/cargo-hack to improve feature flag testing.
cargo-hack is basically wrapper of cargo that propagates subcommand and provides additional flags to avoid some of the limitations of cargo. e.g.:

--each-feature - run for each feature which includes default features and --no-default-features (if the crate has one or more feature) of package
--no-dev-deps - run without dev-dependencies to avoid build-dependencies and dependencies should not have features unified rust-lang/cargo#4866

By using these two features, you can easily test the feature flag gate:

$ cargo hack check --each-feature --no-dev-deps

output:

("info: running cargo check ... on tokio" shows what command is actually being executed.)

info: running `cargo check` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 23.38s
info: running `cargo check --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
info: running `cargo check --features=executor-core --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 0.31s
info: running `cargo check --features=blocking --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 1.13s
info: running `cargo check --features=fs --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 1.67s
info: running `cargo check --features=io-traits --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 0.24s
info: running `cargo check --features=io-util --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 0.79s
info: running `cargo check --features=io --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 0.81s
info: running `cargo check --features=macros --no-default-features` on tokio
   Compiling proc-macro2 v1.0.6
   Compiling syn v1.0.7
   Compiling quote v1.0.2
   Compiling tokio-macros v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio-macros)
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 12.47s
info: running `cargo check --features=net-full --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 2.76s
info: running `cargo check --features=net-driver --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 1.73s
info: running `cargo check --features=rt-current-thread --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 2.43s
info: running `cargo check --features=rt-full --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 4.50s
info: running `cargo check --features=signal --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 1.79s
info: running `cargo check --features=sync --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 0.94s
info: running `cargo check --features=tcp --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 2.45s
info: running `cargo check --features=timer --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 1.60s
info: running `cargo check --features=udp --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 2.41s
info: running `cargo check --features=uds --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 2.53s
info: running `cargo check --features=process --no-default-features` on tokio
    Checking tokio v0.2.0-alpha.6 (/home/vsts/work/1/s/tokio)
    Finished dev [unoptimized + debuginfo] target(s) in 2.70s

This will remove the need to manually list feature flags and tests-build should no longer be needed.

This reverts commit 519fc6f.

Many features of current cargo-hack are not available on windows. Refs: taiki-e/cargo-hack#3

taiki-e · 2019-10-30T12:57:52Z

azure-pipelines.yml

-      examples: []
-
-# Test compilation failure
-# Disable pending: https://github.com/tokio-rs/tokio/pull/1695#issuecomment-547045383


taiki-e · 2019-10-30T13:00:56Z

ci/azure-test-stable.yml

+      displayName: ${{ crate }} - cargo hack check --each-feature
+      workingDirectory: $(Build.SourcesDirectory)/${{ crate }}
+      # FIXME(taiki-e): many features of current cargo-hack are not available on windows: https://github.com/taiki-e/cargo-hack/issues/3
+      condition: not(eq(variables['Agent.OS'], 'Windows_NT'))


Unfortunately, many features of current cargo-hack are not available on windows. (taiki-e/cargo-hack#3)

However, it seems many features are not directly related to OS-specific dependencies (7aee1f8), so for now, I think this is almost all right.

I tried to fix this in taiki-e/cargo-hack#6, but I couldn't fix it completely.

taiki-e · 2019-10-30T13:44:15Z

BTW, another benefit is that CI time is significantly reduced. (see also 6bee4d8)

this - windows/linux: 13m-14m macos: 11m-12m (https://dev.azure.com/tokio-rs/Tokio/_build/results?buildId=3030)
previous - windows/linux: 40m-50m macos: 30m-40m (https://dev.azure.com/tokio-rs/Tokio/_build/results?buildId=3020)

taiki-e · 2019-10-31T01:18:53Z

@carllerche Updated PR to use cargo-hack. Could you review it again?

carllerche

👍 Thanks. We can try it and see how it goes.

I will probably bring back tests-build at some point so we can check that types exist or don't exist w/ various feature flags.

hawkw

This seems good to me.

IMO, the CI build is quite complex and it would be good to have some more documentation of what it's actually doing. I don't think that's a blocker, though.

hawkw · 2019-10-31T23:31:21Z

ci/azure-test-stable.yml

@@ -20,6 +20,11 @@ jobs:
      # rust_version: stable
      rust_version: ${{ parameters.rust }}

+  - script: cargo install cargo-hack


This is nitpicky, but it might be helpful to have a comment here explaining what we're using cargo-hack for and why?

I wrote descriptions for 'check' scripts, but if necessary, I'd happy to add a description to the 'install' script.

tokio/ci/azure-test-stable.yml

Lines 47 to 50 in 02f7264

# Check each specified feature works properly

# * --each-feature - run for each feature which includes --no-default-features and default features of package

# * --no-dev-deps - build without dev-dependencies to avoid https://github.com/rust-lang/cargo/issues/4866

- script: cargo hack check --each-feature --no-dev-deps

tokio/ci/azure-test-stable.yml

Lines 78 to 81 in 02f7264

# Check each specified feature works properly

# * --each-feature - run for each feature which includes --no-default-features and default features of package

# * --no-dev-deps - build without dev-dependencies to avoid https://github.com/rust-lang/cargo/issues/4866

- script: cargo hack check --each-feature --no-dev-deps

taiki-e · 2019-11-01T04:48:54Z

@carllerche @hawkw Thanks for the review!

chore: check each feature works properly

6bb281b

revert all-features test

3eebedc

taiki-e added 3 commits October 27, 2019 01:57

fix build fail with net-driver feature

775ffe4

fix build fail with timer feature

94a39da

use bash on windows

6d4650e

fix build fail with signal feature on windows

7aee1f8

carllerche approved these changes Oct 28, 2019

View reviewed changes

taiki-e added 8 commits October 30, 2019 16:59

Merge branch 'master' into dev-deps

d625c15

tokio: fix blocking and fs features

f09653b

tokio: fix net-driver and rt-current-thread features

4eb745e

ci: remove tests-build

8ea0ebd

this is no longer necessary

fix ci

7a05995

format with rustfmt

0653ca3

It seems rustfmt does not properly format the module in the macro.

try to use bash on windows

519fc6f

taiki-e added 2 commits October 30, 2019 21:28

Revert "try to use bash on windows"

a7c13b7

This reverts commit 519fc6f.

disable cargo hack check on on windows

3bfb32f

Many features of current cargo-hack are not available on windows. Refs: taiki-e/cargo-hack#3

taiki-e mentioned this pull request Oct 30, 2019

executor: move into tokio crate #1702

Merged

taiki-e commented Oct 30, 2019

View reviewed changes

use released cargo-hack

6ee97be

Merge branch 'master' into dev-deps

3d0da91

taiki-e requested a review from carllerche October 31, 2019 01:18

taiki-e mentioned this pull request Oct 31, 2019

Make Delay must_use #1714

Merged

carllerche approved these changes Oct 31, 2019

View reviewed changes

hawkw approved these changes Oct 31, 2019

View reviewed changes

carllerche merged commit 02f7264 into tokio-rs:master Nov 1, 2019

taiki-e deleted the dev-deps branch November 1, 2019 04:30

This was referenced Nov 17, 2019

Cleanup CI config rust-lang/futures-rs#1980

Merged

chore: enable feature flag check on windows #1798

Merged

taiki-e mentioned this pull request Dec 29, 2021

Add resolver = "2" to root Cargo.toml #4357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: check each feature works properly #1695

chore: check each feature works properly #1695

taiki-e commented Oct 26, 2019 •

edited

Loading

taiki-e commented Oct 26, 2019 •

edited

Loading

taiki-e commented Oct 26, 2019

carllerche commented Oct 26, 2019

taiki-e commented Oct 26, 2019

carllerche commented Oct 26, 2019

taiki-e commented Oct 27, 2019

carllerche commented Oct 28, 2019

carllerche commented Oct 28, 2019

taiki-e commented Oct 30, 2019

taiki-e Oct 30, 2019

taiki-e Oct 30, 2019

taiki-e Oct 30, 2019

taiki-e Oct 31, 2019

taiki-e commented Oct 30, 2019 •

edited

Loading

taiki-e commented Oct 31, 2019

carllerche left a comment

hawkw left a comment

hawkw Oct 31, 2019

taiki-e Nov 1, 2019

taiki-e commented Nov 1, 2019

	# Check each specified feature works properly
	# * --each-feature - run for each feature which includes --no-default-features and default features of package
	# * --no-dev-deps - build without dev-dependencies to avoid https://github.com/rust-lang/cargo/issues/4866
	- script: cargo hack check --each-feature --no-dev-deps

chore: check each feature works properly #1695

chore: check each feature works properly #1695

Conversation

taiki-e commented Oct 26, 2019 • edited Loading

taiki-e commented Oct 26, 2019 • edited Loading

taiki-e commented Oct 26, 2019

carllerche commented Oct 26, 2019

taiki-e commented Oct 26, 2019

carllerche commented Oct 26, 2019

taiki-e commented Oct 27, 2019

carllerche commented Oct 28, 2019

carllerche commented Oct 28, 2019

taiki-e commented Oct 30, 2019

taiki-e Oct 30, 2019

Choose a reason for hiding this comment

taiki-e Oct 30, 2019

Choose a reason for hiding this comment

taiki-e Oct 30, 2019

Choose a reason for hiding this comment

taiki-e Oct 31, 2019

Choose a reason for hiding this comment

taiki-e commented Oct 30, 2019 • edited Loading

taiki-e commented Oct 31, 2019

carllerche left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Oct 31, 2019

Choose a reason for hiding this comment

taiki-e Nov 1, 2019

Choose a reason for hiding this comment

taiki-e commented Nov 1, 2019

taiki-e commented Oct 26, 2019 •

edited

Loading

taiki-e commented Oct 26, 2019 •

edited

Loading

taiki-e commented Oct 30, 2019 •

edited

Loading