Can't set debug-assertions via -Z config-profile

[alexcrichton]

Currently I get:

$ cargo new foo
$ cd foo
$ CARGO_PROFILE_DEV_DEBUG_ASSERTIONS=false cargo +nightly build
error: failed to parse manifest at `/home/alex/code/wut/foo/Cargo.toml`

Caused by:
  missing config key `profile.dev.debug`

cc @ehuss

[alexcrichton]

Something to do with this check I believe, gonna try to page serde back in...

[ehuss]

Oh, that's a tricky one. It looks like it is confused on "debug" vs "debug-assertions". I'll take a look at a fix, it may need to scan for longest keys first. There's a bit of ambiguity there.

[ehuss]

Yikes, it's much more difficult than I expected. At the point where it deserializes an Option, it doesn't know what the inner type may be. It may be a map (or struct) with additional key parts, or it may be a basic type with no additional key parts. There's no way to inspect what the type may be.

I'm not sure how to deal with that. There could be a custom deserialize implementation for Option<T> where T is a map or struct, and then it could behave differently, somehow. But it's a little mind-bending.

Another option is to not allow _ in key names. That is, it would be DEBUGASSERTIONS instead of DEBUG_ASSERTIONS. Sounds familiar to the problem where map keys with underscores don't work (like serde_json) from #5552.

Or we could rethink how config env vars work altogether.

[alexcrichton]

I actually think that disallowing _ in key names if it works isn't a bad idea. I don't think we can quite get away with that today because lots of places have things like CARGO_TARGET_X86_64_... and we wouldn't want to break those. I could imagine though also accepting "squished" versions like CARGO_TARGET_X8664..., but do you think that'd be enough to fix this?

Apart from that though I agree we may need to rethink env vars :(

[ehuss]

I could imagine though also accepting "squished" versions

The "target" table works a little differently since it is queried by key (it knows ahead of time what target to look for). I'm not sure, but I think profiles is the only config thing with a map that hits this problem.

I'm going to leave this here so I don't lose it. One test currently fails, and the other fails if these lines are removed.

#[cargo_test]
fn tricky_env_config() {
    // Some issues with underscores in env vars.
    #[derive(Deserialize)]
    struct WithOptMap {
        m: Option<collections::BTreeMap<String, u32>>,
    }
    let config = new_config(&[
        ("CARGO_WITHOPTMAP_M_bitflags", "1"),
    ]);
    let s: WithOptMap = config.get("withoptmap").unwrap();
    assert_eq!(s.m.expect("option").get("bitflags"), Some(&1));

    #[derive(Deserialize)]
    struct Ambig {
        debug: Option<u32>,
        debug_assertions: Option<bool>,
    }
    let config = new_config(&[
        ("CARGO_AMBIG_DEBUG_ASSERTIONS", "true"),
    ]);
    let s: Ambig = config.get("ambig").unwrap();
    assert_eq!(s.debug_assertions, Some(true));
    assert_eq!(s.debug, None);
}

[alexcrichton]

Yeah I've always sort of regretted that we have these weird paths of configuration in Cargo which aren't entirely consistent so some stuff (like target configuration) works well but others based on table accesses (like profiles) don't work so well.

It may be possible with serde-fu to handle these errors and basically deterministically swallow them specifically for optional fields if the env key ended up not being found. That being said I don't personally posess the serde-fu to finagle that.

A perhaps drastic alternative is to say this is "wontfix" and start splitting environment variables on periods instead of underscores (well, both for compat, but you get the idea). That way we could do the equivalent of cargo.profile.dev.debug-assertions=1 as an environment variable, which although it's pretty difficult to set that from the shell (I think you need to use env) it may be good enough for tooling purposes like rustbuild or other build systems which invoke Cargo.

[ehuss]

It may be possible with serde-fu to handle these errors

I initially thought of trying that, but the Visitor takes the deserializer by value and the visitor can't be cloned or anything.

This issue seems to provide something similar to what we're trying to achieve. When I have more time and energy, I'll try to take another stab to see if there is serde magic that'll work.

splitting environment variables on periods

I'm also tempted by it, but it can be very awkward to use in most shells.

[ehuss]

Another option is to do what mdbook does and use double underscores to represent a dot to avoid confusion with dashes: https://rust-lang-nursery.github.io/mdBook/format/config.html#environment-variables

So it would be something like CARGO__PROFILE__DEV__DEBUG_ASSERTIONS. Not very pretty, but an option.

[alexcrichton]

This has been sitting in my inbox for awhile and I've meant to respond to it, sorry about that!

I don't really have a great opinion on what to do here at this point, but double-underscores seems like sort of the least bad option for now and isn't that unreasonable really.