Auto merge of #6484 - Eh2406:modified-during-build, r=alexcrichton

Rebuild on mid build file modification

This is an attempt to Fixes #2426, it is based on the test @dwijnand made for #5417.

Author: bors

src/cargo/core/compiler/custom_build.rs

@@ -332,6 +332,7 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
             state.build_plan(invocation_name, cmd.clone(), Arc::new(Vec::new()));
         } else {
             state.running(&cmd);
+            let timestamp = paths::get_current_filesystem_time(&output_file)?;
             let output = if extra_verbose {
                 let prefix = format!("[{} {}] ", id.name(), id.version());
                 state.capture_output(&cmd, Some(prefix), true)
@@ -354,6 +355,7 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
             // state informing what variables were discovered via our script as
             // well.
             paths::write(&output_file, &output.stdout)?;
+            filetime::set_file_times(output_file, timestamp, timestamp)?;
             paths::write(&err_file, &output.stderr)?;
             paths::write(&root_output_file, util::path2bytes(&script_out_dir)?)?;
             let parsed_output =

src/cargo/core/compiler/fingerprint.rs

@@ -761,23 +761,25 @@ where
             }
         };
 
-        // Note that equal mtimes are considered "stale". For filesystems with
-        // not much timestamp precision like 1s this is a conservative approximation
+        // TODO: fix #5918
+        // Note that equal mtimes should be considered "stale". For filesystems with
+        // not much timestamp precision like 1s this is would be a conservative approximation
         // to handle the case where a file is modified within the same second after
-        // a build finishes. We want to make sure that incremental rebuilds pick that up!
+        // a build starts. We want to make sure that incremental rebuilds pick that up!
         //
         // For filesystems with nanosecond precision it's been seen in the wild that
         // its "nanosecond precision" isn't really nanosecond-accurate. It turns out that
         // kernels may cache the current time so files created at different times actually
         // list the same nanosecond precision. Some digging on #5919 picked up that the
         // kernel caches the current time between timer ticks, which could mean that if
-        // a file is updated at most 10ms after a build finishes then Cargo may not
+        // a file is updated at most 10ms after a build starts then Cargo may not
         // pick up the build changes.
         //
-        // All in all, the equality check here is a conservative assumption that,
+        // All in all, an equality check here would be a conservative assumption that,
         // if equal, files were changed just after a previous build finished.
-        // It's hoped this doesn't cause too many issues in practice!
-        if mtime2 >= mtime {
+        // Unfortunately this became problematic when (in #6484) cargo switch to more accurately
+        // measuring the start time of builds.
+        if mtime2 > mtime {
             info!("stale: {} -- {} vs {}", path.display(), mtime2, mtime);
             true
         } else {

src/cargo/core/compiler/mod.rs

@@ -292,6 +292,7 @@ fn rustc<'a, 'cfg>(
         }
 
         state.running(&rustc);
+        let timestamp = paths::get_current_filesystem_time(&dep_info_loc)?;
         if json_messages {
             exec.exec_json(
                 rustc,
@@ -334,6 +335,7 @@ fn rustc<'a, 'cfg>(
                         rustc_dep_info_loc.display()
                     ))
                 })?;
+            filetime::set_file_times(dep_info_loc, timestamp, timestamp)?;
         }
 
         Ok(())

src/cargo/util/paths.rs

@@ -197,6 +197,18 @@ pub fn mtime(path: &Path) -> CargoResult<FileTime> {
     Ok(FileTime::from_last_modification_time(&meta))
 }
 
+/// get `FileTime::from_system_time(SystemTime::now());` using the exact clock that this file system is using.
+pub fn get_current_filesystem_time(path: &Path) -> CargoResult<FileTime> {
+    // note that if `FileTime::from_system_time(SystemTime::now());` is determined to be sufficient,
+    // then this can be removed.
+    let timestamp = path.with_file_name("invoked.timestamp");
+    write(
+        &timestamp,
+        b"This file has an mtime of when this was started.",
+    )?;
+    Ok(mtime(&timestamp)?)
+}
+
 #[cfg(unix)]
 pub fn path2bytes(path: &Path) -> CargoResult<&[u8]> {
     use std::os::unix::prelude::*;

tests/testsuite/freshness.rs

@@ -1,5 +1,7 @@
-use std::fs::{self, File};
+use std::fs::{self, File, OpenOptions};
 use std::io::prelude::*;
+use std::net::TcpListener;
+use std::thread;
 
 use crate::support::paths::CargoPathExt;
 use crate::support::registry::Package;
@@ -1304,6 +1306,9 @@ fn bust_patched_dep() {
         .build();
 
     p.cargo("build").run();
+    if is_coarse_mtime() {
+        sleep_ms(1000);
+    }
 
     File::create(&p.root().join("reg1new/src/lib.rs")).unwrap();
     if is_coarse_mtime() {
@@ -1332,3 +1337,110 @@ fn bust_patched_dep() {
         )
         .run();
 }
+
+#[test]
+fn rebuild_on_mid_build_file_modification() {
+    let server = TcpListener::bind("127.0.0.1:0").unwrap();
+    let addr = server.local_addr().unwrap();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [workspace]
+            members = ["root", "proc_macro_dep"]
+        "#,
+        )
+        .file(
+            "root/Cargo.toml",
+            r#"
+            [project]
+            name = "root"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            proc_macro_dep = { path = "../proc_macro_dep" }
+        "#,
+        )
+        .file(
+            "root/src/lib.rs",
+            r#"
+            #[macro_use]
+            extern crate proc_macro_dep;
+
+            #[derive(Noop)]
+            pub struct X;
+        "#,
+        )
+        .file(
+            "proc_macro_dep/Cargo.toml",
+            r#"
+            [project]
+            name = "proc_macro_dep"
+            version = "0.1.0"
+            authors = []
+
+            [lib]
+            proc-macro = true
+        "#,
+        )
+        .file(
+            "proc_macro_dep/src/lib.rs",
+            &format!(
+                r#"
+                extern crate proc_macro;
+
+                use std::io::Read;
+                use std::net::TcpStream;
+                use proc_macro::TokenStream;
+
+                #[proc_macro_derive(Noop)]
+                pub fn noop(_input: TokenStream) -> TokenStream {{
+                    let mut stream = TcpStream::connect("{}").unwrap();
+                    let mut v = Vec::new();
+                    stream.read_to_end(&mut v).unwrap();
+                    "".parse().unwrap()
+                }}
+            "#,
+                addr
+            ),
+        )
+        .build();
+    let root = p.root();
+
+    let t = thread::spawn(move || {
+        let socket = server.accept().unwrap().0;
+        sleep_ms(1000);
+        let mut file = OpenOptions::new()
+            .write(true)
+            .append(true)
+            .open(root.join("root/src/lib.rs"))
+            .unwrap();
+        writeln!(file, "// modified").expect("Failed to append to root sources");
+        drop(file);
+        drop(socket);
+        drop(server.accept().unwrap());
+    });
+
+    p.cargo("build")
+        .with_stderr(
+            "\
+[COMPILING] proc_macro_dep v0.1.0 ([..]/proc_macro_dep)
+[COMPILING] root v0.1.0 ([..]/root)
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
+",
+        )
+        .run();
+
+    p.cargo("build")
+        .with_stderr(
+            "\
+[COMPILING] root v0.1.0 ([..]/root)
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
+",
+        )
+        .run();
+
+    t.join().ok().unwrap();
+}

tests/testsuite/path.rs

@@ -356,6 +356,7 @@ fn deep_dependencies_trigger_rebuild() {
     //
     // We base recompilation off mtime, so sleep for at least a second to ensure
     // that this write will change the mtime.
+    sleep_ms(1000);
     File::create(&p.root().join("baz/src/baz.rs"))
         .unwrap()
         .write_all(br#"pub fn baz() { println!("hello!"); }"#)
@@ -372,6 +373,7 @@ fn deep_dependencies_trigger_rebuild() {
         .run();
 
     // Make sure an update to bar doesn't trigger baz
+    sleep_ms(1000);
     File::create(&p.root().join("bar/src/bar.rs"))
         .unwrap()
         .write_all(