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

net: be more specific about winapi features #4764

Merged
merged 5 commits into from
Jun 16, 2022
Merged

Conversation

gftea
Copy link
Contributor

@gftea gftea commented Jun 11, 2022

Motivation

be more specific about winapi features (#4737)

Solution

add winapi/std dependency in net feature, remove features in dependencies.winapi.
Below run all OK on my windows 10 PC

cargo build --all-features
cargo check --all-features
cargo test --all-features

@Darksonn Darksonn changed the title net: be more specific about winapi features (#4737) net: be more specific about winapi features Jun 11, 2022
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Jun 12, 2022
@Darksonn
Copy link
Contributor

Darksonn commented Jun 12, 2022

Can you try running this test (from the tokio subdirectory):

cargo hack check --feature-powerset --no-dev-deps

It will compile Tokio with every possible combination of feature flags. I think there are around 2000 combinations, so it might take an hour or two (it doesn't recompile every dependency every time).

I can't run it myself unfortunately as I don't have a windows computer. You will need cargo hack to do it.

@gftea
Copy link
Contributor Author

gftea commented Jun 13, 2022

running it now, while waiting, can you help explain why it works even though we have below imports?
It seems the docsrs is not cfg by default. e.g. handleapi is not explictly added in feature's dependency, why compilation pass?

#[cfg(not(docsrs))]
mod doc {
    pub(super) use crate::os::windows::ffi::OsStrExt;
    pub(super) use crate::winapi::shared::minwindef::{DWORD, FALSE};
    pub(super) use crate::winapi::um::fileapi;
    pub(super) use crate::winapi::um::handleapi;
    pub(super) use crate::winapi::um::namedpipeapi;
    pub(super) use crate::winapi::um::winbase;
    pub(super) use crate::winapi::um::winnt;

    pub(super) use mio::windows as mio_windows;
}

@Darksonn
Copy link
Contributor

Which version of mio are you building with? (This should be visible in your Cargo.lock.)

@Darksonn
Copy link
Contributor

Darksonn commented Jun 13, 2022

Can you give me the output of this command as well?

cargo tree --features full -i winapi -e all

@gftea
Copy link
Contributor Author

gftea commented Jun 13, 2022

Now I hit a error during running cargo hack as below. But why cargo check --all-features pass? Is it because --all-features has includes some default features which also include those module?

info: running `cargo check --no-default-features --features process` on tokio (67/7176)
   Compiling winapi v0.3.9
    Checking tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio)
error[E0432]: unresolved import `winapi::um::handleapi`
  --> tokio\src\process\windows.rs:35:17
   |
35 | use winapi::um::handleapi::{DuplicateHandle, INVALID_HANDLE_VALUE};
   |                 ^^^^^^^^^ could not find `handleapi` in `um`

error[E0432]: unresolved import `winapi::um::processthreadsapi`
  --> tokio\src\process\windows.rs:36:17
   |
36 | use winapi::um::processthreadsapi::GetCurrentProcess;
   |                 ^^^^^^^^^^^^^^^^^ could not find `processthreadsapi` in `um`

error[E0432]: unresolved import `winapi::um::winbase`
  --> tokio\src\process\windows.rs:38:17
   |
38 | use winapi::um::winbase::{RegisterWaitForSingleObject, INFINITE};
   |                 ^^^^^^^ could not find `winbase` in `um`

@gftea
Copy link
Contributor Author

gftea commented Jun 13, 2022

$ cargo tree --features full -i winapi -e all
winapi v0.3.9
└── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio)
    ├── tokio feature "bytes"
    │   ├── tokio feature "io-util"
    │   │   └── tokio feature "full" (command-line)
    │   └── tokio feature "process"
    │       └── tokio feature "full" (command-line)
    ├── tokio feature "default" (command-line)
    │   ├── tokio-stream v0.1.9 (C:\Users\kenny\Documents\repo\tokio\tokio-stream)
    │   │   ├── tokio-stream feature "default"
    │   │   │   └── tokio-test v0.4.2 (C:\Users\kenny\Documents\repo\tokio\tokio-test)
    │   │   │       └── tokio-test feature "default"
    │   │   │           [dev-dependencies]
    │   │   │           └── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio) (*)
    │   │   │   [dev-dependencies]
    │   │   │   └── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio) (*)
    │   │   └── tokio-stream feature "time"
    │   │       └── tokio-stream feature "default" (*)
    │   └── tokio-test v0.4.2 (C:\Users\kenny\Documents\repo\tokio\tokio-test) (*)
    ├── tokio feature "fs"
    │   └── tokio feature "full" (command-line)
    ├── tokio feature "full" (command-line)
    ├── tokio feature "io-std"
    │   └── tokio feature "full" (command-line)
    ├── tokio feature "io-util" (*)
    ├── tokio feature "libc"
    │   ├── tokio feature "net"
    │   │   └── tokio feature "full" (command-line)
    │   ├── tokio feature "process" (*)
    │   └── tokio feature "signal"
    │       └── tokio feature "full" (command-line)
    ├── tokio feature "macros"
    │   └── tokio feature "full" (command-line)
    ├── tokio feature "memchr"
    │   └── tokio feature "io-util" (*)
    ├── tokio feature "mio"
    │   ├── tokio feature "net" (*)
    │   ├── tokio feature "process" (*)
    │   └── tokio feature "signal" (*)
    ├── tokio feature "net" (*)
    ├── tokio feature "num_cpus"
    │   └── tokio feature "rt-multi-thread"
    │       └── tokio feature "full" (command-line)
    ├── tokio feature "once_cell"
    │   ├── tokio feature "process" (*)
    │   ├── tokio feature "rt"
    │   │   └── tokio-test v0.4.2 (C:\Users\kenny\Documents\repo\tokio\tokio-test) (*)
    │   │   ├── tokio feature "full" (command-line)
    │   │   ├── tokio feature "rt-multi-thread" (*)
    │   │   └── tokio feature "test-util"
    │   │       └── tokio-test v0.4.2 (C:\Users\kenny\Documents\repo\tokio\tokio-test) (*)
    │   └── tokio feature "signal" (*)
    ├── tokio feature "parking_lot"
    │   └── tokio feature "full" (command-line)
    ├── tokio feature "process" (*)
    ├── tokio feature "rt" (*)
    ├── tokio feature "rt-multi-thread" (*)
    ├── tokio feature "signal" (*)
    ├── tokio feature "signal-hook-registry"
    │   ├── tokio feature "process" (*)
    │   └── tokio feature "signal" (*)
    ├── tokio feature "socket2"
    │   └── tokio feature "net" (*)
    ├── tokio feature "sync"
    │   ├── tokio-stream v0.1.9 (C:\Users\kenny\Documents\repo\tokio\tokio-stream) (*)
    │   └── tokio-test v0.4.2 (C:\Users\kenny\Documents\repo\tokio\tokio-test) (*)
    │   ├── tokio feature "full" (command-line)
    │   └── tokio feature "test-util" (*)
    ├── tokio feature "test-util" (*)
    ├── tokio feature "time"
    │   └── tokio-test v0.4.2 (C:\Users\kenny\Documents\repo\tokio\tokio-test) (*)
    │   ├── tokio feature "full" (command-line)
    │   ├── tokio feature "test-util" (*)
    │   └── tokio-stream feature "time" (*)
    ├── tokio feature "tokio-macros"
    │   └── tokio feature "macros" (*)
    └── tokio feature "winapi"
        ├── tokio feature "net" (*)
        ├── tokio feature "process" (*)
        └── tokio feature "signal" (*)
├── winapi feature "cfg"
│   └── ntapi v0.3.7
│       ├── ntapi feature "default"
│       │   [dev-dependencies]
│       │   └── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio) (*)
│       └── ntapi feature "user"
│           └── ntapi feature "default" (*)
├── winapi feature "consoleapi"
│   └── tokio feature "signal" (*)
├── winapi feature "default"
│   ├── ntapi v0.3.7 (*)
│   ├── remove_dir_all v0.5.3
│   │   └── remove_dir_all feature "default"
│   │       └── tempfile v3.3.0
│   │           └── tempfile feature "default"
│   │               ├── proptest v1.0.0
│   │               │   ├── proptest feature "bit-set"
│   │               │   │   └── proptest feature "default"
│   │               │   │       [dev-dependencies]
│   │               │   │       └── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio) (*)
│   │               │   ├── proptest feature "break-dead-code"
│   │               │   │   └── proptest feature "default" (*)
│   │               │   ├── proptest feature "default" (*)
│   │               │   ├── proptest feature "fork"
│   │               │   │   ├── proptest feature "default" (*)
│   │               │   │   └── proptest feature "timeout"
│   │               │   │       └── proptest feature "default" (*)
│   │               │   ├── proptest feature "lazy_static"
│   │               │   │   └── proptest feature "std"
│   │               │   │       ├── proptest feature "default" (*)
│   │               │   │       └── proptest feature "fork" (*)
│   │               │   ├── proptest feature "quick-error"
│   │               │   │   └── proptest feature "std" (*)
│   │               │   ├── proptest feature "regex-syntax"
│   │               │   │   └── proptest feature "std" (*)
│   │               │   ├── proptest feature "rusty-fork"
│   │               │   │   ├── proptest feature "fork" (*)
│   │               │   │   └── proptest feature "timeout" (*)
│   │               │   ├── proptest feature "std" (*)
│   │               │   ├── proptest feature "tempfile"
│   │               │   │   └── proptest feature "fork" (*)
│   │               │   └── proptest feature "timeout" (*)
│   │               └── rusty-fork v0.3.0
│   │                   └── proptest v1.0.0 (*)
│   │                   ├── rusty-fork feature "timeout"
│   │                   │   └── proptest feature "timeout" (*)
│   │                   └── rusty-fork feature "wait-timeout"
│   │                       └── rusty-fork feature "timeout" (*)
│   │               [dev-dependencies]
│   │               └── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio) (*)
│   ├── socket2 v0.4.4
│   │   ├── socket2 feature "all"
│   │   │   └── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio) (*)
│   │   └── socket2 feature "default"
│   │       └── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio) (*)
│   │       [dev-dependencies]
│   │       └── tokio v1.19.2 (C:\Users\kenny\Documents\repo\tokio\tokio) (*)
│   └── tempfile v3.3.0 (*)
├── winapi feature "errhandlingapi"
│   └── remove_dir_all v0.5.3 (*)
├── winapi feature "evntrace"
│   └── ntapi v0.3.7 (*)
├── winapi feature "fileapi"
│   ├── remove_dir_all v0.5.3 (*)
│   └── tempfile v3.3.0 (*)
├── winapi feature "handleapi"
│   ├── socket2 v0.4.4 (*)
│   └── tempfile v3.3.0 (*)
├── winapi feature "in6addr"
│   └── ntapi v0.3.7 (*)
├── winapi feature "inaddr"
│   └── ntapi v0.3.7 (*)
├── winapi feature "minwinbase"
│   └── ntapi v0.3.7 (*)
├── winapi feature "namedpipeapi"
│   └── tokio feature "net" (*)
├── winapi feature "ntsecapi"
│   └── ntapi v0.3.7 (*)
├── winapi feature "std"
│   └── remove_dir_all v0.5.3 (*)
│   └── tokio feature "net" (*)
├── winapi feature "threadpoollegacyapiset"
│   └── tokio feature "process" (*)
├── winapi feature "winbase"
│   ├── remove_dir_all v0.5.3 (*)
│   └── tempfile v3.3.0 (*)
├── winapi feature "windef"
│   └── ntapi v0.3.7 (*)
├── winapi feature "winerror"
│   └── remove_dir_all v0.5.3 (*)
├── winapi feature "winioctl"
│   └── ntapi v0.3.7 (*)
├── winapi feature "ws2ipdef"
│   └── socket2 v0.4.4 (*)
└── winapi feature "ws2tcpip"
    └── socket2 v0.4.4 (*)

@gftea
Copy link
Contributor Author

gftea commented Jun 13, 2022

$ grep mio -C 2 ../Cargo.lock

[[package]]
name = "mio"
version = "0.7.14"
source = "registry+https://github.com/rust-lang/crates.io-index"
--
 "libc",
 "log",
 "miow",
 "ntapi",
 "winapi",
--

[[package]]
name = "mio"
version = "0.8.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
--

[[package]]
name = "mio-aio"
version = "0.6.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8a316ed8ed5bc3989660d8333ad91abe9c165f6347aa876cfdfbddb0b906fafc"
dependencies = [
 "mio 0.7.14",
 "nix 0.22.3",
]

[[package]]
name = "miow"
version = "0.3.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
--
 "loom",
 "memchr",
 "mio 0.8.3",
 "mio-aio",
 "mockall",
 "nix 0.24.1",

@Darksonn
Copy link
Contributor

Okay, so let's take handleapi as an example. We see this output:

├── winapi feature "handleapi"
│   ├── socket2 v0.4.4 (*)
│   └── tempfile v3.3.0 (*)

This means that both socket2 and tempfile enable the winapi/handleapi feature. The tempfile crate is only used in tests, so we can ignore it.

Since features are combined from all dependencies, this means that the feature is also available to Tokio if socket2 requires it. However, we only add socket2 when tokio/net is enabled, so if you are compiling with --features process like in the failing example, then we are not enabling net, and therefore socket2 does not get enabled, and therefore we don't get winapi/handleapi available in Tokio.

Furthermore, it is possible that socket2 stops requiring the winapi/handleapi feature in a future release. For example, this could happen if socket2 switches to the windows-sys crate instead. In that case, Tokio would break because we use the handleapi feature, but there is no longer any crates that list the feature as required.

What we are looking for here is that Tokio should list all of the winapi features that Tokio needs such that even if socket2 (or another dependency) switches to windows-sys, this does not result in Tokio breaking. This is what happened in #4662 where the mio crate and socket2 both switched to windows-sys simultaneously. It seems like socket2 has reverted this change since then, but we need to be prepared in case this happens again.

@gftea
Copy link
Contributor Author

gftea commented Jun 13, 2022

@Darksonn thanks! Now I understand better.
by the way there 7176 combination of feature flags when running cargo hack check --feature-powerset --no-dev-deps
Now so far only +50% has been run, taking more quite a long time.

@Darksonn
Copy link
Contributor

Since I saw that cargo tree output, I understand the situation a bit better myself as well. You can stop the cargo hack run. It has become clear that it can't really tell us if we've added the flags everywhere we need them because socket2 already enables most of the features for us.

Can you figure out which winapi features listed in the cargo tree output are used by Tokio, and list them under the appropriate Tokio features?

@gftea
Copy link
Contributor Author

gftea commented Jun 13, 2022

please check latest comitted changes.
I searched all use of winapi::um, and add them to corresponding features

tokio/Cargo.toml Outdated Show resolved Hide resolved
tokio/Cargo.toml Show resolved Hide resolved
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.

I think the below is everything else we are missing.

tokio/Cargo.toml Show resolved Hide resolved
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.

Assuming CI passes, then this looks good to me.

@Darksonn Darksonn enabled auto-merge (squash) June 16, 2022 09:26
@Darksonn Darksonn merged commit 90bc5fa into tokio-rs:master Jun 16, 2022
gftea added a commit to gftea/tokio that referenced this pull request Jun 17, 2022
gftea added a commit to gftea/tokio that referenced this pull request Jun 28, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jul 18, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.19.2` -> `1.20.0` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.19.2` -> `1.20.0` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.20.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.20.0)

[Compare Source](tokio-rs/tokio@tokio-1.19.2...tokio-1.20.0)

##### 1.20.0 (July 12, 2022)

##### Added

-   tokio: add track_caller to public APIs ([#&#8203;4772], [#&#8203;4791], [#&#8203;4793], [#&#8203;4806], [#&#8203;4808])
-   sync: Add `has_changed` method to `watch::Ref` ([#&#8203;4758])

##### Changed

-   time: remove `src/time/driver/wheel/stack.rs` ([#&#8203;4766])
-   rt: clean up arguments passed to basic scheduler ([#&#8203;4767])
-   net: be more specific about winapi features ([#&#8203;4764])
-   tokio: use const initialized thread locals where possible ([#&#8203;4677])
-   task: various small improvements to LocalKey ([#&#8203;4795])

##### Fixed

##### Documented

-   fs: warn about performance pitfall ([#&#8203;4762])
-   chore: fix spelling ([#&#8203;4769])
-   sync: document spurious failures in oneshot ([#&#8203;4777])
-   sync: add warning for watch in non-Send futures ([#&#8203;4741])
-   chore: fix typo ([#&#8203;4798])

##### Unstable

-   joinset: rename `join_one` to `join_next` ([#&#8203;4755])
-   rt: unhandled panic config for current thread rt ([#&#8203;4770])

[#&#8203;4677]: tokio-rs/tokio#4677

[#&#8203;4741]: tokio-rs/tokio#4741

[#&#8203;4755]: tokio-rs/tokio#4755

[#&#8203;4758]: tokio-rs/tokio#4758

[#&#8203;4762]: tokio-rs/tokio#4762

[#&#8203;4764]: tokio-rs/tokio#4764

[#&#8203;4766]: tokio-rs/tokio#4766

[#&#8203;4767]: tokio-rs/tokio#4767

[#&#8203;4769]: tokio-rs/tokio#4769

[#&#8203;4770]: tokio-rs/tokio#4770

[#&#8203;4772]: tokio-rs/tokio#4772

[#&#8203;4777]: tokio-rs/tokio#4777

[#&#8203;4791]: tokio-rs/tokio#4791

[#&#8203;4793]: tokio-rs/tokio#4793

[#&#8203;4795]: tokio-rs/tokio#4795

[#&#8203;4798]: tokio-rs/tokio#4798

[#&#8203;4806]: tokio-rs/tokio#4806

[#&#8203;4808]: tokio-rs/tokio#4808

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTEuMSIsInVwZGF0ZWRJblZlciI6IjMyLjExMS4xIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1458
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants