Skip to content

Commit

Permalink
Handle SkippedCommand values in DirectoryContents
Browse files Browse the repository at this point in the history
Under some cancellation conditions, the directory value observed by
DirectoryContentsTask may be a SkippedCommand. Prior to this change, the
provideInput() handler was assuming that such values had output info and
would read a garbage value and crash (in release mode).

rdar://problem/50380532
  • Loading branch information
dmbryson committed Jun 27, 2019
1 parent a75c546 commit 1808e05
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 1 deletion.
9 changes: 8 additions & 1 deletion lib/BuildSystem/BuildSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,13 @@ class DirectoryContentsTask : public Task {
return;
}

// The input directory may be a 'mkdir' command, which can be cancelled or
// skipped by the engine or the delegate. rdar://problem/50380532
if (directoryValue.isSkippedCommand()) {
engine.taskIsComplete(this, BuildValue::makeSkippedCommand().toData());
return;
}

std::vector<std::string> filenames;
getContents(path, filenames);

Expand Down Expand Up @@ -1294,7 +1301,7 @@ class DirectoryTreeStructureSignatureTask : public Task {

// Request the inputs for each subpath.
auto value = BuildValue::fromData(valueData);
if (value.isMissingInput())
if (value.isMissingInput() || value.isSkippedCommand())
return;

assert(value.isDirectoryContents());
Expand Down
88 changes: 88 additions & 0 deletions unittests/BuildSystem/BuildSystemTaskTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1078,3 +1078,91 @@ TEST(BuildSystemTaskTests, producedNodeAfterPreviouslyMissing) {
ASSERT_TRUE(!afterFileInfo.isMissing());
}
}

/// Check that directory contents properly handles when commands have been
/// skipped. rdar://problem/50380532
TEST(BuildSystemTaskTests, directoryContentsWithSkippedCommand) {
TmpDir tempDir(__func__);

SmallString<256> manifest{ tempDir.str() };
for (auto& c : manifest) {
if (c == '\\')
c = '/';
}
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 <<
"client:\n"
" name: mock\n"
"\n"
"targets:\n"
" \"\": [\"<all>\"]\n"
"\n"
"commands:\n"
" \"mkdir-inputDir\":\n"
" tool: mkdir\n"
" outputs: [\"inputDir\"]\n"
" \"read-inputDir\":\n"
" tool: shell\n"
" inputs: [\"inputDir/\"]\n"
" outputs: [\"read-inputDir\"]\n"
" description: \"read-inputDir\"\n"
" args:\n"
" touch read-inputDir\n"
" \"<all>\":\n"
" tool: phony\n"
" inputs: [\"read-inputDir\"]\n"
" outputs: [\"<all>\"]";
}

class CancellingDelegate: public MockBuildSystemDelegate {
public:
BuildSystem* system = nullptr;

CancellingDelegate(): MockBuildSystemDelegate() { }

virtual bool shouldCommandStart(Command* command) override {
if (command->getName() == "mkdir-inputDir") {
return false;
}
return true;
}
};

// Create the build system.
CancellingDelegate delegate;
BuildSystem system(delegate, createLocalFileSystem());
delegate.system = &system;
bool loadingResult = system.loadDescription(manifest);
ASSERT_TRUE(loadingResult);

// Build the default target
auto result = system.build(BuildKey::makeTarget(""));
ASSERT_TRUE(result.hasValue());

// The contents should propagate the skipped command
result = system.build(BuildKey::makeDirectoryContents("inputDir"));
ASSERT_TRUE(result.hasValue());
ASSERT_TRUE(result.getValue().isSkippedCommand());

// Signatures don't need the contents and should be fine with encoding the
// skipped value in the signature.
result = system.build(BuildKey::makeDirectoryTreeSignature("inputDir", {}));
ASSERT_TRUE(result.hasValue());
ASSERT_FALSE(result.getValue().isSkippedCommand());

auto filters = StringList({"filter"});
result = system.build(BuildKey::makeDirectoryTreeSignature("inputDir", filters));
ASSERT_TRUE(result.hasValue());
ASSERT_FALSE(result.getValue().isSkippedCommand());

result = system.build(BuildKey::makeDirectoryTreeStructureSignature("inputDir"));
ASSERT_TRUE(result.hasValue());
ASSERT_FALSE(result.getValue().isSkippedCommand());
}

0 comments on commit 1808e05

Please sign in to comment.