Custom cfg `loom` makes cargo spam warnings

simonwuelker commented 5 months ago

The newest cargo nightly release includes automatic checking of expected cfg values. ^1 The TLDR is that this is supposed to catch typos like #[cfg(windosw)].

Unfortunately, this lint triggers on oneshot's loom cfg quite a lot (24 times at the time of writing).

warning: unexpected `cfg` condition name: `loom`
   --> src/lib.rs:125:11
    |
125 | #[cfg(not(loom))]
    |           ^^^^
    |
    = help: consider using a Cargo feature instead or adding `println!("cargo::rustc-check-cfg=cfg(loom)");` to the top of the `build.rs`
    = note: see <https://doc.rust-lang.org/nightly/cargo/reference/build-scripts.html#rustc-check-cfg> for more information about checking conditional configuration

Solutions

There are a few ways of fixing this:

Make rustc aware of the cfg. ^2 Can be done by adding build.rs that looks like this
```
fn main() {
  println!("cargo::rustc-check-cfg=cfg(loom)");
}
```
I am personally against this, since it would be adding a whole compilation step for basically nothing.
Add #![allow(unexpected_cfgs)] to lib.rs Seems lazy?
Use a regular cargo feature instead. Not great because cargo features are supposed to be additive ^3. But seems possible.
Use #[cfg(test)] or some other standard-cfg instead of #[cfg(loom)] Haven't looked at the actual tests enough to say whether this is reasonable at all

I would be happy to implement a fix for this, any thoughts on what would be a reasonable move?

faern commented 5 months ago

Hmm.. None of the options seem ideal. I suggest bringing the question upstream, this pattern comes from loom itself and is nothing I invented here. Would be nice to have input from the loom developers and/or maybe have them update their recommendation and following that.

faern commented 5 months ago

... adding build.rs that looks like this: ..

I agree this is not nice! A build script should only be used when needed. I personally don't care as much about the compile time penalty. More the supply chain security aspect. A crate with a build.rs is a crate that can do anything at build time.

Add #![allow(unexpected_cfgs)] to lib.rs

Lazy indeed! I don't think this is an ideal solution. But it's what I'm doing temporary to get the CI working again (https://github.com/faern/oneshot/pull/36)

Use a regular cargo feature instead.

Indeed not additive. And I'm a fairly strong proponent for features being additive. Another downside is that cargo features are kind of "public features" of a crate. Loom is strictly for internal testing. This also mess with the cargo hack --feature-powerset functionality we currently use that expect loom to be either on or off for all combinations right now.

Use #[cfg(test)] or some other standard-cfg instead of #[cfg(loom)]

I don't think we are in "test mode" all the times when we want to use loom? :thinking:

simonwuelker commented 5 months ago

Unfortunately it seems like silencing the lint is the most sane option available at the moment :/

Its what was suggested on the loom issue i filed, and other projects are doing the same:

The only real "solution" that I've seen is adding a build.rs that is not shipped on crates.io. (https://github.com/tokio-rs/tokio/pull/6542) But this is bad in its own right, as you would be testing different code than what is shipped to the user...

simonwuelker commented 4 months ago

The blog post^1 was updated, you can now modify [lints.rust.unexpected_cfgs], like this:

[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(loom)', 'cfg(fuzzing)'] }

I'll open a pr for this later

faern commented 4 months ago

This solution seems waay cleaner! Looking forward to the PR :sparkles:

faern / oneshot

Custom cfg `loom` makes cargo spam warnings #34

Solutions