search

oxidecomputer / usdt

Dust your Rust with USDT probes
Apache License 2.0
84 stars 10 forks source link

[meta] add check-cfg directives to build.rs #268

Closed sunshowers closed 1 month ago

sunshowers commented 1 month ago

These directives are checked by newer versions of Rust to ensure that there aren't typos.

pfmooney commented 1 month ago

Older toolchains won't complain about these unrecognized (to them) directives, right?

sunshowers commented 1 month ago

Older toolchains won't complain about these unrecognized (to them) directives, right?

Good question! Depends on if you're doing the build in this workspace or not, and which version of Rust you're compiling with.

Build within this workspace + Rust < 1.80

If you're in this workspace, then these older Rust versions will produce warnings of the form:

warning: usdt-impl@0.5.0: cargo:rustc-check-cfg requires -Zcheck-cfg flag
warning: usdt-impl@0.5.0: cargo:rustc-check-cfg requires -Zcheck-cfg flag
warning: usdt-impl@0.5.0: cargo:rustc-check-cfg requires -Zcheck-cfg flag

These warnings are harmless, but will show up each time the build happens.

(That's actually why I suggested #269 -- as things stand, if this PR lands in master a cargo build will produce warnings. If the rust-toolchain.toml file is removed, a standard cargo build won't produce any warnings.)

Build outside this workspace

If you're in a downstream workspace that depends on this one via crates.io or a Git dependency, then no warnings are produced regardless of version.

However, if you're in a downstream workspace that depends on this one via a path dependency, and you're building with older versions of Rust, then you'll see warnings.

One option is to only write out the check-cfg directives for Rust versions 1.80 and up. I'm not sure how much I like that to be honest... do you think we should do that?

pfmooney commented 1 month ago

One option is to only write out the check-cfg directives for Rust versions 1.80 and up. I'm not sure how much I like that to be honest... do you think we should do that?

That seems worthwhile to me (although it's not a strongly held opinion). The version check is a small thing, and it cuts down on the avenues for one to potentially encounter noisy warnings.

sunshowers commented 1 month ago

Superseded by #275.