Skip to content

mark few deps as optional, which is used only from some features#3774

Merged
rami3l merged 2 commits intorust-lang:masterfrom
klensy:tracing
Apr 14, 2024
Merged

mark few deps as optional, which is used only from some features#3774
rami3l merged 2 commits intorust-lang:masterfrom
klensy:tracing

Conversation

@klensy
Copy link
Copy Markdown
Contributor

@klensy klensy commented Apr 14, 2024

No description provided.

@djc
Copy link
Copy Markdown
Contributor

djc commented Apr 14, 2024

Makes sense, thanks! Have you looked at whether there is CI coverage of the various possible feature sets?

@klensy
Copy link
Copy Markdown
Contributor Author

klensy commented Apr 14, 2024

No, i didn't checked CI, only manually grepped sources for usage and tried to build with curl/reqwest backends.

Copy link
Copy Markdown
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

@djc cargo-all-features in our CI seems to be working.

Thanks a lot @klensy!

@rami3l rami3l added this pull request to the merge queue Apr 14, 2024
@rami3l rami3l added this to the 1.27.1 milestone Apr 14, 2024
Merged via the queue into rust-lang:master with commit 1de2394 Apr 14, 2024
@djc
Copy link
Copy Markdown
Contributor

djc commented Apr 14, 2024

Yeah, I was wondering about the --no-default-features case.

@rami3l
Copy link
Copy Markdown
Member

rami3l commented Apr 14, 2024

Yeah, I was wondering about the --no-default-features case.

@djc It would surprise me if cargo-all-features considers default features at all.

Turns out that's indeed not the case:
https://github.com/frewsxcv/cargo-all-features/blob/fe252f463d6cf2969bf7ce5f911cc6222bc80c9d/src/test_runner.rs#L25

@djc
Copy link
Copy Markdown
Contributor

djc commented Apr 14, 2024

I meant I was wondering if we have CI coverage of compiling with no default features.

@djc
Copy link
Copy Markdown
Contributor

djc commented Apr 15, 2024

And the answer is no, because:

#[cfg(all(not(feature = "reqwest-backend"), not(feature = "curl-backend")))]
compile_error!("Must enable at least one backend");

@rami3l
Copy link
Copy Markdown
Member

rami3l commented Apr 15, 2024

And the answer is no, because:

#[cfg(all(not(feature = "reqwest-backend"), not(feature = "curl-backend")))]
compile_error!("Must enable at least one backend");

@djc That case has been explicitly excluded to prevent CI failure:

rustup/Cargo.toml

Lines 193 to 195 in f26d16b

[package.metadata.cargo-all-features]
# Building with no web backend will error.
always_include_features = ["reqwest-backend", "reqwest-rustls-tls"]

Apart from that, our cargo-all-features CI does test all possible feature combinations since it always adds --no-default-features to the call, as mentioned in #3774 (comment).

PS: You have probably misunderstood me: when I say cargo-all-features I mean the test plugin to ensure all feature combinations compile, rather than the cargo flag named --all-features.

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