Implement `cargo:rerun-if-env-changed=FOO` #4125

alexcrichton · 2017-06-05T16:25:47Z

This commit implements a new method of rerunning a build script if an
environment variable changes. Environment variables are one of the primary
methods of giving inputs to a build script today, and this'll help situations
where if you change an env var you don't have to remember to clean out an old
build directory to ensure fresh results.

Closes #2776

rust-highfive · 2017-06-05T16:25:57Z

r? @brson

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

alexcrichton · 2017-06-05T16:25:59Z

r? @matklad

matklad · 2017-06-07T18:55:20Z

src/cargo/ops/cargo_rustc/fingerprint.rs

 }

 struct MtimeSlot(Mutex<Option<FileTime>>);
+struct VarSlot(Mutex<Option<String>>);


Hm, why do we need need to compute env-based fingerprints lazily? That is, why do we need Mutex here?

For MtimeSlot is needed because filesystem state will change after cargo is run, so we need to fetch mtime late.

But env::var(var) does not change, so we can eagerly calculate it.

Huh now I'm not even sure how I ended up having it implemented this way... In any case I believe you're 100% correct!

matklad · 2017-06-07T19:01:29Z

src/cargo/ops/cargo_rustc/fingerprint.rs

+        local.push(LocalFingerprint::MtimeBased(mtime, output.clone()));
+    }
+
+    for var in deps.rerun_if_env_changed.iter() {


Let's sort env vars here to avoid dependency on the order of environment variables?

Ah I don't believe this should be necessary, the rerun_if_env_changed has a deterministic ordering of whatever order the build script printed out, so the order here shouldn't change over time.

But there's a slim chance that buildscript itself prints them non-deterministically (like iterating via HashMap). But yeah, that probably doesn't matter much.

matklad · 2017-06-07T19:16:02Z

This is so much better than than just manually cargo cleaning! However, there's still a rather nasty failure mode: if one reads an env variable in build.rs or some child build process, but does not report it via env_var_changed, then one gets stale artifacts and non deterministic builds. And we can't detect this situation and issue a warning either

Would it be feasible to take a more proactive stance towards env vars? Could we perhaps require to white-list all environment variables and execute build.rs in a clean env?

alexcrichton · 2017-06-07T22:10:51Z

It's true yeah that there's a footgun that you forget an env var, but that's no different from today, no? All our builds basically can't be guaranteed reproducible b/c you still have access to things like the network. In that sense I see this as a feature for proactive authors but not bulletproof.

Now I'd totally be on board with some sort of whitelisting solution, but that's got backwards compatibility hazards with it. It's just something I haven't thought about and wouldn't know how to do immediately at least :(

matklad · 2017-06-07T22:21:26Z

Now I'd totally be on board with some sort of whitelisting solution, but that's got backwards compatibility hazards with it. It's just something I haven't thought about and wouldn't know how to do immediately at least :(

A strawman proposal would be to add a build-env field to Cargo.toml which lists env vars names. If cargo runs a build script and build-env is defined, it cleans the env, leaving only cargo specific variables and those listed in build-env, calculating the fingerprint along the way. If build-env is not defined, than Cargo works exactly as today.

retep998 · 2017-06-08T00:43:57Z

@matklad Note that wiping all environment variables causes things to break on Windows. rust-lang/rust#31259

matklad · 2017-06-08T18:33:29Z

@alexcrichton raised an interesting point that even if we whitelist env in Cargo.toml, we might still need rerun-if-env-changed to specify on which subset of vars we actually depend. For example, a build.rs for retrieving a native libfoo dependency might use both a LIB_FOO var to get compiled libfoo and CC to compile libfoo by itself, but only if LIB_FOO is not provided.

So in some sense, rerun-if-env-changed and whitelisting of env vars is orthogonal. I think we should merge this as is then, the implementation and tests look great!

alexcrichton · 2017-06-13T19:27:37Z

Ah yes sorry I meant to write up a comment as well, but looks like @matklad beat me to it!

r? @matklad

matklad · 2017-06-13T21:07:20Z

src/cargo/ops/cargo_rustc/fingerprint.rs

-            if !outputs[&key].rerun_if_changed.is_empty() {
-                let slot = MtimeSlot(Mutex::new(None));
-                fingerprint.local = LocalFingerprint::MtimeBased(slot,
-                                                                 output_path);


I don't fully understand what happens here, but looks like the logic before was

if we need a costly fingerprint update operation: update_local() do costly operation

and now it is like

if we need a costly fingerprint update operation: update_local() if we need a costly fingerprint update operation: do costly operation

That is, looks like this if and hash_busted variable from the update_local function are trying to do the same thing in two different places. Perhaps we can get rid of this outer if?

Hm so due to the way that this works I think we can actually avoid calling update_local entirely, the call in local_fingerprints_deps should already load the mtime and new env vars which should be all that's needed.

Er actually I had to keep update_local()?. I don't know why and don't quite have time to investigate right now. I'm also not sure I understand your comment though, can you elaborate?

I'm also not sure I understand your comment though, can you elaborate?

It was just a gut feeling that something's wrong about this if. However today I have a more concrete question :)

This test currently fails, and I believe that it should pass (according to the current docs):

#[test] fn rerun_if_only_file_changes() { let p = project("foo") .file("Cargo.toml", r#" [package] name = "foo" version = "0.5.0" authors = [] "#) .file("src/main.rs", r#" fn main() { println!("Hello, World"); } "#) .file("build.rs", r#" fn main() { println!("cargo:rerun-if-env-changed=FOO"); } "#) .file("foo", ""); p.build(); assert_that(p.cargo("build"), execs().with_status(0) .with_stderr("\ [COMPILING] foo v0.5.0 ([..]) [FINISHED] [..] ")); sleep_ms(1000); File::create(&p.root().join("some-new-file")).unwrap(); File::create(p.root().join("foo")).unwrap(); assert_that(p.cargo("build"), execs().with_status(0) .with_stderr("\ [COMPILING] foo v0.5.0 ([..]) [FINISHED] [..] ")); }

If build.rs does not produce any rerun-if-changed then we promise that it will be rerun on any change inside the build directory, which is different from the case when build.rs specifies an empty set of files in rerun-if-changed.

Looks like currently specifying only rerun-if-env-changed implies empty rerun-if-changed.

To clarify, if you comment out println!("cargo:rerun-if-env-changed=FOO"); in the test, it passes.

Ah yeah it was originally my intention that the test you gisted here should fail. In other words specifying any rerun-if-*-changed is sufficient for telling Cargo "I've told you about all of my dependencies"

Sounds reasonable! It's probably worth mentioning in the docs more explicitly?

Sounds good to me! I've added a clause to the docs.

alexcrichton · 2017-06-13T21:33:10Z

Updated!

This commit implements a new method of rerunning a build script if an environment variable changes. Environment variables are one of the primary methods of giving inputs to a build script today, and this'll help situations where if you change an env var you don't have to remember to clean out an old build directory to ensure fresh results. Closes rust-lang#2776

matklad · 2017-06-15T16:18:17Z

#bors r+

matklad · 2017-06-15T16:18:26Z

@bors r+

bors · 2017-06-15T16:18:27Z

📌 Commit fe8bbb7 has been approved by matklad

bors · 2017-06-15T17:52:47Z

⌛ Testing commit fe8bbb7 with merge b4b7ed5...

Implement `cargo:rerun-if-env-changed=FOO` This commit implements a new method of rerunning a build script if an environment variable changes. Environment variables are one of the primary methods of giving inputs to a build script today, and this'll help situations where if you change an env var you don't have to remember to clean out an old build directory to ensure fresh results. Closes #2776

bors · 2017-06-15T18:26:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing b4b7ed5 to master...

@alexcrichton

…ulacrum rustc_llvm: re-run build script when env var LLVM_CONFIG changes This removes the changes done in rust-lang#42429 and use the newly introduced `cargo:rerun-if-env-changed` in rust-lang/cargo#4125. As `LLVM_CONFIG` env var points to the `llvm-config` and changes when it gets configured in `config.toml` or removed from it, we can re-run the build script if this env var changes. closes rust-lang#42444 r? @alexcrichton

@alexcrichton

…ulacrum rustc_llvm: re-run build script when env var LLVM_CONFIG changes This removes the changes done in rust-lang#42429 and use the newly introduced `cargo:rerun-if-env-changed` in rust-lang/cargo#4125. As `LLVM_CONFIG` env var points to the `llvm-config` and changes when it gets configured in `config.toml` or removed from it, we can re-run the build script if this env var changes. closes rust-lang#42444 r? @alexcrichton

@alexcrichton

…ulacrum rustc_llvm: re-run build script when env var LLVM_CONFIG changes This removes the changes done in rust-lang#42429 and use the newly introduced `cargo:rerun-if-env-changed` in rust-lang/cargo#4125. As `LLVM_CONFIG` env var points to the `llvm-config` and changes when it gets configured in `config.toml` or removed from it, we can re-run the build script if this env var changes. closes rust-lang#42444 r? @alexcrichton

Fix fingerprint calculation for patched deps. If you have A→B→C where B and C are in a registry, and you `[patch]` C, the fingerprint calculation wasn't working correctly when C changes. The following sequence illustrates the problem: 1. Do a build from scratch. 2. Touch a file in C. 3. Build again. Everything rebuilds as expected. 4. Build again. You would expect this to be all fresh, but it rebuilds A. The problem is the hash-busting doesn't propagate up to parents from dependencies. Normal targets normally aren't a problem because they have a `LocalFingerprint::MtimeBased` style local value which always recomputes the hash. However, registry dependencies have a `Precalculated` style local value which never recomputes the hash. The solution here is to always recompute the hash. This shouldn't be too expensive, and is only done when writing the fingerprint, which should only happen when the target is dirty. I'm not entirely certain why the caching logic was added in #4125. Fixes rust-lang/rust#57142

rust-highfive assigned brson Jun 5, 2017

rust-highfive assigned matklad and unassigned brson Jun 5, 2017

alexcrichton mentioned this pull request Jun 5, 2017

rustc_llvm: re-run build script if config.toml changes rust-lang/rust#42429

Merged

matklad reviewed Jun 7, 2017

View reviewed changes

alexcrichton force-pushed the rerun-if-env-changed branch from 3f6a85c to cbffd7f Compare June 7, 2017 22:10

matklad mentioned this pull request Jun 7, 2017

Add a way for build scripts to be re-run if specific environment variables change #2776

Closed

matklad reviewed Jun 13, 2017

View reviewed changes

alexcrichton force-pushed the rerun-if-env-changed branch from cbffd7f to 0b6cc4a Compare June 13, 2017 21:33

alexcrichton force-pushed the rerun-if-env-changed branch from 0b6cc4a to 9720d3b Compare June 14, 2017 14:52

alexcrichton force-pushed the rerun-if-env-changed branch from 9720d3b to fe8bbb7 Compare June 14, 2017 21:23

bors merged commit fe8bbb7 into rust-lang:master Jun 15, 2017

alexcrichton deleted the rerun-if-env-changed branch June 15, 2017 18:42

This was referenced Jun 29, 2017

Don't rebuild LLVM on non-related changes to config.toml rust-lang/rust#42444

Closed

rustc_llvm: re-run build script when env var LLVM_CONFIG changes rust-lang/rust#42985

Merged

alexcrichton mentioned this pull request Jul 1, 2017

Use feature guard instead of environment variable to enable static link sfackler/rust-openssl#653

Closed

retep998 mentioned this pull request Jul 15, 2017

Use cargo:rerun-if-env-changed in build scripts gtk-rs/sys#54

Open

ehuss mentioned this pull request Dec 28, 2018

Fix fingerprint calculation for patched deps. #6493

Merged

ehuss added this to the 1.20.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `cargo:rerun-if-env-changed=FOO` #4125

Implement `cargo:rerun-if-env-changed=FOO` #4125

alexcrichton commented Jun 5, 2017

rust-highfive commented Jun 5, 2017

alexcrichton commented Jun 5, 2017

matklad Jun 7, 2017

alexcrichton Jun 7, 2017

matklad Jun 7, 2017 •

edited

Loading

alexcrichton Jun 7, 2017

matklad Jun 7, 2017

matklad commented Jun 7, 2017

alexcrichton commented Jun 7, 2017

matklad commented Jun 7, 2017

retep998 commented Jun 8, 2017

matklad commented Jun 8, 2017

alexcrichton commented Jun 13, 2017

matklad Jun 13, 2017

alexcrichton Jun 13, 2017

alexcrichton Jun 14, 2017

matklad Jun 14, 2017

matklad Jun 14, 2017

alexcrichton Jun 14, 2017

matklad Jun 14, 2017

alexcrichton Jun 14, 2017

alexcrichton commented Jun 13, 2017

matklad commented Jun 15, 2017

matklad commented Jun 15, 2017

bors commented Jun 15, 2017

bors commented Jun 15, 2017

bors commented Jun 15, 2017

Implement cargo:rerun-if-env-changed=FOO #4125

Implement cargo:rerun-if-env-changed=FOO #4125

Conversation

alexcrichton commented Jun 5, 2017

rust-highfive commented Jun 5, 2017

alexcrichton commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Jun 7, 2017

alexcrichton commented Jun 7, 2017

matklad commented Jun 7, 2017

retep998 commented Jun 8, 2017

matklad commented Jun 8, 2017

alexcrichton commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jun 13, 2017

matklad commented Jun 15, 2017

matklad commented Jun 15, 2017

bors commented Jun 15, 2017

bors commented Jun 15, 2017

bors commented Jun 15, 2017

Implement `cargo:rerun-if-env-changed=FOO` #4125

Implement `cargo:rerun-if-env-changed=FOO` #4125

matklad Jun 7, 2017 •

edited

Loading