Build script rerun-if changes break rebuild detection.

[ehuss]

If a build script changes what values it prints for "rerun-if" directives between runs, then the rebuild detection doesn't work correctly. An example of what happens:

1. Perform a build, which emits a certain set of rerun-if directives.
2. Change something that invalidates a rerun-if directive, and build. During this step, the build script should emit a different set of rerun-if directives to trigger the bug.
3. Build again with nothing changed from step 2, causes a rebuild when it shouldn't.

A real-world example is the llvm and sanitizer build scripts in rust-lang/rust (like this). I suspect this is a relatively common idiom.

There is also a problem, when running a debug build of cargo, it causes cargo to panic and hangs. In non-debug builds, it hits this unhelpful line because it has become confused.

The problem is this line. When the local value is updated, the cached hash is invalidated. However, the invalidation needs to propagate to all parent units as well. Otherwise they write out their fingerprint with the deps value using the old hash.

The panics in debug mode are due to the json validations. The round-trip fails because the cached hash values are incorrect. It hangs because of #6433.

I've been trying to think of some way to work around this, but haven't come up with any good ideas. @alexcrichton do you have any ideas how to invalidate the memoized hashes? Or maybe a completely different approach?

Note that this is similar in spirit to #6779, but I think the solution may be different.

Here is a sample test. Similar problems occur with rerun-if-changed, but env vars are a little easier to use. Each of the "FIXME" lines fails with current cargo.

#[cargo_test]
fn rerun_if_changes() {
    let p = project()
        .file(
            "build.rs",
            r#"
                fn main() {
                    println!("cargo:rerun-if-env-changed=FOO");
                    if std::env::var("FOO").is_ok() {
                        println!("cargo:rerun-if-env-changed=BAR");
                    }
                }
            "#,
        )
        .file("src/lib.rs", "")
        .build();

    p.cargo("build").run();
    // Stays fresh.
    p.cargo("build").with_stderr("[FINISHED] [..]").run();
    p.cargo("build -v")
        .env("FOO", "1")
        .with_stderr(
            "\
[COMPILING] foo [..]
[RUNNING] `[..]build-script-build`
[RUNNING] `rustc [..]
[FINISHED] [..]
",
        )
        .run();
    // Stays fresh.
    // FIXME: Error here.
    p.cargo("build")
        .env("FOO", "1")
        .with_stderr("[FINISHED] [..]")
        .run();

    p.cargo("build -v")
        .env("FOO", "1")
        .env("BAR", "1")
        .with_stderr(
            "\
[COMPILING] foo [..]
[RUNNING] `[..]build-script-build`
[RUNNING] `rustc [..]
[FINISHED] [..]
",
        )
        .run();
    // Stays fresh.
    p.cargo("build")
        .env("FOO", "1")
        .env("BAR", "1")
        .with_stderr("[FINISHED] [..]")
        .run();

    p.cargo("build -v")
        .env("BAR", "2")
        .with_stderr(
            "\
[COMPILING] foo [..]
[RUNNING] `[..]build-script-build`
[RUNNING] `rustc [..]
[FINISHED] [..]
",
        )
        .run();
    // Ignores BAR.
    // FIXME: Error here.
    p.cargo("build")
        .env("BAR", "2")
        .with_stderr("[FINISHED] [..]")
        .run();
}

[alexcrichton]

Ok I think I understand what's happening here as well, and everything you mentioned seems spot on.

One thing I was a bit confused at is where the memoized hashes that need to be invalidated come from, but this happens during an incremental build where we compare a bunch of hashes, so before the build starts we've calculated a bunch of hashes, including the parents of the one being invalidated. Then when it's invalidated everything stops working because the parent memoized hashes aren't invalidated when the child is.

Also to clarify where debug cargo fails, it trips this assertion which asserts the memoized hash is indeed matching the hash of the actual struct.

---

As to how to fix this, that's a good question. This is the only instance of invalidating a hash and I would prefer that it doesn't have too large of an impact on Cargo. That being said the fact that the hash changes at runtime is pretty fundamental to how build scripts work, so it should probably have some impact on design.

A quick-and-dirty solution would look like:

diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs
index 27392ac7c..ff7320abe 100644
--- a/src/cargo/core/compiler/fingerprint.rs
+++ b/src/cargo/core/compiler/fingerprint.rs
@@ -301,7 +301,6 @@ pub fn prepare_target<'a, 'cfg>(
             // hobble along if it happens to return `Some`.
             if let Some(new_local) = (gen_local)(&deps, None)? {
                 *fingerprint.local.lock().unwrap() = new_local;
-                *fingerprint.memoized_hash.lock().unwrap() = None;
             }
 
             write_fingerprint(&loc, &fingerprint)
@@ -613,6 +612,10 @@ impl Fingerprint {
         ret
     }
 
+    pub fn clear_memoized(&self) {
+        *self.memoized_hash.lock().unwrap() = None;
+    }
+
     /// Compares this fingerprint with an old version which was previously
     /// serialized to filesystem.
     ///
diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs
index cea9e4e23..f7637eba1 100644
--- a/src/cargo/core/compiler/job_queue.rs
+++ b/src/cargo/core/compiler/job_queue.rs
@@ -248,6 +248,10 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
             .take()
             .map(move |srv| srv.start(move |msg| drop(tx.send(Message::FixDiagnostic(msg)))));
 
+        for unit in cx.fingerprints.values() {
+            unit.clear_memoized();
+        }
+
         // Use `crossbeam` to create a scope in which we can execute scoped
         // threads. Note that this isn't currently required by Cargo but it was
         // historically required. This is left in for now in case we need the

which is to basically just zero out all memoizations just before we start doing real work. That may be an alright long-term solution too, but I'd want to at least attempt to take a look at the perf impact of that.

[ehuss]

I thought about doing the same thing, too. I was wondering if there might be a more clever solution (I was investigating if the local value could be excluded from the hash, but that's not possible).

I did some tests with your patch, and I don't see any performance difference. Do you want to open a PR with it?

[alexcrichton]

Sure yeah, I'll run that tomorrow and do some more poking at it myself as well.