ensure state for all outputs of phony builds

If a build is missing its inputs or its outputs, we normally stop examining
disk state upon discovering the first missing file because we know we'll need
to execute it regardless.

But we don't execute phony builds, so they were skipping some logic that was
breaking an invariant: any step downstream of an output relies on that step
gathering the disk state (mtime) for that output.

For phony steps we expect outputs to be missing, and (due to working
around a Ninja bug) we also don't care about missing inputs(!).  Without
special casing, the "stop on first missing" file meant we wouldn't have
state for their outputs.

From a patch from [email protected], from #84.

src/graph.rs

@@ -362,6 +362,11 @@ impl FileState {
         self.0.set_grow(id, Some(mtime), None);
         Ok(mtime)
     }
+
+    /// Set a file to Missing.  This is used for outputs of phony rules.
+    pub fn set_missing(&mut self, id: FileId) {
+        self.0.set_grow(id, Some(MTime::Missing), None);
+    }
 }
 
 #[derive(Default)]

src/work.rs

@@ -513,24 +513,10 @@ impl<'a> Work<'a> {
     /// Otherwise returns the missing id if any expected but not required files,
     /// e.g. outputs, are missing, implying that the build needs to be executed.
     fn check_build_files_missing(&mut self, id: BuildId) -> anyhow::Result<Option<FileId>> {
-        let phony = self.graph.builds[id].cmdline.is_none();
-        // TODO: do we just return true immediately if phony?
-        // There are likely weird interactions with builds that depend on
-        // a phony output, despite that not really making sense.
-
-        // True if we need to work around
-        //   https://github.com/ninja-build/ninja/issues/1779
-        // which is a bug that a phony rule with a missing input
-        // dependency doesn't fail the build.
-        // TODO: key this behavior off of the "ninja compat" flag.
-        // TODO: reconsider how phony deps work, maybe we should always promote
-        // phony deps to order-only?
-        let workaround_missing_phony_deps = phony;
-
         // Ensure we have state for all input files.
         if let Some(missing) = self.ensure_input_files(id, false)? {
             let file = self.graph.file(missing);
-            if file.input.is_none() && !workaround_missing_phony_deps {
+            if file.input.is_none() {
                 let build = &self.graph.builds[id];
                 anyhow::bail!("{}: input {} missing", build.location, file.name);
             }
@@ -564,21 +550,44 @@ impl<'a> Work<'a> {
         Ok(None)
     }
 
+    /// Like check_build_files_missing, but for phony rules, which have
+    /// different behavior for both inputs and outputs.
+    fn check_build_files_missing_phony(&mut self, id: BuildId) {
+        // We don't consider the input files.  This works around
+        //   https://github.com/ninja-build/ninja/issues/1779
+        // which is a bug that a phony rule with a missing input
+        // dependency doesn't fail the build.
+        // TODO: key this behavior off of the "ninja compat" flag.
+        // TODO: reconsider how phony deps work, maybe we should always promote
+        // phony deps to order-only?
+
+        // Maintain the invariant that we have stat info for all outputs.
+        // This build didn't run anything so the output is not expected to
+        // exist.
+        // TODO: what should happen if a rule uses a phony output as its own input?
+        // The Ninja manual suggests you can use phony rules to aggregate outputs
+        // together, so we might need to create some sort of fake mtime here?
+        let build = &self.graph.builds[id];
+        for &id in build.outs() {
+            self.file_state.set_missing(id);
+        }
+    }
+
     /// Check a ready build for whether it needs to run, returning true if so.
     /// Prereq: any dependent input is already generated.
     fn check_build_dirty(&mut self, id: BuildId) -> anyhow::Result<bool> {
-        let file_missing = self.check_build_files_missing(id)?;
-
         let build = &self.graph.builds[id];
-
-        // A phony build can never be dirty.
         let phony = build.cmdline.is_none();
-        if phony {
-            return Ok(false);
-        }
+        let file_missing = if phony {
+            self.check_build_files_missing_phony(id);
+            return Ok(false); // Do not need to run.
+        } else {
+            self.check_build_files_missing(id)?
+        };
 
         // If any files are missing, the build is dirty without needing
         // to consider hashes.
+        let build = &self.graph.builds[id];
         if let Some(missing) = file_missing {
             if self.options.explain {
                 self.progress.log(&format!(

tests/e2e/basic.rs

@@ -292,3 +292,26 @@ build out: custom
     assert_output_contains(&out, "echo 123 hello 123");
     Ok(())
 }
+
+// Repro for issue #84.
+#[test]
+fn phony_depends() -> anyhow::Result<()> {
+    let space = TestSpace::new()?;
+    space.write(
+        "build.ninja",
+        &[
+            "
+rule touch
+  command = touch outfile",
+            "
+build out1: touch
+build out2: phony out1
+build out3: phony out2
+",
+        ]
+        .join("\n"),
+    )?;
+    space.run_expect(&mut n2_command(vec!["out3"]))?;
+    space.read("outfile")?;
+    Ok(())
+}