Parse # env-dep directives in dep-info files

[alexcrichton]

This commit updates Cargo's parsing of rustc's dep-info files to account
for changes made upstream in rust-lang/rust#71858. This means that if
env! or option_env! is used in crate files Cargo will correctly
rebuild the crate if the env var changes.

Closes #8417

[rust-highfive]

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

Note that this'll fail CI until we get a nightly tonight, but I was testing with a toolchain that has support for # env-dep so I think everything should otherwise be working.

[alexcrichton]

Ok I've pushed up a commit which fixes some dep-info parsing bits in the test suite, but this actually uncovered an unfortunate behavior that I'm not sure what to do about:

#[cargo_test]
fn env_build_script_no_rebuild() {
    // Only nightly has support in dep-info files for this
    if !cargo_test_support::is_nightly() {
        return;
    }
    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                version = "0.1.0"
            "#,
        )
        .file(
            "build.rs",
            r#"
                fn main() {
                    println!("cargo:rustc-env=FOO=bar");
                }
            "#,
        )
        .file(
            "src/main.rs",
            r#"
                fn main() {
                    println!("{:?}", env!("FOO"));
                }
            "#,
        )
        .build();

    p.cargo("build").run();
    p.cargo("build").with_stderr("[FINISHED] [..]").run();
}

That test fails because it rebuilds. The problem is that Cargo sees the previous compilation read the env var FOO, but it's reading the value of FOO from Cargo's own environment, which misses how this env var is defined by a build script. A similar issue is happening in the target dir renaming test because the value of OUT_DIR is changing, and that's not defined in Cargo's process.

Cargo can't really predict the value of env vars coming from build scripts, so I guess it just has to ignore them? We'd have to keep a list of "these env vars are defined by build scripts" from the previous compilation and use that to filter the env vars to check in Cargo itself.

I'll try to think about this and see if there's something better. Handling OUT_DIR I think will also be a bit tricky.

[Mark-Simulacrum]

Well, the behavior I would expect is that we don't necessarily rebuild the crate after re-running the build script -- I think today that's never the case, though, so would be "new feature" in some sense. I think I recall an issue about this but can't find it today.

I think that sort of thing is pretty hard given Cargo's architecture today -- IIRC, we need to upfront decide everything we're planning to build. I guess in theory the crate could be a no-op build somewhat hackily (the cargo 'step' runs but compiler isn't invoked).

[alexcrichton]

Ok I've pushed up a commit I think addresses the issue. I figured out an easy-ish way to do it because translate_dep_info happens so late in the process.

FWIW we unconditionally rebuild the crate if a build script is rerun, we definitely don't have any red/green style tracking like the compiler does where if the build script produces the same output we don't rebuild any crates. (same for if you recompile a crate and it produces byte-for-byte the same output we still build everything else)

The main problem I was imagining is that we can't even predict what env vars a build script will set. At the point we translate rustc's dep info to Cargo's dep info, however, we have a ProcessBuilder which contains all specific env vars to just that compilation which we can exclude from rustc's set of recorded env vars used during the compile. That should limit us precisely to the set of env vars used that weren't defined by Cargo, which is what we're interested in tracking. (the comment inline has more info to this)

[ehuss]

I'm thinking it might be a good idea to update the metadata version since this changes the format of the dep-info file. It should be backwards-compatible since the new cargo ignores parse errors, but older versions of cargo do not (it fails with dep-info invalid error). Not strictly necessary since switching to an older nightly is unusual, but might be worth avoiding.

There are a few comments that I'd like to see updated with the new behavior. Ones that I see (github won't let me annotate them):

• A brief note in the module-docs section "dep-info files" about env var support.
• LocalFingerprint::find_stale_item "a stale file or env var".
• parse_dep_info "a list of paths and env vars"

[ehuss]

Could this at least log a warning that it failed to parse the dep-info file? I assume this would only happen if the file is corrupt (or in some different format?). I assume you changed this so if the format is changed again in the future, cargo will just silently rebuild (which seems like the correct approach).

[alexcrichton]

Hm actually I misinterpreted the previous code. I thought the previous iteration also ignored parsing errors but it looks like it only ignores I/O and/or not present errors. I do think, though, that it's reasonable to ignore corrupt files here since we'll rewrite them later anyway.

[alexcrichton]

Ok updated!

[ehuss]

Not sure if you saw the note about changing the version in compute_metadata. I feel it would be a little safer to change that.

[alexcrichton]

Er oops sorry I saw that and then completely forgot to add it, I'll look into escaping, I don't know what kind of escaping rustc does...

[alexcrichton]

Ok read up on the escaping rustc does and am now matching that here too

[ehuss]

Thanks, looks good!

@bors r+

[bors]

📌 Commit 0750a6f has been approved by ehuss

[bors]

⌛ Testing commit 0750a6f with merge 77b4058...

[bors]

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 77b4058 to master...

[RalfJung]

@alecmocatta I think this comment here exactly describes your issue https://github.com/rust-lang/rust/issues/76511 ;)

[alecmocatta]

Yes I believe it does, thanks for finding this @RalfJung!