Merge pull request #64 from m-i-r-z-a/command-skipping

Support command skipping with shouldCommandStart.

include/llbuild/BuildSystem/BuildSystem.h

@@ -151,6 +151,21 @@ class BuildSystemDelegate {
   /// commands).
   virtual void commandPreparing(Command*) = 0;
 
+  /// Called by the build system to allow the delegate to skip a command without
+  /// implicitly skipping its dependents.
+  ///
+  /// WARNING: Clients need to take special care when using this. Skipping
+  /// commands without considering their dependencies or dependents can easily
+  /// produce an inconsistent build.
+  ///
+  /// This method is called before the command starts, when the system has
+  /// identified that it will eventually need to run (after all of its inputs
+  /// have been satisfied).
+  ///
+  /// The system guarantees that all such calls will be paired with a
+  /// corresponding \see commandFinished() call.
+  virtual bool shouldCommandStart(Command*) = 0;
+
   /// Called by the build system to report that a declared command has started.
   ///
   /// The system guarantees that all such calls will be paired with a

include/llbuild/BuildSystem/BuildSystemFrontend.h

@@ -167,6 +167,21 @@ class BuildSystemFrontendDelegate : public BuildSystemDelegate {
   /// corresponding \see commandFinished() call.
   virtual void commandPreparing(Command*) override;
 
+  /// Called by the build system to allow the delegate to skip a command without
+  /// implicitly skipping its dependents.
+  ///
+  /// WARNING: Clients need to take special care when using this. Skipping
+  /// commands without considering their dependencies or dependents can easily
+  /// produce an inconsistent build.
+  ///
+  /// This method is called before the command starts, when the system has
+  /// identified that it will eventually need to run (after all of its inputs
+  /// have been satisfied).
+  ///
+  /// The system guarantees that all such calls will be paired with a
+  /// corresponding \see commandFinished() call.
+  virtual bool shouldCommandStart(Command*) override;
+
   /// Called by the build system to report that a declared command has started.
   ///
   /// The system guarantees that all such calls will be paired with a

include/llbuild/BuildSystem/BuildValue.h

@@ -50,15 +50,22 @@ class BuildValue {
     /// missing.
     MissingOutput,
 
-    /// A value for a produced output whose command failed.
+    /// A value for a produced output whose command failed or was cancelled.
     FailedInput,
 
     /// A value produced by a successful command.
     SuccessfulCommand,
 
     /// A value produced by a failing command.
     FailedCommand,
-
+
+    /// A value produced by a command which was skipped because one of its
+    /// dependencies failed.
+    PropagatedFailureCommand,
+
+    /// A value produced by a command which was cancelled.
+    CancelledCommand,
+
     /// A value produced by a command which was skipped.
     SkippedCommand,
 
@@ -174,6 +181,12 @@ class BuildValue {
   static BuildValue makeFailedCommand() {
     return BuildValue(Kind::FailedCommand);
   }
+  static BuildValue makePropagatedFailureCommand() {
+    return BuildValue(Kind::PropagatedFailureCommand);
+  }
+  static BuildValue makeCancelledCommand() {
+    return BuildValue(Kind::CancelledCommand);
+  }
   static BuildValue makeSkippedCommand() {
     return BuildValue(Kind::SkippedCommand);
   }
@@ -195,6 +208,8 @@ class BuildValue {
   bool isFailedInput() const { return kind == Kind::FailedInput; }
   bool isSuccessfulCommand() const {return kind == Kind::SuccessfulCommand; }
   bool isFailedCommand() const { return kind == Kind::FailedCommand; }
+  bool isPropagatedFailureCommand() const { return kind == Kind::PropagatedFailureCommand; }
+  bool isCancelledCommand() const { return kind == Kind::CancelledCommand; }
   bool isSkippedCommand() const { return kind == Kind::SkippedCommand; }
   bool isTarget() const { return kind == Kind::Target; }
 

include/llbuild/BuildSystem/ExternalCommand.h

@@ -15,7 +15,9 @@
 
 #include "llbuild/BuildSystem/BuildFile.h"
 #include "llbuild/BuildSystem/BuildSystem.h"
+#include "llbuild/BuildSystem/BuildValue.h"
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 
 #include <string>
@@ -65,8 +67,8 @@ class ExternalCommand : public Command {
   /// The previous build result command signature, if available.
   uint64_t priorResultCommandSignature;
 
-  /// If true, the command should be skipped (because of an error in an input).
-  bool shouldSkip = false;
+  /// If not None, the command should be skipped with the provided BuildValue.
+  llvm::Optional<BuildValue> skipValue;
 
   /// If true, the command had a missing input (this implies ShouldSkip is
   /// true).

lib/BuildSystem/BuildSystem.cpp

@@ -128,7 +128,7 @@ class BuildSystemEngineDelegate : public BuildEngineDelegate {
 
 class BuildSystemImpl : public BuildSystemCommandInterface {
   /// The internal schema version.
-  static const uint32_t internalSchemaVersion = 1;
+  static const uint32_t internalSchemaVersion = 3;
 
   BuildSystem& buildSystem;
 
@@ -534,7 +534,23 @@ class CommandTask : public Task {
   }
 
   virtual void inputsAvailable(BuildEngine& engine) override {
-    command.inputsAvailable(getBuildSystem(engine).getCommandInterface(), this);
+    auto& bsci = getBuildSystem(engine).getCommandInterface();
+    auto fn = [this, &bsci=bsci](QueueJobContext* context) {
+      bool shouldSkip = !bsci.getDelegate().shouldCommandStart(&command);
+
+      if (shouldSkip) {
+        // We need to call commandFinished here because commandPreparing and
+        // shouldCommandStart guarantee that they're followed by
+        // commandFinished.
+        bsci.getDelegate().commandFinished(&command);
+
+        bsci.taskIsComplete(this, BuildValue::makeSkippedCommand());
+      } else {
+        command.inputsAvailable(bsci, this);
+      }
+    };
+
+    bsci.addJob({ &command, std::move(fn) });
   }
 
 public:
@@ -1480,8 +1496,9 @@ class MkdirCommand : public Command {
 
   virtual BuildValue getResultForOutput(Node* node,
                                         const BuildValue& value) override {
-    // If the value was a failed or skipped command, propagate the failure.
-    if (value.isFailedCommand() || value.isSkippedCommand())
+    // If the value was a failed command, propagate the failure.
+    if (value.isFailedCommand() || value.isPropagatedFailureCommand() ||
+        value.isCancelledCommand())
       return BuildValue::makeFailedInput();
 
     // Otherwise, we should have a successful command -- return the actual
@@ -1549,7 +1566,7 @@ class MkdirCommand : public Command {
                                core::Task* task) override {
     // If the build should cancel, do nothing.
     if (bsci.getDelegate().isCancelled()) {
-      bsci.taskIsComplete(task, BuildValue::makeSkippedCommand());
+      bsci.taskIsComplete(task, BuildValue::makeCancelledCommand());
       return;
     }
 
@@ -1718,8 +1735,9 @@ class SymlinkCommand : public Command {
 
   virtual BuildValue getResultForOutput(Node* node,
                                         const BuildValue& value) override {
-    // If the value was a failed or skipped command, propagate the failure.
-    if (value.isFailedCommand() || value.isSkippedCommand())
+    // If the value was a failed command, propagate the failure.
+    if (value.isFailedCommand() || value.isPropagatedFailureCommand() ||
+        value.isCancelledCommand())
       return BuildValue::makeFailedInput();
 
     // Otherwise, we should have a successful command -- return the actual
@@ -1774,7 +1792,7 @@ class SymlinkCommand : public Command {
                                core::Task* task) override {
     // If the build should cancel, do nothing.
     if (bsci.getDelegate().isCancelled()) {
-      bsci.taskIsComplete(task, BuildValue::makeSkippedCommand());
+      bsci.taskIsComplete(task, BuildValue::makeCancelledCommand());
       return;
     }
 

lib/BuildSystem/BuildSystemFrontend.cpp

@@ -359,6 +359,10 @@ void BuildSystemFrontendDelegate::commandStatusChanged(Command*,
 void BuildSystemFrontendDelegate::commandPreparing(Command*) {
 }
 
+bool BuildSystemFrontendDelegate::shouldCommandStart(Command*) {
+  return true;
+}
+
 void BuildSystemFrontendDelegate::commandStarted(Command* command) {
   // Don't report status if opted out by the command.
   if (!command->shouldShowStatus()) {

lib/BuildSystem/BuildValue.cpp

@@ -30,6 +30,8 @@ StringRef BuildValue::stringForKind(BuildValue::Kind kind) {
     CASE(FailedInput);
     CASE(SuccessfulCommand);
     CASE(FailedCommand);
+    CASE(PropagatedFailureCommand);
+    CASE(CancelledCommand);
     CASE(SkippedCommand);
     CASE(Target);
 #undef CASE

lib/BuildSystem/ExternalCommand.cpp

@@ -117,9 +117,12 @@ bool ExternalCommand::configureAttribute(
 
 BuildValue ExternalCommand::
 getResultForOutput(Node* node, const BuildValue& value) {
-  // If the value was a failed or skipped command, propagate the failure.
-  if (value.isFailedCommand() || value.isSkippedCommand())
+  // If the value was a failed or cancelled command, propagate the failure.
+  if (value.isFailedCommand() || value.isPropagatedFailureCommand() ||
+      value.isCancelledCommand())
     return BuildValue::makeFailedInput();
+  if (value.isSkippedCommand())
+      return BuildValue::makeSkippedCommand();
 
   // Otherwise, we should have a successful command -- return the actual
   // result for the output.
@@ -198,7 +201,7 @@ void ExternalCommand::start(BuildSystemCommandInterface& bsci,
   bsci.getDelegate().commandPreparing(this);
 
   // Initialize the build state.
-  shouldSkip = false;
+  skipValue = llvm::None;
   hasMissingInput = false;
 
   // Request all of the inputs.
@@ -227,13 +230,13 @@ void ExternalCommand::provideValue(BuildSystemCommandInterface& bsci,
   assert(!value.hasMultipleOutputs());
   assert(value.isExistingInput() || value.isMissingInput() ||
          value.isMissingOutput() || value.isFailedInput() ||
-         value.isVirtualInput());
+         value.isVirtualInput()  || value.isSkippedCommand());
 
-  // Predicate for whether the input should cause the command to skip.
-  auto shouldSkipForInput = [&] {
+  // If the input should cause this command to skip, how should it skip?
+  auto getSkipValueForInput = [&]() -> llvm::Optional<BuildValue> {
     // If the value is an existing or virtual input, we are always good.
     if (value.isExistingInput() || value.isVirtualInput())
-      return false;
+      return llvm::None;
 
     // We explicitly allow running the command against a missing output, under
     // the expectation that responsibility for reporting this situation falls to
@@ -242,19 +245,31 @@ void ExternalCommand::provideValue(BuildSystemCommandInterface& bsci,
     // FIXME: Eventually, it might be nice to harden the format so that we know
     // when an output was actually required versus optional.
     if (value.isMissingOutput())
-      return false;
+      return llvm::None;
 
     // If the value is a missing input, but those are allowed, it is ok.
-    if (allowMissingInputs && value.isMissingInput())
-      return false;
+    if (value.isMissingInput()) {
+      if (allowMissingInputs)
+        return llvm::None;
+      else
+        return BuildValue::makePropagatedFailureCommand();
+    }
 
-    // For anything else, this is an error and the command should be skipped.
-    return true;
+    // Propagate failure.
+    if (value.isFailedInput())
+      return BuildValue::makePropagatedFailureCommand();
+
+    // A skipped dependency doesn't cause this command to skip.
+    if (value.isSkippedCommand())
+        return llvm::None;
+
+    llvm_unreachable("unexpected input");
   };
 
   // Check if we need to skip the command because of this input.
-  if (shouldSkipForInput()) {
-    shouldSkip = true;
+  auto skipValueForInput = getSkipValueForInput();
+  if (skipValueForInput.hasValue()) {
+    skipValue = std::move(skipValueForInput);
     if (value.isMissingInput()) {
       hasMissingInput = true;
 
@@ -309,12 +324,12 @@ void ExternalCommand::inputsAvailable(BuildSystemCommandInterface& bsci,
                                       core::Task* task) {
   // If the build should cancel, do nothing.
   if (bsci.getDelegate().isCancelled()) {
-    bsci.taskIsComplete(task, BuildValue::makeSkippedCommand());
+    bsci.taskIsComplete(task, BuildValue::makeCancelledCommand());
     return;
   }
 
   // If this command should be skipped, do nothing.
-  if (shouldSkip) {
+  if (skipValue.hasValue()) {
     // If this command had a failed input, treat it as having failed.
     if (hasMissingInput) {
       // FIXME: Design the logging and status output APIs.
@@ -326,7 +341,7 @@ void ExternalCommand::inputsAvailable(BuildSystemCommandInterface& bsci,
       bsci.getDelegate().hadCommandFailure();
     }
 
-    bsci.taskIsComplete(task, BuildValue::makeSkippedCommand());
+    bsci.taskIsComplete(task, skipValue.getValue());
     return;
   }
   assert(!hasMissingInput);

llbuild.xcodeproj/project.pbxproj

@@ -70,6 +70,10 @@
 		9DB047BC1DF9D4AA006CDF52 /* libllvmSupport.a in Frameworks */ = {isa = PBXBuildFile; fileRef = E1B838A21B52E7DE00DB876B /* libllvmSupport.a */; };
 		9DB047BD1DF9D4B0006CDF52 /* libllbuildBuildSystem.a in Frameworks */ = {isa = PBXBuildFile; fileRef = E1B839571B541BFD00DB876B /* libllbuildBuildSystem.a */; };
 		9DB047C01DF9F592006CDF52 /* LaneBasedExecutionQueueTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9DB0478B1DF9D3E2006CDF52 /* LaneBasedExecutionQueueTest.cpp */; };
+		C5740D091E03523100567DD8 /* BuildSystemFrontendTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = C5740D081E03523100567DD8 /* BuildSystemFrontendTest.cpp */; };
+		C5740D0A1E03527B00567DD8 /* libllbuildCore.a in Frameworks */ = {isa = PBXBuildFile; fileRef = E1A2243E19F997150059043E /* libllbuildCore.a */; };
+		C5740D0B1E03528600567DD8 /* libllbuildBasic.a in Frameworks */ = {isa = PBXBuildFile; fileRef = E1A2242519F991B40059043E /* libllbuildBasic.a */; };
+		C5740D0C1E03529300567DD8 /* libsqlite3.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = E1E221081A00B82100957481 /* libsqlite3.dylib */; };
 		E104FAF71B655A97005C68A0 /* BuildSystemPerfTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = E104FAF61B655A97005C68A0 /* BuildSystemPerfTests.mm */; };
 		E104FAFA1B655BBA005C68A0 /* libllbuildBuildSystem.a in Frameworks */ = {isa = PBXBuildFile; fileRef = E1B839571B541BFD00DB876B /* libllbuildBuildSystem.a */; };
 		E104FAFB1B655C33005C68A0 /* libllvmSupport.a in Frameworks */ = {isa = PBXBuildFile; fileRef = E1B838A21B52E7DE00DB876B /* libllvmSupport.a */; };
@@ -688,6 +692,8 @@
 		54E187B81CD296EA00F7EC89 /* SwiftTools.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = SwiftTools.h; sourceTree = "<group>"; };
 		9DB0478B1DF9D3E2006CDF52 /* LaneBasedExecutionQueueTest.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = LaneBasedExecutionQueueTest.cpp; sourceTree = "<group>"; };
 		9DB047A81DF9D43D006CDF52 /* BuildSystemTests */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.executable"; includeInIndex = 0; path = BuildSystemTests; sourceTree = BUILT_PRODUCTS_DIR; };
+		C5740D081E03523100567DD8 /* BuildSystemFrontendTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = BuildSystemFrontendTest.cpp; sourceTree = "<group>"; };
+		C5740D0D1E0352D800567DD8 /* CMakeLists.txt */ = {isa = PBXFileReference; lastKnownFileType = text; path = CMakeLists.txt; sourceTree = "<group>"; };
 		E104FAF61B655A97005C68A0 /* BuildSystemPerfTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = BuildSystemPerfTests.mm; sourceTree = "<group>"; };
 		E104FAFF1B6568E0005C68A0 /* BuildSystem.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = BuildSystem.cpp; sourceTree = "<group>"; };
 		E1066C071BC5ACAB00B892CE /* LaneBasedExecutionQueue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = LaneBasedExecutionQueue.cpp; sourceTree = "<group>"; };
@@ -976,6 +982,9 @@
 			buildActionMask = 2147483647;
 			files = (
 				9D2107C61DFADDFA00BE26FF /* libcurses.dylib in Frameworks */,
+				C5740D0C1E03529300567DD8 /* libsqlite3.dylib in Frameworks */,
+				C5740D0B1E03528600567DD8 /* libllbuildBasic.a in Frameworks */,
+				C5740D0A1E03527B00567DD8 /* libllbuildCore.a in Frameworks */,
 				9DB047BD1DF9D4B0006CDF52 /* libllbuildBuildSystem.a in Frameworks */,
 				9DB047BC1DF9D4AA006CDF52 /* libllvmSupport.a in Frameworks */,
 				9DB047BA1DF9D4A4006CDF52 /* libgtest_main.a in Frameworks */,
@@ -1159,6 +1168,8 @@
 		9DB0478A1DF9D39E006CDF52 /* BuildSystem */ = {
 			isa = PBXGroup;
 			children = (
+				C5740D0D1E0352D800567DD8 /* CMakeLists.txt */,
+				C5740D081E03523100567DD8 /* BuildSystemFrontendTest.cpp */,
 				9DB0478B1DF9D3E2006CDF52 /* LaneBasedExecutionQueueTest.cpp */,
 			);
 			path = BuildSystem;
@@ -2583,6 +2594,7 @@
 			buildActionMask = 2147483647;
 			files = (
 				9DB047C01DF9F592006CDF52 /* LaneBasedExecutionQueueTest.cpp in Sources */,
+				C5740D091E03523100567DD8 /* BuildSystemFrontendTest.cpp in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};

products/libllbuild/BuildSystem-C-API.cpp

@@ -188,6 +188,15 @@ class CAPIBuildSystemFrontendDelegate : public BuildSystemFrontendDelegate {
     }
   }
 
+  virtual bool shouldCommandStart(Command * command) override {
+    if (cAPIDelegate.should_command_start) {
+      return cAPIDelegate.should_command_start(
+          cAPIDelegate.context, (llb_buildsystem_command_t*) command);
+    } else {
+      return true;
+    }
+  }
+
   virtual void commandStarted(Command* command) override {
     if (cAPIDelegate.command_started) {
       cAPIDelegate.command_started(

products/libllbuild/public-api/llbuild/buildsystem.h

@@ -227,6 +227,21 @@ typedef struct llb_buildsystem_delegate_t_ {
   /// with exactly one \see command_finished() call.
   void (*command_preparing)(void* context, llb_buildsystem_command_t* command);
 
+  /// Called by the build system to allow the delegate to skip a command without
+  /// implicitly skipping its dependents.
+  ///
+  /// WARNING: Clients need to take special care when using this. Skipping
+  /// commands without considering their dependencies or dependents can easily
+  /// produce an inconsistent build.
+  ///
+  /// This method is called before the command starts, when the system has
+  /// identified that it will eventually need to run (after all of its inputs
+  /// have been satisfied).
+  ///
+  /// The system guarantees that all such calls will be paired with a
+  /// corresponding \see commandFinished() call.
+  bool (*should_command_start)(void* context, llb_buildsystem_command_t* command);
+
   /// Called when a command has been started.
   ///
   /// The system guarantees that any commandStart() call will be paired with