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

fix(udp): feature flag tracing in windows.rs #1932

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Jul 22, 2024

8712910 made tracing optional and added optional log, but forgot to update quinn-udp/src/windows.rs. This commit fixes the oversight and bumps the quinn-udp version to v0.5.4.


Discovered in mozilla/neqo#2002.

I assume that this is not an issue for most quinn users as they don't run quinn-udp with default-features = false".

I am sorry for the oversight. I can look into a GitHub CI workflow that prevents this in the future tomorrow.

mxinden added a commit to mxinden/neqo that referenced this pull request Jul 22, 2024
quinn-udp v0.5.3 introduces a compile time error on Windows. This will be fixed
with quinn-rs/quinn#1932. To unblock CI in the meantime,
exclude v0.5.3.
@mxinden
Copy link
Contributor Author

mxinden commented Jul 22, 2024

Not sure what your policy towards cargo yank is, but this might be a good candidate.

Again, sorry for the trouble.

@mxinden mxinden marked this pull request as draft July 22, 2024 19:14
@mxinden
Copy link
Contributor Author

mxinden commented Jul 22, 2024

🤦 another mistake. Too late in the evening for me. Will push a commit in a minute.

8712910 made `tracing` optional and added optional `log`, but forgot to update
`quinn-udp/src/windows.rs`. This commit fixes the oversight and bumps the
`quinn-udp` version to `v0.5.4`.
@mxinden mxinden marked this pull request as ready for review July 22, 2024 19:17
@mxinden
Copy link
Contributor Author

mxinden commented Jul 22, 2024

Alright, this should be it. For the record, compiling with all feature combinations:

➜  src git:(optional-tracing-windows) ✗ cargo check --no-default-features --target x86_64-pc-windows-msvc
warning: /home/mxinden/code/github.com/quinn-rs/quinn/quinn-udp/Cargo.toml: unused manifest key: target.cfg(any(target_os = "linux", target_os = "windows")).bench
    Checking quinn-udp v0.5.4 (/home/mxinden/code/github.com/quinn-rs/quinn/quinn-udp)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.34s
➜  src git:(optional-tracing-windows) ✗ cargo check --no-default-features --target x86_64-pc-windows-msvc --features tracing
warning: /home/mxinden/code/github.com/quinn-rs/quinn/quinn-udp/Cargo.toml: unused manifest key: target.cfg(any(target_os = "linux", target_os = "windows")).bench
    Checking tracing v0.1.40
    Checking quinn-udp v0.5.4 (/home/mxinden/code/github.com/quinn-rs/quinn/quinn-udp)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.46s
➜  src git:(optional-tracing-windows) ✗ cargo check --no-default-features --target x86_64-pc-windows-msvc --features direct-log
warning: /home/mxinden/code/github.com/quinn-rs/quinn/quinn-udp/Cargo.toml: unused manifest key: target.cfg(any(target_os = "linux", target_os = "windows")).bench
    Checking quinn-udp v0.5.4 (/home/mxinden/code/github.com/quinn-rs/quinn/quinn-udp)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.27s
➜  src git:(optional-tracing-windows) ✗ cargo check --no-default-features --target x86_64-pc-windows-msvc --features tracing,direct-log
warning: /home/mxinden/code/github.com/quinn-rs/quinn/quinn-udp/Cargo.toml: unused manifest key: target.cfg(any(target_os = "linux", target_os = "windows")).bench
    Checking quinn-udp v0.5.4 (/home/mxinden/code/github.com/quinn-rs/quinn/quinn-udp)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.30s

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Good catch, thanks! No worries about the error, it happens.

A good follow-up item might be to avoid the fragile and verbose duplicate conditionals by introducing some internal facade macros that dispatch to tracing or log or nothing as appropriate.

@Ralith Ralith merged commit 061a74f into quinn-rs:main Jul 22, 2024
8 checks passed
@Ralith
Copy link
Collaborator

Ralith commented Jul 22, 2024

Published.

@djc
Copy link
Member

djc commented Jul 22, 2024

That sounds good to me. rustls has something like this which we can probably just copy over.

github-merge-queue bot pushed a commit to mozilla/neqo that referenced this pull request Jul 22, 2024
quinn-udp v0.5.3 introduces a compile time error on Windows. This will be fixed
with quinn-rs/quinn#1932. To unblock CI in the meantime,
exclude v0.5.3.
@mxinden
Copy link
Contributor Author

mxinden commented Jul 23, 2024

A good follow-up item might be to avoid the fragile and verbose duplicate conditionals by introducing some internal facade macros that dispatch to tracing or log or nothing as appropriate.

That sounds good to me. rustls has something like this which we can probably just copy over.

Looking into this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants