diff --git a/include/llbuild/BuildSystem/BuildSystem.h b/include/llbuild/BuildSystem/BuildSystem.h index c4c18b7ac..e1e395bd7 100644 --- a/include/llbuild/BuildSystem/BuildSystem.h +++ b/include/llbuild/BuildSystem/BuildSystem.h @@ -235,7 +235,9 @@ class BuildSystemDelegate { /// \param reason - Describes why the rule needs to run. For example, because it has never run or because an input was rebuilt. /// /// \param inputRule - If `reason` is `InputRebuilt`, the rule for the rebuilt input, else `nullptr`. - virtual void determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule) = 0; + /// + /// \param details - Unstructured additional detail explaining why the rule needs to run. + virtual void determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule, StringRef details) = 0; }; /// The BuildSystem class is used to perform builds using the native build diff --git a/include/llbuild/BuildSystem/BuildSystemFrontend.h b/include/llbuild/BuildSystem/BuildSystemFrontend.h index 659988dcf..ddf095de1 100644 --- a/include/llbuild/BuildSystem/BuildSystemFrontend.h +++ b/include/llbuild/BuildSystem/BuildSystemFrontend.h @@ -295,7 +295,9 @@ class BuildSystemFrontendDelegate : public BuildSystemDelegate { /// \param reason - Describes why the rule needs to run. For example, because it has never run or because an input was rebuilt. /// /// \param inputRule - If `reason` is `InputRebuilt`, the rule for the rebuilt input, else `nullptr`. - virtual void determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule) override; + /// + /// \param details - Unstructured additional detail explaining why the rule needs to run. + virtual void determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule, StringRef details) override; /// Called when a cycle is detected by the build engine and it cannot make /// forward progress. diff --git a/include/llbuild/BuildSystem/Command.h b/include/llbuild/BuildSystem/Command.h index 4e9170386..69570d6f9 100644 --- a/include/llbuild/BuildSystem/Command.h +++ b/include/llbuild/BuildSystem/Command.h @@ -20,6 +20,7 @@ namespace llbuild { namespace core { class TaskInterface; +class Rule; } @@ -141,8 +142,8 @@ class Command : public basic::JobDescriptor { /// /// @{ - virtual bool isResultValid(BuildSystem& system, const BuildValue& value) = 0; - + virtual core::Rule::ValidationResult isResultValid(BuildSystem& system, const BuildValue& value) = 0; + virtual void start(BuildSystem& system, core::TaskInterface ti) = 0; virtual void providePriorValue(BuildSystem& system, core::TaskInterface ti, diff --git a/include/llbuild/BuildSystem/ExternalCommand.h b/include/llbuild/BuildSystem/ExternalCommand.h index d1df570a3..53b207e51 100644 --- a/include/llbuild/BuildSystem/ExternalCommand.h +++ b/include/llbuild/BuildSystem/ExternalCommand.h @@ -129,8 +129,8 @@ class ExternalCommand : public Command { virtual BuildValue getResultForOutput(Node* node, const BuildValue& value) override; - - virtual bool isResultValid(BuildSystem&, const BuildValue& value) override; + + virtual core::Rule::ValidationResult isResultValid(BuildSystem&, const BuildValue& value) override; virtual void start(BuildSystem& system, core::TaskInterface ti) override; diff --git a/include/llbuild/Core/BuildEngine.h b/include/llbuild/Core/BuildEngine.h index 48a2410fb..6094fb6ed 100644 --- a/include/llbuild/Core/BuildEngine.h +++ b/include/llbuild/Core/BuildEngine.h @@ -332,6 +332,17 @@ class Rule { Forced = 4 }; + struct ValidationResult { + /// Whether the rule's value is valid. + bool isValid; + + /// Optional unstructured description of why the value is invalid. + std::string details; + + ValidationResult(bool valid, const std::string& details = "") + : isValid(valid), details(details) {} + }; + public: /// The key computed by the rule. const KeyType key; @@ -358,7 +369,7 @@ class Rule { /// state managed externally to the build engine. For example, a rule which /// computes something on the file system may use this to verify that the /// computed output has not changed since it was built. - virtual bool isResultValid(BuildEngine&, const ValueType&) = 0; + virtual ValidationResult isResultValid(BuildEngine& engine, const ValueType& value) = 0; /// Called to indicate a change in the rule status. virtual void updateStatus(BuildEngine&, StatusKind); @@ -388,7 +399,10 @@ class BuildEngineDelegate { /// \param reason - Describes why the rule needs to run. For example, because it has never run or because an input was rebuilt. /// /// \param inputRule - If `reason` is `InputRebuilt`, the rule for the rebuilt input, else `nullptr`. - virtual void determinedRuleNeedsToRun(Rule* ruleNeedingToRun, Rule::RunReason reason, Rule* inputRule); + /// + /// \param details - Optional unstructured additional detail about why the rule needs to run. + virtual void determinedRuleNeedsToRun(Rule* ruleNeedingToRun, Rule::RunReason reason, Rule* inputRule, + StringRef details); /// Called when a cycle is detected by the build engine to check if it should /// attempt to resolve the cycle and continue diff --git a/lib/BuildSystem/BuildSystem.cpp b/lib/BuildSystem/BuildSystem.cpp index ba99987a8..9bdcdbdae 100644 --- a/lib/BuildSystem/BuildSystem.cpp +++ b/lib/BuildSystem/BuildSystem.cpp @@ -147,7 +147,7 @@ class BuildSystemEngineDelegate : public BuildEngineDelegate { const BuildDescription& getBuildDescription() const; virtual std::unique_ptr lookupRule(const KeyType& keyData) override; - virtual void determinedRuleNeedsToRun(Rule* ruleNeedingToRun, Rule::RunReason reason, Rule* inputRule) override; + virtual void determinedRuleNeedsToRun(Rule* ruleNeedingToRun, Rule::RunReason reason, Rule* inputRule, StringRef details) override; virtual bool shouldResolveCycle(const std::vector& items, Rule* candidateRule, Rule::CycleAction action) override; @@ -467,9 +467,9 @@ class TargetTask : public Task { public: TargetTask(Target& target) : target(target) {} - static bool isResultValid(BuildEngine&, Target&, const BuildValue&) { + static Rule::ValidationResult isResultValid(BuildEngine&, Target&, const BuildValue&) { // Always treat target tasks as invalid. - return false; + return Rule::ValidationResult(false); } }; @@ -514,8 +514,8 @@ class FileInputNodeTask : public Task { assert(!node.isVirtual()); } - static bool isResultValid(BuildEngine& engine, const BuildNode& node, - const BuildValue& value) { + static Rule::ValidationResult isResultValid(BuildEngine& engine, const BuildNode& node, + const BuildValue& value) { // The result is valid if the existence matches the value type and the file // information remains the same. // @@ -532,9 +532,17 @@ class FileInputNodeTask : public Task { auto info = node.getFileInfo( getBuildSystem(engine).getFileSystem()); if (info.isMissing()) { - return value.isMissingInput(); + if (value.isMissingInput()) { + return Rule::ValidationResult(true); + } else { + return Rule::ValidationResult(false, ("'" + node.getName() + "' was removed").str()); + } } else { - return value.isExistingInput() && value.getOutputInfo() == info; + if (value.isExistingInput() && value.getOutputInfo() == info) { + return Rule::ValidationResult(true); + } else { + return Rule::ValidationResult(false, ("'" + node.getName() + "' was updated").str()); + } } } }; @@ -575,9 +583,9 @@ class StatTask : public Task { public: StatTask(StatNode& statnode) : statnode(statnode) {} - static bool isResultValid(BuildEngine&, const StatNode&, const BuildValue&) { + static Rule::ValidationResult isResultValid(BuildEngine&, const StatNode&, const BuildValue&) { // Always read the stat information - return false; + return Rule::ValidationResult(false); } }; @@ -717,10 +725,10 @@ class VirtualInputNodeTask : public Task { public: VirtualInputNodeTask() {} - static bool isResultValid(BuildEngine& engine, const BuildNode& node, + static Rule::ValidationResult isResultValid(BuildEngine& engine, const BuildNode& node, const BuildValue& value) { // Virtual input nodes are always valid unless the value type is wrong. - return value.isVirtualInput(); + return Rule::ValidationResult(value.isVirtualInput()); } }; @@ -797,21 +805,23 @@ class ProducedNodeTask : public Task { ProducedNodeTask(Node& node) : node(node), nodeResult(BuildValue::makeInvalid()) {} - static bool isResultValid(BuildEngine& engine, Node& node, + static Rule::ValidationResult isResultValid(BuildEngine& engine, Node& node, const BuildValue& value) { // If the result was failure, we always need to rebuild (it may produce an // error). - if (value.isFailedInput()) - return false; + if (value.isFailedInput()) { + return Rule::ValidationResult(false, ("'" + node.getName()).str() + "' previously failed"); + } // If the result was previously a missing input, it may have been because // we did not previously know how to produce this node. We do now, so // attempt to build it now. - if (value.isMissingInput()) - return false; + if (value.isMissingInput()) { + return Rule::ValidationResult(false, ("'" + node.getName()).str() + "' was previously recorded as a missing input"); + } // The produced node result itself doesn't need any synchronization. - return true; + return Rule::ValidationResult(true); } }; @@ -903,22 +913,24 @@ class ProducedDirectoryNodeTask : public Task { ProducedDirectoryNodeTask(Node& node) : node(node), nodeResult(BuildValue::makeInvalid()) {} - static bool isResultValid(BuildEngine& engine, Node& node, + static Rule::ValidationResult isResultValid(BuildEngine& engine, Node& node, const BuildValue& value) { // If the result was failure, we always need to rebuild (it may produce an // error). - if (value.isFailedInput()) - return false; + if (value.isFailedInput()) { + return Rule::ValidationResult(false, ("'" + node.getName()).str() + "' previously failed"); + } // If the result was previously a missing input, it may have been because // we did not previously know how to produce this node. We do now, so // attempt to build it now. - if (value.isMissingInput()) - return false; + if (value.isMissingInput()) { + return Rule::ValidationResult(false, ("'" + node.getName()).str() + "' was previously recorded as a missing input"); + } // The produced node result itself doesn't need any synchronization. // If the directory signature is changed, it will be reflected in value of this node. - return true; + return Rule::ValidationResult(true); } }; @@ -1050,25 +1062,40 @@ class DirectoryContentsTask : public Task { DirectoryContentsTask(StringRef path) : path(path), directoryValue(BuildValue::makeInvalid()) {} - static bool isResultValid(BuildEngine& engine, StringRef path, + static Rule::ValidationResult isResultValid(BuildEngine& engine, StringRef path, const BuildValue& value) { // The result is valid if the existence matches the existing value type, and // the file information remains the same. auto info = getBuildSystem(engine).getFileSystem().getFileInfo( path); if (info.isMissing()) { - return value.isMissingInput(); + if (value.isMissingInput()) { + return Rule::ValidationResult(true); + } else { + return Rule::ValidationResult(false, ("'" + path + "' was removed").str()); + } } else { - if (!value.isDirectoryContents()) - return false; + if (!value.isDirectoryContents()) { + return Rule::ValidationResult(false); + } // If the type changes rebuild - if (info.isDirectory() != value.getOutputInfo().isDirectory()) - return false; + if (info.isDirectory() != value.getOutputInfo().isDirectory()) { + if (info.isDirectory()) { + return Rule::ValidationResult(false, ("'" + path + "' changed from a file to a directory").str()); + } else { + return Rule::ValidationResult(false, ("'" + path + "' changed from a directory to a file").str()); + } + } // For files, it is direct stat info that matters - if (!info.isDirectory()) - return value.getOutputInfo() == info; + if (!info.isDirectory()) { + if (value.getOutputInfo() == info) { + return Rule::ValidationResult(true); + } else { + return Rule::ValidationResult(false, ("'" + path + "' changed").str()); + } + } // With filters, we list the current filtered contents and then compare // the lists. @@ -1082,17 +1109,20 @@ class DirectoryContentsTask : public Task { auto prev = value.getDirectoryContents(); if (cur.size() != prev.size()) - return false; + return Rule::ValidationResult(false, ("'" + path + "' has " + std::to_string(prev.size()) + + "entries, but previously recorded " + std::to_string(cur.size()) + + " entries").str()); auto cur_it = cur.begin(); auto prev_it = prev.begin(); for (; cur_it != cur.end() && prev_it != prev.end(); cur_it++, prev_it++) { if (*cur_it != *prev_it) { - return false; + return Rule::ValidationResult(false, ("'" + path + "' contains entry which was renamed from '" + + *prev_it + "' to '" + *cur_it + "'").str()); } } - return true; + return Rule::ValidationResult(true); } } }; @@ -1598,8 +1628,8 @@ class CommandTask : public Task { public: CommandTask(Command& command) : command(command) {} - static bool isResultValid(BuildEngine& engine, Command& command, - const BuildValue& value) { + static Rule::ValidationResult isResultValid(BuildEngine& engine, Command& command, + const BuildValue& value) { // Delegate to the command for further checking. auto& buildSystem = static_cast(engine.getDelegate())->getBuildSystem(); @@ -1666,8 +1696,8 @@ class BuildSystemRule : public Rule { /// state managed externally to the build engine. For example, a rule which /// computes something on the file system may use this to verify that the /// computed output has not changed since it was built. - std::function resultValid; + std::function resultValid; /// Called to indicate a change in the rule status. std::function update; @@ -1677,7 +1707,7 @@ class BuildSystemRule : public Rule { const KeyType& key, const basic::CommandSignature& signature, std::function action, - std::function valid = nullptr, + std::function valid = nullptr, std::function update = nullptr) : Rule(key, signature), action(action), resultValid(valid), update(update) { } @@ -1687,8 +1717,8 @@ class BuildSystemRule : public Rule { return action(engine); } - bool isResultValid(BuildEngine& engine, const ValueType& value) override { - if (!resultValid) return true; + ValidationResult isResultValid(BuildEngine& engine, const ValueType& value) override { + if (!resultValid) return ValidationResult(true); return resultValid(engine, *this, value); } @@ -1739,9 +1769,9 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa /*Action=*/ [](BuildEngine& engine) -> Task* { return new MissingCommandTask(); }, - /*IsValid=*/ [](BuildEngine&, const Rule&, const ValueType&) -> bool { + /*IsValid=*/ [](BuildEngine&, const Rule&, const ValueType&) -> Rule::ValidationResult { // The cached result for a missing command is never valid. - return false; + return Rule::ValidationResult(false); } )); } @@ -1755,7 +1785,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new CommandTask(*command); }, /*IsValid=*/ [command](BuildEngine& engine, const Rule& rule, - const ValueType& value) -> bool { + const ValueType& value) -> Rule::ValidationResult { return CommandTask::isResultValid( engine, *command, BuildValue::fromData(value)); }, @@ -1788,7 +1818,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new CommandTask(*command); }, /*IsValid=*/ [command](BuildEngine& engine, const Rule& rule, - const ValueType& value) -> bool { + const ValueType& value) -> Rule::ValidationResult { return CommandTask::isResultValid( engine, *command, BuildValue::fromData(value)); }, @@ -1808,9 +1838,9 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa /*Action=*/ [](BuildEngine& engine) -> Task* { return new MissingCommandTask(); }, - /*IsValid=*/ [](BuildEngine&, const Rule&, const ValueType&) -> bool { + /*IsValid=*/ [](BuildEngine&, const Rule&, const ValueType&) -> Rule::ValidationResult { // The cached result for a missing command is never valid. - return false; + return Rule::ValidationResult(false); } )); } @@ -1824,7 +1854,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new DirectoryContentsTask(path); }, /*IsValid=*/ [path](BuildEngine& engine, const Rule& rule, - const ValueType& value) mutable -> bool { + const ValueType& value) mutable -> Rule::ValidationResult { return DirectoryContentsTask::isResultValid( engine, path, BuildValue::fromData(value)); } @@ -1900,7 +1930,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new VirtualInputNodeTask(); }, /*IsValid=*/ [node](BuildEngine& engine, const Rule& rule, - const ValueType& value) -> bool { + const ValueType& value) -> Rule::ValidationResult { return VirtualInputNodeTask::isResultValid( engine, *node, BuildValue::fromData(value)); } @@ -1942,7 +1972,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new FileInputNodeTask(*node); }, /*IsValid=*/ [node](BuildEngine& engine, const Rule& rule, - const ValueType& value) -> bool { + const ValueType& value) -> Rule::ValidationResult { return FileInputNodeTask::isResultValid( engine, *node, BuildValue::fromData(value)); } @@ -1959,7 +1989,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new ProducedDirectoryNodeTask(*node); }, /*IsValid=*/ [node](BuildEngine& engine, const Rule& rule, - const ValueType& value) -> bool { + const ValueType& value) -> Rule::ValidationResult { return ProducedDirectoryNodeTask::isResultValid( engine, *node, BuildValue::fromData(value)); } @@ -1974,7 +2004,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new ProducedNodeTask(*node); }, /*IsValid=*/ [node](BuildEngine& engine, const Rule& rule, - const ValueType& value) -> bool { + const ValueType& value) -> Rule::ValidationResult { return ProducedNodeTask::isResultValid( engine, *node, BuildValue::fromData(value)); } @@ -2001,7 +2031,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new StatTask(*statnode); }, /*IsValid=*/ [statnode](BuildEngine& engine, const Rule& rule, - const ValueType& value) -> bool { + const ValueType& value) -> Rule::ValidationResult { return StatTask::isResultValid( engine, *statnode, BuildValue::fromData(value)); } @@ -2025,7 +2055,7 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa return new TargetTask(*target); }, /*IsValid=*/ [target](BuildEngine& engine, const Rule& rule, - const ValueType& value) -> bool { + const ValueType& value) -> Rule::ValidationResult { return TargetTask::isResultValid( engine, *target, BuildValue::fromData(value)); } @@ -2037,8 +2067,8 @@ std::unique_ptr BuildSystemEngineDelegate::lookupRule(const KeyType& keyDa abort(); } -void BuildSystemEngineDelegate::determinedRuleNeedsToRun(Rule* ruleNeedingToRun, Rule::RunReason reason, Rule* inputRule) { - return getBuildSystem().getDelegate().determinedRuleNeedsToRun(ruleNeedingToRun, reason, inputRule); +void BuildSystemEngineDelegate::determinedRuleNeedsToRun(Rule* ruleNeedingToRun, Rule::RunReason reason, Rule* inputRule, StringRef details) { + return getBuildSystem().getDelegate().determinedRuleNeedsToRun(ruleNeedingToRun, reason, inputRule, details); } bool BuildSystemEngineDelegate::shouldResolveCycle(const std::vector& cycle, @@ -2490,10 +2520,10 @@ class SwiftGetVersionCommand : public Command { llvm_unreachable("unexpected"); return BuildValue::makeInvalid(); } - - virtual bool isResultValid(BuildSystem&, const BuildValue& value) override { + + virtual core::Rule::ValidationResult isResultValid(BuildSystem&, const BuildValue& value) override { // Always rebuild this task. - return false; + return core::Rule::ValidationResult(false); } virtual void start(BuildSystem&, TaskInterface ti) override { } @@ -3078,21 +3108,21 @@ class MkdirCommand : public ExternalCommand { } } - virtual bool isResultValid(BuildSystem& system, + virtual core::Rule::ValidationResult isResultValid(BuildSystem& system, const BuildValue& value) override { // If the prior value wasn't for a successful command, recompute. if (!value.isSuccessfulCommand()) - return false; + return core::Rule::ValidationResult(false, "directory creation previously failed"); // Otherwise, the result is valid if the directory still exists. auto info = getOutputs()[0]->getFileInfo( system.getFileSystem()); if (info.isMissing()) - return false; + return core::Rule::ValidationResult(false, "directory was removed"); // If the item is not a directory, it needs to be recreated. if (!info.isDirectory()) - return false; + return core::Rule::ValidationResult(false, "not a directory"); // FIXME: We should strictly enforce the integrity of this validity routine // by ensuring that the build result for this command does not fully encode @@ -3100,8 +3130,7 @@ class MkdirCommand : public ExternalCommand { // out the details of the file info (like the timestamp), but not rerunning // when they change. This is by design for this command, but it would still // be nice to be strict about it. - - return true; + return core::Rule::ValidationResult(true); } virtual void startExternalCommand(BuildSystem&, TaskInterface ti) override { @@ -3303,28 +3332,27 @@ class SymlinkCommand : public Command { return BuildValue::makeExistingInput(info); } - virtual bool isResultValid(BuildSystem& system, - const BuildValue& value) override { + virtual core::Rule::ValidationResult isResultValid(BuildSystem& system, const BuildValue& value) override { // It is an error if this command isn't configured properly. StringRef outputPath = getActualOutputPath(); if (outputs.empty() || outputPath.empty()) - return false; + return core::Rule::ValidationResult(false); // If the prior value wasn't for a successful command, recompute. if (!value.isSuccessfulCommand()) - return false; + return core::Rule::ValidationResult(false, "symlink creation previously failed"); // If the prior command doesn't look like one for a link, recompute. if (value.getNumOutputs() != 1) - return false; + return core::Rule::ValidationResult(false); // Otherwise, assume the result is valid if its link status matches the // previous one. auto info = system.getFileSystem().getLinkInfo(outputPath); if (info.isMissing()) - return false; + return core::Rule::ValidationResult(false, "not a symlink"); - return info == value.getOutputInfo(); + return core::Rule::ValidationResult(info == value.getOutputInfo()); } virtual void start(BuildSystem&, TaskInterface ti) override { @@ -3870,10 +3898,9 @@ class StaleFileRemovalCommand : public Command { return BuildValue::fromData(value.toData());; } - virtual bool isResultValid(BuildSystem& system, - const BuildValue& value) override { + virtual core::Rule::ValidationResult isResultValid(BuildSystem& system, const BuildValue& value) override { // Always re-run stale file removal. - return false; + return core::Rule::ValidationResult(false); } virtual void start(BuildSystem&, TaskInterface) override {} diff --git a/lib/BuildSystem/BuildSystemFrontend.cpp b/lib/BuildSystem/BuildSystemFrontend.cpp index cadda3dc4..a575d9cf8 100644 --- a/lib/BuildSystem/BuildSystemFrontend.cpp +++ b/lib/BuildSystem/BuildSystemFrontend.cpp @@ -741,7 +741,7 @@ void BuildSystemFrontendDelegate::commandProcessStarted(Command*, ProcessHandle) { } -void BuildSystemFrontendDelegate::determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule) {} +void BuildSystemFrontendDelegate::determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule, StringRef details) {} bool BuildSystemFrontendDelegate:: shouldResolveCycle(const std::vector& items, diff --git a/lib/BuildSystem/ExternalCommand.cpp b/lib/BuildSystem/ExternalCommand.cpp index ed680b53c..6ab19aeef 100644 --- a/lib/BuildSystem/ExternalCommand.cpp +++ b/lib/BuildSystem/ExternalCommand.cpp @@ -164,17 +164,16 @@ getResultForOutput(Node* node, const BuildValue& value) { return BuildValue::makeExistingInput(info); } - -bool ExternalCommand::isResultValid(BuildSystem& system, - const BuildValue& value) { + +core::Rule::ValidationResult ExternalCommand::isResultValid(BuildSystem& system, const BuildValue& value) { // Treat the command as always out-of-date, if requested. if (alwaysOutOfDate) - return false; - + return core::Rule::ValidationResult(false, "command is considered 'always out of date'"); + // If the prior value wasn't for a successful command, recompute. if (!value.isSuccessfulCommand()) - return false; - + return core::Rule::ValidationResult(false, "command previously failed"); + // Check the timestamps on each of the outputs. for (unsigned i = 0, e = outputs.size(); i != e; ++i) { auto* node = outputs[i]; @@ -198,21 +197,38 @@ bool ExternalCommand::isResultValid(BuildSystem& system, // could enable behavior to remove such output files if annotated prior to // running the command. auto info = node->getFileInfo(system.getFileSystem()); + auto oldInfo = value.getNthOutputInfo(i); // If this output is mutated by the build, we can't rely on equivalence, // only existence. if (node->isMutated()) { - if (value.getNthOutputInfo(i).isMissing() != info.isMissing()) - return false; + if (oldInfo.isMissing() != info.isMissing()) { + std::string msg; + if (info.isMissing()) { + msg = ("'" + node->getName() + "' was removed").str(); + } else { + msg = ("'" + node->getName() + "' was previously missing, but now exists").str(); + } + return core::Rule::ValidationResult(false, msg); + } continue; } - if (value.getNthOutputInfo(i) != info) - return false; + if (oldInfo != info) { + std::string msg; + if (info.isMissing() == oldInfo.isMissing()) { + msg = ("'" + node->getName() + "' changed").str(); + } else if (info.isMissing()) { + msg = ("'" + node->getName() + "' was removed").str(); + } else if (oldInfo.isMissing()) { + msg = ("'" + node->getName() + "' was previously missing, but now exists").str(); + } + return core::Rule::ValidationResult(false, msg); + } } // Otherwise, the result is ok. - return true; + return core::Rule::ValidationResult(true); } void ExternalCommand::start(BuildSystem& system, diff --git a/lib/Commands/BuildEngineCommand.cpp b/lib/Commands/BuildEngineCommand.cpp index aff98e411..b22be6193 100644 --- a/lib/Commands/BuildEngineCommand.cpp +++ b/lib/Commands/BuildEngineCommand.cpp @@ -205,8 +205,8 @@ static int runAckermannBuild(int m, int n, int recomputeCount, auto k = AckermannKey(key); return new AckermannTask(engine, k.m, k.n); } - bool isResultValid(core::BuildEngine&, const core::ValueType&) override { - return true; + ValidationResult isResultValid(core::BuildEngine&, const core::ValueType&) override { + return ValidationResult(true); } }; diff --git a/lib/Commands/BuildSystemCommand.cpp b/lib/Commands/BuildSystemCommand.cpp index 46a4cd96c..140de0d18 100644 --- a/lib/Commands/BuildSystemCommand.cpp +++ b/lib/Commands/BuildSystemCommand.cpp @@ -227,8 +227,8 @@ class ParseDummyCommand : public Command { const BuildValue& value) override { return BuildValue::makeMissingInput(); } - virtual bool isResultValid(BuildSystem&, const BuildValue&) override { - return false; + virtual core::Rule::ValidationResult isResultValid(BuildSystem&, const BuildValue&) override { + return core::Rule::ValidationResult(false); } virtual void start(BuildSystem&, TaskInterface) override {} virtual void providePriorValue(BuildSystem&, TaskInterface, const BuildValue&) override {} diff --git a/lib/Commands/NinjaBuildCommand.cpp b/lib/Commands/NinjaBuildCommand.cpp index b589ed03d..d818c5ac4 100644 --- a/lib/Commands/NinjaBuildCommand.cpp +++ b/lib/Commands/NinjaBuildCommand.cpp @@ -1713,7 +1713,7 @@ std::unique_ptr NinjaBuildEngineDelegate::lookupRule(const core::Key return buildInput(*context, node); } - bool isResultValid(core::BuildEngine&, const core::ValueType& value) override { + ValidationResult isResultValid(core::BuildEngine&, const core::ValueType& value) override { // If simulating, assume cached results are valid. if (context->simulate) return true; @@ -2123,7 +2123,7 @@ int commands::executeNinjaBuildCommand(std::vector args) { return buildCommand(context, command); } - bool isResultValid(core::BuildEngine&, const core::ValueType& value) override { + ValidationResult isResultValid(core::BuildEngine&, const core::ValueType& value) override { // If simulating, assume cached results are valid. if (context.simulate) return true; @@ -2152,7 +2152,7 @@ int commands::executeNinjaBuildCommand(std::vector args) { return selectCompositeBuildResult(context, command, index, compositeRuleName); } - bool isResultValid(core::BuildEngine&, const core::ValueType& value) override { + ValidationResult isResultValid(core::BuildEngine&, const core::ValueType& value) override { // If simulating, assume cached results are valid. if (context.simulate) return true; @@ -2173,7 +2173,7 @@ int commands::executeNinjaBuildCommand(std::vector args) { return buildTargets(context, targets); } - bool isResultValid(core::BuildEngine&, const core::ValueType& value) override { + ValidationResult isResultValid(core::BuildEngine&, const core::ValueType& value) override { return false; } }; diff --git a/lib/Core/BuildEngine.cpp b/lib/Core/BuildEngine.cpp index 162c9eb86..ec3fcb78c 100644 --- a/lib/Core/BuildEngine.cpp +++ b/lib/Core/BuildEngine.cpp @@ -48,7 +48,7 @@ void Rule::updateStatus(BuildEngine&, StatusKind) {} BuildEngineDelegate::~BuildEngineDelegate() {} -void BuildEngineDelegate::determinedRuleNeedsToRun(Rule* ruleNeedingToRun, Rule::RunReason, Rule* inputRule) {} +void BuildEngineDelegate::determinedRuleNeedsToRun(Rule* ruleNeedingToRun, Rule::RunReason, Rule* inputRule, StringRef detail) {} bool BuildEngineDelegate::shouldResolveCycle(const std::vector& items, Rule* candidateRule, @@ -467,7 +467,7 @@ class BuildEngineImpl : public BuildDBDelegate { if (trace) trace->ruleNeedsToRunBecauseNeverBuilt(ruleInfo.rule.get()); ruleInfo.state = RuleInfo::StateKind::NeedsToRun; - delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::NeverBuilt, nullptr); + delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::NeverBuilt, nullptr, StringRef()); return true; } @@ -475,7 +475,7 @@ class BuildEngineImpl : public BuildDBDelegate { if (trace) trace->ruleNeedsToRunBecauseSignatureChanged(ruleInfo.rule.get()); ruleInfo.state = RuleInfo::StateKind::NeedsToRun; - delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::SignatureChanged, nullptr); + delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::SignatureChanged, nullptr, StringRef()); return true; } @@ -484,11 +484,12 @@ class BuildEngineImpl : public BuildDBDelegate { // FIXME: We should probably try and move this so that it can be done by // clients in the background, either by us backgrounding it, or by using a // completion model as we do for inputs. - if (!ruleInfo.rule->isResultValid(buildEngine, ruleInfo.result.value)) { + auto validationResult = ruleInfo.rule->isResultValid(buildEngine, ruleInfo.result.value); + if (!validationResult.isValid) { if (trace) trace->ruleNeedsToRunBecauseInvalidValue(ruleInfo.rule.get()); ruleInfo.state = RuleInfo::StateKind::NeedsToRun; - delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::InvalidValue, nullptr); + delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::InvalidValue, nullptr, validationResult.details); return true; } @@ -668,7 +669,7 @@ class BuildEngineImpl : public BuildDBDelegate { trace->ruleNeedsToRunBecauseInputRebuilt( ruleInfo.rule.get(), inputRuleInfo.rule.get()); finishScanRequest(ruleInfo, RuleInfo::StateKind::NeedsToRun); - delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::InputRebuilt, inputRuleInfo.rule.get()); + delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::InputRebuilt, inputRuleInfo.rule.get(), StringRef()); return; } } @@ -1253,7 +1254,7 @@ class BuildEngineImpl : public BuildDBDelegate { // mark this rule as needs to run (forced) finishScanRequest(ruleInfo, RuleInfo::StateKind::NeedsToRun); - delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::Forced, nullptr); + delegate.determinedRuleNeedsToRun(ruleInfo.rule.get(), Rule::RunReason::Forced, nullptr, StringRef()); ruleInfo.wasForced = true; return true; diff --git a/perftests/Xcode/PerfTests/CorePerfTests.mm b/perftests/Xcode/PerfTests/CorePerfTests.mm index cfd329ed2..942fd9381 100644 --- a/perftests/Xcode/PerfTests/CorePerfTests.mm +++ b/perftests/Xcode/PerfTests/CorePerfTests.mm @@ -106,7 +106,7 @@ virtual void inputsAvailable(TaskInterface ti) override { Task* createTask(BuildEngine&) override { return new SimpleTask(inputs, compute); } - bool isResultValid(BuildEngine&, const ValueType& value) override { + ValidationResult isResultValid(BuildEngine&, const ValueType& value) override { if (!valid) return true; return valid(value); } diff --git a/products/libllbuild/BuildSystem-C-API.cpp b/products/libllbuild/BuildSystem-C-API.cpp index 77c90acf1..30afccaf6 100644 --- a/products/libllbuild/BuildSystem-C-API.cpp +++ b/products/libllbuild/BuildSystem-C-API.cpp @@ -496,8 +496,8 @@ class CAPIBuildSystemFrontendDelegate : public BuildSystemFrontendDelegate { return llb_rule_run_reason_invalid_value; } - virtual void determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule) override { - if (cAPIDelegate.determined_rule_needs_to_run) { + virtual void determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule, StringRef details) override { + if (cAPIDelegate.determined_rule_needs_to_run || cAPIDelegate.determined_rule_needs_to_run_v2) { auto key = BuildKey::fromData(ruleNeedingToRun->key); auto needsToRun = (llb_build_key_t *)new CAPIBuildKey(key); llb_build_key_t * input; @@ -507,7 +507,24 @@ class CAPIBuildSystemFrontendDelegate : public BuildSystemFrontendDelegate { } else { input = nullptr; } - cAPIDelegate.determined_rule_needs_to_run(cAPIDelegate.context, needsToRun, convertRunReason(reason), input); + + if (cAPIDelegate.determined_rule_needs_to_run_v2) { + cAPIDelegate.determined_rule_needs_to_run_v2( + cAPIDelegate.context, + needsToRun, + convertRunReason(reason), + input, + details.empty() ? nullptr : details.str().c_str() + ); + } else { + cAPIDelegate.determined_rule_needs_to_run( + cAPIDelegate.context, + needsToRun, + convertRunReason(reason), + input + ); + } + llb_build_key_destroy(needsToRun); if (input) { llb_build_key_destroy(input); @@ -1145,42 +1162,44 @@ class CAPIExternalCommand : public ExternalCommand { return ExternalCommand::computeCommandResult(system, ti); } - bool isResultValid(BuildSystem& system, const BuildValue& value) override { + core::Rule::ValidationResult isResultValid(BuildSystem& system, const BuildValue& value) override { if (cAPIDelegate.is_result_valid_with_fallback) { struct FallbackContext { BuildSystem *buildSystem; - + static bool handle(void* c_ctx, llb_buildsystem_command_t* c_command, llb_build_value* c_value) { auto* command = (CAPIExternalCommand*)c_command; auto* value = (CAPIBuildValue*)c_value; - + auto* ctx = static_cast(c_ctx); BuildSystem& system = *ctx->buildSystem; delete ctx; - - return command->ExternalCommand::isResultValid(system, value->getInternalBuildValue()); + + return command->ExternalCommand::isResultValid(system, value->getInternalBuildValue()).isValid; } }; - + auto value_p = (llb_build_value *)new CAPIBuildValue(BuildValue(value)); - return cAPIDelegate.is_result_valid_with_fallback( + bool valid = cAPIDelegate.is_result_valid_with_fallback( cAPIDelegate.context, (llb_buildsystem_command_t*)this, value_p, new FallbackContext{&system}, FallbackContext::handle ); + return core::Rule::ValidationResult(valid); } - + if (cAPIDelegate.is_result_valid) { auto value_p = (llb_build_value *)new CAPIBuildValue(BuildValue(value)); - return cAPIDelegate.is_result_valid( + bool valid = cAPIDelegate.is_result_valid( cAPIDelegate.context, (llb_buildsystem_command_t*)this, value_p ); + return core::Rule::ValidationResult(valid); } return ExternalCommand::isResultValid(system, value); diff --git a/products/libllbuild/Core-C-API.cpp b/products/libllbuild/Core-C-API.cpp index d7823ac84..e7e147c48 100644 --- a/products/libllbuild/Core-C-API.cpp +++ b/products/libllbuild/Core-C-API.cpp @@ -43,12 +43,12 @@ class CAPIBuildEngineDelegate : public BuildEngineDelegate, public basic::Execut return (Task*) rule.create_task(rule.context, engineContext); } - bool isResultValid(BuildEngine&, const ValueType& value) override { - if (!rule.is_result_valid) return true; + ValidationResult isResultValid(BuildEngine&, const ValueType& value) override { + if (!rule.is_result_valid) return ValidationResult(true); llb_data_t value_data{ value.size(), value.data() }; - return rule.is_result_valid(rule.context, engineContext, &rule, - &value_data); + bool valid = rule.is_result_valid(rule.context, engineContext, &rule, &value_data); + return ValidationResult(valid); } void updateStatus(BuildEngine&, Rule::StatusKind status) override { diff --git a/products/libllbuild/include/llbuild/buildsystem.h b/products/libllbuild/include/llbuild/buildsystem.h index 85c3df5b8..b73a23aaf 100644 --- a/products/libllbuild/include/llbuild/buildsystem.h +++ b/products/libllbuild/include/llbuild/buildsystem.h @@ -508,7 +508,10 @@ typedef struct llb_buildsystem_delegate_t_ { /// Xparam reason Describes why the rule needs to run. For example, because it has never run or because an input was rebuilt. /// /// Xparam input_rule If `reason` is `InputRebuilt`, the rule for the rebuilt input, else `nullptr`. + /// + /// Xparam details Optional additional unstructured description of why the rule needs to run. void (*determined_rule_needs_to_run)(void* context, llb_build_key_t* rule_needing_to_run, llb_rule_run_reason_t reason, llb_build_key_t* input_rule); + void (*determined_rule_needs_to_run_v2)(void* context, llb_build_key_t* rule_needing_to_run, llb_rule_run_reason_t reason, llb_build_key_t* input_rule, const char* details); /// Called when a cycle is detected by the build engine and it cannot make /// forward progress. diff --git a/products/llbuildSwift/BuildSystemBindings.swift b/products/llbuildSwift/BuildSystemBindings.swift index 37f6b7b52..49a68889a 100644 --- a/products/llbuildSwift/BuildSystemBindings.swift +++ b/products/llbuildSwift/BuildSystemBindings.swift @@ -980,6 +980,9 @@ public protocol BuildSystemDelegate { /// - parameter reason: Describes why the rule needs to run. For example, because it has never run or because an input was rebuilt. /// /// - parameter inputRule: If `reason` is `InputRebuilt`, the rule for the rebuilt input, else `nil`. + /// + /// - parameter details: Optional additional unstructured description of why the rule needs to run.. + func determinedRuleNeedsToRun(_ rule: BuildKey, reason: RuleRunReason, inputRule: BuildKey?, details: String?) func determinedRuleNeedsToRun(_ rule: BuildKey, reason: RuleRunReason, inputRule: BuildKey?) /// Called when a cycle is detected by the build engine and it cannot make @@ -1015,8 +1018,12 @@ extension BuildSystemDelegate { return nil } + public func determinedRuleNeedsToRun(_ rule: BuildKey, reason: RuleRunReason, inputRule: BuildKey?, details: String?) { + determinedRuleNeedsToRun(rule, reason: reason, inputRule: inputRule) + } + public func determinedRuleNeedsToRun(_ rule: BuildKey, reason: RuleRunReason, inputRule: BuildKey?) { - // default implementation for compatibility with older clients + // default implementation for ABI compatibility with older clients } public func commandCannotBuildOutputDueToMissingInputs(_ command: Command, output: BuildKey?, inputs: [BuildKey]) { @@ -1174,8 +1181,9 @@ public final class BuildSystem { _delegate.command_process_had_error = { BuildSystem.toSystem($0!).commandProcessHadError(Command(handle: $1), ProcessHandle($2!), $3!) } _delegate.command_process_had_output = { BuildSystem.toSystem($0!).commandProcessHadOutput(Command(handle: $1), ProcessHandle($2!), $3!) } _delegate.command_process_finished = { BuildSystem.toSystem($0!).commandProcessFinished(Command(handle: $1), ProcessHandle($2!), CommandExtendedResult($3!)) } - _delegate.determined_rule_needs_to_run = { - BuildSystem.toSystem($0!).determinedRuleNeedsToRun(BuildKey.construct(key: $1!), reason: $2, inputRule: $3.map { BuildKey.construct(key: $0) }) + _delegate.determined_rule_needs_to_run_v2 = { + let details = $4.map { String(cString: $0) } + BuildSystem.toSystem($0!).determinedRuleNeedsToRun(BuildKey.construct(key: $1!), reason: $2, inputRule: $3.map { BuildKey.construct(key: $0) }, details: details) } _delegate.cycle_detected = { var rules = [BuildKey]() @@ -1404,8 +1412,8 @@ public final class BuildSystem { delegate.commandProcessFinished(command, process: process, result: result) } - private func determinedRuleNeedsToRun(_ rule: BuildKey, reason: RuleRunReason, inputRule: BuildKey?) { - delegate.determinedRuleNeedsToRun(rule, reason: reason, inputRule: inputRule) + private func determinedRuleNeedsToRun(_ rule: BuildKey, reason: RuleRunReason, inputRule: BuildKey?, details: String?) { + delegate.determinedRuleNeedsToRun(rule, reason: reason, inputRule: inputRule, details: details) } private func cycleDetected(_ rules: [BuildKey]) { diff --git a/unittests/BuildSystem/BuildSystemFrontendTest.cpp b/unittests/BuildSystem/BuildSystemFrontendTest.cpp index e6cb1997d..9c49628fd 100644 --- a/unittests/BuildSystem/BuildSystemFrontendTest.cpp +++ b/unittests/BuildSystem/BuildSystemFrontendTest.cpp @@ -742,7 +742,7 @@ TEST(BuildSystemInvocationTest, formatCycle) { public: NullRule(const KeyType& key) : Rule(key) { } Task* createTask(core::BuildEngine&) override { return nullptr; } - bool isResultValid(core::BuildEngine&, const core::ValueType&) override { + ValidationResult isResultValid(core::BuildEngine&, const core::ValueType&) override { return true; } }; diff --git a/unittests/BuildSystem/MockBuildSystemDelegate.h b/unittests/BuildSystem/MockBuildSystemDelegate.h index f58fe77b9..84dd0ed2f 100644 --- a/unittests/BuildSystem/MockBuildSystemDelegate.h +++ b/unittests/BuildSystem/MockBuildSystemDelegate.h @@ -204,7 +204,7 @@ class MockBuildSystemDelegate : public BuildSystemDelegate { } } - virtual void determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule) { } + virtual void determinedRuleNeedsToRun(core::Rule* ruleNeedingToRun, core::Rule::RunReason reason, core::Rule* inputRule, StringRef details) { } }; } diff --git a/unittests/Core/BuildEngineCancellationTest.cpp b/unittests/Core/BuildEngineCancellationTest.cpp index ed78e4cc5..ce81bebc4 100644 --- a/unittests/Core/BuildEngineCancellationTest.cpp +++ b/unittests/Core/BuildEngineCancellationTest.cpp @@ -139,7 +139,7 @@ class SimpleRule: public Rule { return new SimpleTask([this]{ return inputs; }, compute); } - bool isResultValid(BuildEngine&, const ValueType&) override { return true; } + ValidationResult isResultValid(BuildEngine&, const ValueType&) override { return true; } }; diff --git a/unittests/Core/BuildEngineTest.cpp b/unittests/Core/BuildEngineTest.cpp index b35d709c3..69bf37ee0 100644 --- a/unittests/Core/BuildEngineTest.cpp +++ b/unittests/Core/BuildEngineTest.cpp @@ -159,7 +159,7 @@ class SimpleRule: public Rule { Task* createTask(BuildEngine&) override { return new SimpleTask([this]{ return inputs; }, compute); } - bool isResultValid(BuildEngine&, const ValueType& value) override { + ValidationResult isResultValid(BuildEngine&, const ValueType& value) override { if (!valid) return true; return valid(value); } @@ -653,7 +653,7 @@ TEST(BuildEngineTest, discoveredDependencies) { builtKeys.push_back(key.str()); return new TaskWithDiscoveredDependency(dep); } - bool isResultValid(BuildEngine&, const ValueType&) override { return true; } + ValidationResult isResultValid(BuildEngine&, const ValueType&) override { return true; } }; engine.addRule(std::unique_ptr(new RuleWithDiscoveredDependency("output", builtKeys, valueB))); @@ -794,7 +794,7 @@ TEST(BuildEngineTest, CycleDuringScanningFromTop) { Task* createTask(BuildEngine&) override { return new SimpleTask(input, compute); } - bool isResultValid(BuildEngine&, const ValueType&) override { + ValidationResult isResultValid(BuildEngine&, const ValueType&) override { return valid; } }; diff --git a/unittests/Core/DepsBuildEngineTest.cpp b/unittests/Core/DepsBuildEngineTest.cpp index ac2a5f9b6..e366b4440 100644 --- a/unittests/Core/DepsBuildEngineTest.cpp +++ b/unittests/Core/DepsBuildEngineTest.cpp @@ -133,7 +133,7 @@ class SimpleRule: public Rule { Task* createTask(BuildEngine&) override { return new SimpleTask(inputs, compute); } - bool isResultValid(BuildEngine&, const ValueType& value) override { + ValidationResult isResultValid(BuildEngine&, const ValueType& value) override { if (!valid) return true; return valid(value); } @@ -237,7 +237,7 @@ TEST(DepsBuildEngineTest, BogusConcurrentDepScan) { builtKeys.push_back(key); return new DynamicTask(); } - bool isResultValid(BuildEngine&, const ValueType&) override { return true; } + ValidationResult isResultValid(BuildEngine&, const ValueType&) override { return true; } }; engine.addRule(std::unique_ptr(new DynamicRule("output", builtKeys)));