Merge pull request #131 from ddunbar/directory-contents-recomputation-take-2

[BuildSystem] Fix bug where directory contents were always recomputed.

Author: ddunbar

lib/BuildSystem/BuildSystem.cpp

@@ -681,7 +681,7 @@ class DirectoryContentsTask : public Task {
     if (info.isMissing()) {
       return value.isMissingInput();
     } else {
-      return value.isExistingInput() && value.getOutputInfo() == info;
+      return value.isDirectoryContents() && value.getOutputInfo() == info;
     }
   }
 };

tests/BuildSystem/Build/directory-contents.llbuild

@@ -0,0 +1,42 @@
+# Check that directory contents are rebuilt *only* when appropriate.
+#
+# RUN: rm -rf %t.build
+# RUN: mkdir -p %t.build
+# RUN: cp %s %t.build/build.llbuild
+# RUN: mkdir -p %t.build/dir
+# RUN: %{llbuild} buildsystem build --serial --trace %t.initial.trace --chdir %t.build
+# RUN: %{FileCheck} --check-prefix=CHECK-INITIAL --input-file=%t.initial.trace %s
+#
+# CHECK-INITIAL: { "new-rule", "[[RULE_NAME:R[0-9]+]]", "Ddir" },
+# CHECK-INITIAL: { "rule-needs-to-run", "[[RULE_NAME]]", "never-built" },
+
+
+# A null rebuild shouldn't rebuild the directory.
+#
+# RUN: %{llbuild} buildsystem build --serial --trace %t.rebuild.trace --chdir %t.build
+# RUN: %{FileCheck} --check-prefix=CHECK-REBUILD --input-file=%t.rebuild.trace %s
+#
+# CHECK-REBUILD: { "new-rule", "[[RULE_NAME:R[0-9]+]]", "Ddir" },
+# CHECK-REBUILD: { "rule-does-not-need-to-run", "[[RULE_NAME]]" },
+
+
+# A rebuild after adding a new file should rebuild the directory.
+#
+# RUN: touch %t.build/dir/file
+# RUN: %{llbuild} buildsystem build --serial --trace %t.modified.trace --chdir %t.build
+# RUN: %{FileCheck} --check-prefix=CHECK-MODIFIED --input-file=%t.modified.trace %s
+#
+# CHECK-MODIFIED: { "new-rule", "[[RULE_NAME:R[0-9]+]]", "Ddir" },
+# CHECK-MODIFIED: { "rule-needs-to-run", "[[RULE_NAME]]", "invalid-value" },
+
+client:
+  name: basic
+
+targets:
+  "": ["<all>"]
+
+commands:
+  C.all:
+    tool: phony
+    inputs: ["dir/"]
+    outputs: ["<all>"]

unittests/BuildSystem/BuildSystemTaskTests.cpp

@@ -31,6 +31,7 @@
 
 using namespace llvm;
 using namespace llbuild;
+using namespace llbuild::basic;
 using namespace llbuild::buildsystem;
 using namespace llbuild::unittests;
 
@@ -115,7 +116,8 @@ TEST(BuildSystemTaskTests, directoryContents) {
 /// Check the evaluation of directory signatures.
 TEST(BuildSystemTaskTests, directorySignature) {
   TmpDir tempDir{ __FUNCTION__ };
-
+  auto localFS = createLocalFileSystem();
+  
   // Create a directory with sample files.
   SmallString<256> fileA{ tempDir.str() };
   sys::path::append(fileA, "fileA");
@@ -159,25 +161,39 @@ TEST(BuildSystemTaskTests, directorySignature) {
   // Modify the immediate directory and rebuild.
   SmallString<256> fileC{ tempDir.str() };
   sys::path::append(fileC, "fileC");
-  {
+
+  // We must write the file in a loop until the directory actually changes (we
+  // currently assume we can trust the directory file info to detect changes,
+  // which is not always strictly true).
+  auto dirFileInfo = localFS->getFileInfo(tempDir.str());
+  do {
     std::error_code ec;
+    llvm::sys::fs::remove(fileC.str());
     llvm::raw_fd_ostream os(fileC, ec, llvm::sys::fs::F_Text);
     assert(!ec);
     os << "fileC";
-  }
+  } while (dirFileInfo == localFS->getFileInfo(tempDir.str()));
+  
   auto resultB = system.build(keyToBuild);
   ASSERT_TRUE(resultB.hasValue() && resultB->isDirectoryTreeSignature());
   ASSERT_TRUE(resultA->toData() != resultB->toData());
 
   // Modify the subdirectory and rebuild.
   SmallString<256> subdirFileD{ subdirA };
   sys::path::append(subdirFileD, "fileD");
-  {
+
+  // We must write the file in a loop until the directory actually changes (we
+  // currently assume we can trust the directory file info to detect changes,
+  // which is not always strictly true).
+  dirFileInfo = localFS->getFileInfo(subdirA.str());
+  do {
     std::error_code ec;
+    llvm::sys::fs::remove(subdirFileD.str());
     llvm::raw_fd_ostream os(subdirFileD, ec, llvm::sys::fs::F_Text);
     assert(!ec);
     os << "fileD";
-  }
+  } while (dirFileInfo == localFS->getFileInfo(subdirA.str()));
+  
   auto resultC = system.build(keyToBuild);
   ASSERT_TRUE(resultC.hasValue() && resultC->isDirectoryTreeSignature());
   ASSERT_TRUE(resultA->toData() != resultB->toData());