Merge pull request #157 from ddunbar/change-to-keyid-result-v2
Change DB API to find constant key IDs.
diff --git a/docs/buildsystem.rst b/docs/buildsystem.rst
index eb82dd8..f5447fb 100644
--- a/docs/buildsystem.rst
+++ b/docs/buildsystem.rst
@@ -689,3 +689,25 @@
FIXME: currently the archive is always recreated entirely, it would be
preferable in future to correctly update/delete/create the archive file
as required.
+
+Stale File Removal Tool
+-----------------------
+
+**Identifier**: *stale-file-removal*
+
+A tool to remove stale files from previous builds.
+
+The build system records the last value of `expectedOutputs` and compares it
+to the current one. Any path that was previously present, but isn't in the
+current string list will be removed from the file system.
+
+.. list-table::
+ :header-rows: 1
+ :widths: 20 80
+
+ * - Name
+ - Description
+
+ * - expectedOutputs
+ - A string list of paths that are expected to be produced by the given
+ manifest.
diff --git a/include/llbuild/Basic/FileSystem.h b/include/llbuild/Basic/FileSystem.h
index ab47294..055de7d 100644
--- a/include/llbuild/Basic/FileSystem.h
+++ b/include/llbuild/Basic/FileSystem.h
@@ -59,6 +59,13 @@
/// \returns The file contents, on success, or null on error.
virtual std::unique_ptr<llvm::MemoryBuffer>
getFileContents(const std::string& path) = 0;
+
+ /// Remove the file or directory at the given path.
+ ///
+ /// Directory removal is non-recursive, so only empty directories will be removed.
+ ///
+ /// \returns True if the item was removed, false otherwise.
+ virtual bool remove(const std::string& path) = 0;
/// Get the information to represent the state of the given path in the file
/// system.
diff --git a/include/llbuild/Basic/PlatformUtility.h b/include/llbuild/Basic/PlatformUtility.h
index c78d4a9..1fd4851 100644
--- a/include/llbuild/Basic/PlatformUtility.h
+++ b/include/llbuild/Basic/PlatformUtility.h
@@ -38,6 +38,7 @@
int pipe(int ptHandles[2]);
FILE *popen(const char *command, const char *mode);
int read(int fileHandle, void *destinationBuffer, unsigned int maxCharCount);
+int rmdir(const char *path);
int stat(const char *fileName, StatStruct *buf);
int unlink(const char *fileName);
int write(int fileHandle, void *destinationBuffer, unsigned int maxCharCount);
diff --git a/include/llbuild/BuildSystem/BuildValue.h b/include/llbuild/BuildSystem/BuildValue.h
index 47c8129..d96cb04 100644
--- a/include/llbuild/BuildSystem/BuildValue.h
+++ b/include/llbuild/BuildSystem/BuildValue.h
@@ -57,6 +57,9 @@
/// The signature of a directories contents.
DirectoryTreeSignature,
+ /// A value produced by stale file removal.
+ StaleFileRemoval,
+
/// A value produced by a command which succeeded, but whose output was
/// missing.
MissingOutput,
@@ -123,7 +126,7 @@
}
bool kindHasStringList() const {
- return isDirectoryContents();
+ return isDirectoryContents() || isStaleFileRemoval();
}
bool kindHasOutputInfo() const {
@@ -175,6 +178,23 @@
stringValues.size = size;
}
+ BuildValue(Kind kind, ArrayRef<std::string> values) : kind(kind) {
+ // Construct the concatenated data.
+ uint64_t size = 0;
+ for (auto value: values) {
+ size += value.size() + 1;
+ }
+ char *p, *contents = p = new char[size];
+ for (auto value: values) {
+ assert(value.find('\0') == StringRef::npos);
+ memcpy(p, value.data(), value.size());
+ p += value.size();
+ *p++ = '\0';
+ }
+ stringValues.contents = contents;
+ stringValues.size = size;
+ }
+
std::vector<StringRef> getStringListValues() const {
assert(kindHasStringList());
@@ -296,6 +316,9 @@
static BuildValue makeTarget() {
return BuildValue(Kind::Target);
}
+ static BuildValue makeStaleFileRemoval(ArrayRef<std::string> values) {
+ return BuildValue(Kind::StaleFileRemoval, values);
+ }
/// @}
@@ -311,6 +334,7 @@
bool isDirectoryTreeSignature() const {
return kind == Kind::DirectoryTreeSignature;
}
+ bool isStaleFileRemoval() const { return kind == Kind::StaleFileRemoval; }
bool isMissingOutput() const { return kind == Kind::MissingOutput; }
bool isFailedInput() const { return kind == Kind::FailedInput; }
@@ -327,6 +351,11 @@
assert(isDirectoryContents() && "invalid call for value kind");
return getStringListValues();
}
+
+ std::vector<StringRef> getStaleFileList() const {
+ assert(isStaleFileRemoval() && "invalid call for value kind");
+ return getStringListValues();
+ }
uint64_t getDirectoryTreeSignature() const {
assert(isDirectoryTreeSignature() && "invalid call for value kind");
diff --git a/lib/Basic/FileSystem.cpp b/lib/Basic/FileSystem.cpp
index 39dc7fe..7ab3b17 100644
--- a/lib/Basic/FileSystem.cpp
+++ b/lib/Basic/FileSystem.cpp
@@ -62,6 +62,30 @@
}
return std::unique_ptr<llvm::MemoryBuffer>(result->release());
}
+
+ virtual bool remove(const std::string& path) override {
+ // Assume `path` is a regular file.
+ if (sys::unlink(path.c_str()) == 0) {
+ return true;
+ }
+
+ // Error can't be that `path` is actually a directory.
+ if (errno != EPERM) {
+ return false;
+ }
+
+ // Check if `path` is a directory.
+ struct stat statbuf;
+ if (sys::stat(path.c_str(), &statbuf) != 0) {
+ return false;
+ }
+
+ if (S_ISDIR(statbuf.st_mode)) {
+ return sys::rmdir(path.c_str()) == 0;
+ }
+
+ return false;
+ }
virtual FileInfo getFileInfo(const std::string& path) override {
return FileInfo::getInfoForPath(path);
diff --git a/lib/Basic/PlatformUtility.cpp b/lib/Basic/PlatformUtility.cpp
index 0ebd529..60e4f40 100644
--- a/lib/Basic/PlatformUtility.cpp
+++ b/lib/Basic/PlatformUtility.cpp
@@ -88,6 +88,14 @@
#endif
}
+int sys::rmdir(const char *path) {
+#if defined(_WIN32)
+ return ::_rmdir(path);
+#else
+ return ::rmdir(path);
+#endif
+}
+
int sys::stat(const char *fileName, StatStruct *buf) {
#if defined(_WIN32)
return ::_stat(fileName, buf);
diff --git a/lib/BuildSystem/BuildSystem.cpp b/lib/BuildSystem/BuildSystem.cpp
index 8cdeb54..9b4b137 100644
--- a/lib/BuildSystem/BuildSystem.cpp
+++ b/lib/BuildSystem/BuildSystem.cpp
@@ -46,6 +46,7 @@
#include <memory>
#include <mutex>
+#include <unordered_set>
#include <unistd.h>
@@ -143,10 +144,11 @@
/// The internal schema version.
///
/// Version History:
+ /// * 7: Added StaleFileRemoval to BuildValue
/// * 6: Added DirectoryContents to BuildKey
/// * 5: Switch BuildValue to be BinaryCoding based
/// * 4: Pre-history
- static const uint32_t internalSchemaVersion = 6;
+ static const uint32_t internalSchemaVersion = 7;
BuildSystem& buildSystem;
@@ -2297,6 +2299,182 @@
}
};
+#pragma mark - StaleFileRemovalTool implementation
+
+class StaleFileRemovalCommand : public Command {
+ std::string description;
+
+ std::vector<std::string> expectedOutputs;
+ std::vector<std::string> filesToDelete;
+ bool computedFilesToDelete = false;
+
+ BuildValue priorValue;
+ bool hasPriorResult = false;
+
+ virtual void configureDescription(const ConfigureContext&, StringRef value) override {
+ description = value;
+ }
+
+ virtual void getShortDescription(SmallVectorImpl<char> &result) override {
+ llvm::raw_svector_ostream(result) << (description.empty() ? "Stale file removal" : description);
+ }
+
+ virtual void getVerboseDescription(SmallVectorImpl<char> &result) override {
+ computeFilesToDelete();
+
+ getShortDescription(result);
+ llvm::raw_svector_ostream(result) << ", stale files: [";
+ for (auto fileToDelete : filesToDelete) {
+ llvm::raw_svector_ostream(result) << fileToDelete;
+ if (fileToDelete != *(--filesToDelete.end())) {
+ llvm::raw_svector_ostream(result) << ", ";
+ }
+ }
+ llvm::raw_svector_ostream(result) << "]";
+ }
+
+ virtual void configureInputs(const ConfigureContext& ctx,
+ const std::vector<Node*>& value) override {}
+
+ virtual void configureOutputs(const ConfigureContext& ctx,
+ const std::vector<Node*>& value) override {}
+
+ virtual bool configureAttribute(const ConfigureContext& ctx, StringRef name,
+ StringRef value) override {
+ // No supported attributes.
+ ctx.error("unexpected attribute: '" + name + "'");
+ return false;
+ }
+ virtual bool configureAttribute(const ConfigureContext& ctx, StringRef name,
+ ArrayRef<StringRef> values) override {
+ if (name == "expectedOutputs") {
+ expectedOutputs.reserve(values.size());
+ for (auto value : values) {
+ expectedOutputs.emplace_back(value.str());
+ }
+ return true;
+ }
+
+ ctx.error("unexpected attribute: '" + name + "'");
+ return false;
+ }
+ virtual bool configureAttribute(const ConfigureContext& ctx, StringRef name,
+ ArrayRef<std::pair<StringRef, StringRef>> values) override {
+ // No supported attributes.
+ ctx.error("unexpected attribute: '" + name + "'");
+ return false;
+ }
+
+ virtual BuildValue getResultForOutput(Node* node,
+ const BuildValue& value) override {
+ // If the value was a failed command, propagate the failure.
+ if (value.isFailedCommand() || value.isPropagatedFailureCommand() ||
+ value.isCancelledCommand())
+ return BuildValue::makeFailedInput();
+ if (value.isSkippedCommand())
+ return BuildValue::makeSkippedCommand();
+
+ // Otherwise, this was successful, return the value as-is.
+ return BuildValue::fromData(value.toData());;
+ }
+
+ virtual bool isResultValid(BuildSystem& system,
+ const BuildValue& value) override {
+ // Always re-run stale file removal.
+ return false;
+ }
+
+ virtual void start(BuildSystemCommandInterface& bsci,
+ core::Task* task) override {}
+
+ virtual void providePriorValue(BuildSystemCommandInterface&, core::Task*,
+ const BuildValue& value) override {
+ hasPriorResult = true;
+ priorValue = BuildValue::fromData(value.toData());
+ }
+
+ virtual void provideValue(BuildSystemCommandInterface&, core::Task*,
+ uintptr_t inputID,
+ const BuildValue& value) override {
+ assert(0 && "unexpected API call");
+ }
+
+ void computeFilesToDelete() {
+ if (computedFilesToDelete) {
+ return;
+ }
+
+ std::vector<StringRef> priorValueList = priorValue.getStaleFileList();
+ std::unordered_set<std::string> priorNodes(priorValueList.begin(), priorValueList.end());
+ std::unordered_set<std::string> expectedNodes(expectedOutputs.begin(), expectedOutputs.end());
+
+ std::set_difference(priorNodes.begin(), priorNodes.end(),
+ expectedNodes.begin(), expectedNodes.end(),
+ std::back_inserter(filesToDelete));
+
+ computedFilesToDelete = true;
+ }
+
+ virtual BuildValue execute(BuildSystemCommandInterface& bsci,
+ core::Task* task,
+ QueueJobContext* context) override {
+ // Nothing to do if we do not have a prior result.
+ if (!hasPriorResult || !priorValue.isStaleFileRemoval()) {
+ bsci.getDelegate().commandStarted(this);
+ bsci.getDelegate().commandFinished(this);
+ return BuildValue::makeStaleFileRemoval(expectedOutputs);
+ }
+
+ computeFilesToDelete();
+
+ bsci.getDelegate().commandStarted(this);
+
+ for (auto fileToDelete : filesToDelete) {
+ if (!getBuildSystem(bsci.getBuildEngine()).getDelegate().getFileSystem().remove(fileToDelete)) {
+ bsci.getDelegate().error("", {}, (Twine("cannot remove stale file '") + fileToDelete + "': " + strerror(errno)));
+ }
+ }
+
+ bsci.getDelegate().commandFinished(this);
+
+ // Complete with a successful result.
+ return BuildValue::makeStaleFileRemoval(expectedOutputs);
+ }
+
+public:
+ StaleFileRemovalCommand(const StringRef name)
+ : Command(name), priorValue(BuildValue::makeInvalid()) {}
+};
+
+class StaleFileRemovalTool : public Tool {
+public:
+ using Tool::Tool;
+
+ virtual bool configureAttribute(const ConfigureContext& ctx, StringRef name,
+ StringRef value) override {
+ // No supported attributes.
+ ctx.error("unexpected attribute: '" + name + "'");
+ return false;
+ }
+ virtual bool configureAttribute(const ConfigureContext& ctx, StringRef name,
+ ArrayRef<StringRef> values) override {
+ // No supported attributes.
+ ctx.error("unexpected attribute: '" + name + "'");
+ return false;
+ }
+ virtual bool configureAttribute(
+ const ConfigureContext& ctx, StringRef name,
+ ArrayRef<std::pair<StringRef, StringRef>> values) override {
+ // No supported attributes.
+ ctx.error("unexpected attribute: '" + name + "'");
+ return false;
+ }
+
+ virtual std::unique_ptr<Command> createCommand(StringRef name) override {
+ return llvm::make_unique<StaleFileRemovalCommand>(name);
+ }
+};
+
#pragma mark - BuildSystemFileDelegate
BuildSystemDelegate& BuildSystemFileDelegate::getSystemDelegate() {
@@ -2354,6 +2532,8 @@
return llvm::make_unique<SymlinkTool>(name);
} else if (name == "archive") {
return llvm::make_unique<ArchiveTool>(name);
+ } else if (name == "stale-file-removal") {
+ return llvm::make_unique<StaleFileRemovalTool>(name);
}
return nullptr;
diff --git a/lib/BuildSystem/BuildValue.cpp b/lib/BuildSystem/BuildValue.cpp
index 27e1abf..651e47f 100644
--- a/lib/BuildSystem/BuildValue.cpp
+++ b/lib/BuildSystem/BuildValue.cpp
@@ -36,6 +36,7 @@
CASE(CancelledCommand);
CASE(SkippedCommand);
CASE(Target);
+ CASE(StaleFileRemoval);
#undef CASE
}
return "<unknown>";
diff --git a/products/libllbuild/BuildSystem-C-API.cpp b/products/libllbuild/BuildSystem-C-API.cpp
index 2b58f71..c5262b8 100644
--- a/products/libllbuild/BuildSystem-C-API.cpp
+++ b/products/libllbuild/BuildSystem-C-API.cpp
@@ -82,6 +82,14 @@
return result;
}
+ virtual bool remove(const std::string& path) override {
+ if (!cAPIDelegate.fs_remove) {
+ return localFileSystem->remove(path);
+ }
+
+ return cAPIDelegate.fs_remove(cAPIDelegate.context, path.c_str());
+ }
+
virtual basic::FileInfo getFileInfo(const std::string& path) override {
if (!cAPIDelegate.fs_get_file_info) {
return localFileSystem->getFileInfo(path);
diff --git a/products/libllbuild/public-api/llbuild/buildsystem.h b/products/libllbuild/public-api/llbuild/buildsystem.h
index f8c7c50..3935feb 100644
--- a/products/libllbuild/public-api/llbuild/buildsystem.h
+++ b/products/libllbuild/public-api/llbuild/buildsystem.h
@@ -215,6 +215,13 @@
bool (*fs_get_file_contents)(void* context, const char* path,
llb_data_t* data_out);
+ /// Remove file or directory at given path.
+ ///
+ /// Xparam path The path to remove.
+ ///
+ /// \\returns True on success.
+ bool (*fs_remove)(void* context, const char* path);
+
/// Get the file information for the given path.
void (*fs_get_file_info)(void* context, const char* path,
llb_fs_file_info_t* data_out);
diff --git a/tests/BuildSystem/Build/stale-file-removal.llbuild b/tests/BuildSystem/Build/stale-file-removal.llbuild
new file mode 100644
index 0000000..125b1e9
--- /dev/null
+++ b/tests/BuildSystem/Build/stale-file-removal.llbuild
@@ -0,0 +1,57 @@
+# Check the handling of stale file removal.
+#
+# We use 'grep' to slice out two different subfiles from the same file.
+#
+# RUN: rm -rf %t.build
+# RUN: mkdir -p %t.build
+#
+# RUN: grep -A1000 "VERSION-BEGIN-[1]" %s | grep -B10000 "VERSION-END-1" | grep -ve '^--$' > %t.build/build-1.llbuild
+# RUN: %{llbuild} buildsystem build --serial --chdir %t.build -f build-1.llbuild > %t.1.out
+# RUN: %{FileCheck} --check-prefix CHECK-VERSION-1 --input-file=%t.1.out %s
+#
+# CHECK-VERSION-1: STALE-FILE-REMOVAL
+#
+# RUN: grep -A1000 "VERSION-BEGIN-[2]" %s | grep -B10000 "VERSION-END-2" | grep -ve '^--$' > %t.build/build-2.llbuild
+# Create stale file
+# RUN: touch %t.build/a.out
+# RUN: %{llbuild} buildsystem build --serial --chdir %t.build -f build-2.llbuild > %t2.out
+# RUN: %{FileCheck} --check-prefix CHECK-VERSION-2 --input-file %t2.out %s
+# Check for absence of stale file
+# RUN: test ! -f %t.build/a.out
+#
+# CHECK-VERSION-2: STALE-FILE-REMOVAL-2
+#
+
+##### VERSION-BEGIN-1 #####
+
+client:
+ name: basic
+
+targets:
+ "": ["<all>"]
+
+commands:
+ C.1:
+ tool: stale-file-removal
+ description: STALE-FILE-REMOVAL
+ expectedOutputs: ["a.out"]
+ outputs: ["<all>"]
+
+##### VERSION-END-1 #####
+
+##### VERSION-BEGIN-2 #####
+
+client:
+ name: basic
+
+targets:
+ "": ["<all>"]
+
+commands:
+ C.1:
+ tool: stale-file-removal
+ description: STALE-FILE-REMOVAL-2
+ expectedOutputs: ["b.out"]
+ outputs: ["<all>"]
+
+##### VERSION-END-2 #####
diff --git a/unittests/BuildSystem/BuildSystemTaskTests.cpp b/unittests/BuildSystem/BuildSystemTaskTests.cpp
index f907fab..2ecde55 100644
--- a/unittests/BuildSystem/BuildSystemTaskTests.cpp
+++ b/unittests/BuildSystem/BuildSystemTaskTests.cpp
@@ -324,4 +324,87 @@
}), delegate.getMessages());
}
+// Tests the behaviour of StaleFileRemovalTool
+TEST(BuildSystemTaskTests, staleFileRemoval) {
+ TmpDir tempDir{ __FUNCTION__ };
+
+ SmallString<256> manifest{ tempDir.str() };
+ sys::path::append(manifest, "manifest.llbuild");
+ {
+ std::error_code ec;
+ llvm::raw_fd_ostream os(manifest, ec, llvm::sys::fs::F_Text);
+ assert(!ec);
+
+ os << R"END(
+client:
+ name: mock
+
+commands:
+ C.1:
+ tool: stale-file-removal
+ description: STALE-FILE-REMOVAL
+ expectedOutputs: ["a.out"]
+)END";
+ }
+
+ auto keyToBuild = BuildKey::makeCommand("C.1");
+ MockBuildSystemDelegate delegate(/*trackAllMessages=*/true);
+ BuildSystem system(delegate);
+
+ SmallString<256> builddb{ tempDir.str() };
+ sys::path::append(builddb, "build.db");
+ system.attachDB(builddb.c_str(), nullptr);
+
+ bool loadingResult = system.loadDescription(manifest);
+ ASSERT_TRUE(loadingResult);
+
+ auto result = system.build(keyToBuild);
+
+ ASSERT_TRUE(result.getValue().isStaleFileRemoval());
+ ASSERT_EQ(result.getValue().getStaleFileList().size(), 1UL);
+ ASSERT_TRUE(strcmp(result.getValue().getStaleFileList()[0].str().c_str(), "a.out") == 0);
+
+ ASSERT_EQ(std::vector<std::string>({
+ "commandPreparing(C.1)",
+ "commandStarted(C.1)",
+ "commandFinished(C.1)",
+ }), delegate.getMessages());
+
+ {
+ std::error_code ec;
+ llvm::raw_fd_ostream os(manifest, ec, llvm::sys::fs::F_Text);
+ assert(!ec);
+
+ os << R"END(
+client:
+ name: mock
+
+commands:
+ C.1:
+ tool: stale-file-removal
+ description: STALE-FILE-REMOVAL
+ expectedOutputs: ["b.out"]
+)END";
+ }
+
+ MockBuildSystemDelegate delegate2(/*trackAllMessages=*/true);
+ BuildSystem system2(delegate2);
+ system2.attachDB(builddb.c_str(), nullptr);
+ loadingResult = system2.loadDescription(manifest);
+ ASSERT_TRUE(loadingResult);
+ result = system2.build(keyToBuild);
+
+ ASSERT_TRUE(result.getValue().isStaleFileRemoval());
+ ASSERT_EQ(result.getValue().getStaleFileList().size(), 1UL);
+ ASSERT_TRUE(strcmp(result.getValue().getStaleFileList()[0].str().c_str(), "b.out") == 0);
+
+ ASSERT_EQ(std::vector<std::string>({
+ "commandPreparing(C.1)",
+ "commandStarted(C.1)",
+ // FIXME: Maybe it's worth creating a virtual FileSystem implementation and checking if `remove` has been called
+ "cannot remove stale file 'a.out': No such file or directory",
+ "commandFinished(C.1)",
+ }), delegate2.getMessages());
+}
+
}
diff --git a/unittests/BuildSystem/BuildValueTest.cpp b/unittests/BuildSystem/BuildValueTest.cpp
index 8fa399a..70ec4e7 100644
--- a/unittests/BuildSystem/BuildValueTest.cpp
+++ b/unittests/BuildSystem/BuildValueTest.cpp
@@ -161,4 +161,25 @@
}
}
+TEST(BuildValueTest, staleFileRemovalValues) {
+ std::vector<std::string> files { "a.out", "Info.plist" };
+
+ {
+ BuildValue a = BuildValue::makeStaleFileRemoval(ArrayRef<std::string>(files));
+ auto nodes = a.getStaleFileList();
+ EXPECT_EQ(2UL, nodes.size());
+ EXPECT_EQ(nodes[0], "a.out");
+ EXPECT_EQ(nodes[1], "Info.plist");
+ }
+
+ {
+ BuildValue a = BuildValue::makeStaleFileRemoval(ArrayRef<std::string>(files));
+ BuildValue b = BuildValue::fromData(a.toData());
+ auto nodes = b.getStaleFileList();
+ EXPECT_EQ(2UL, nodes.size());
+ EXPECT_EQ(nodes[0], "a.out");
+ EXPECT_EQ(nodes[1], "Info.plist");
+ }
+}
+
}
