Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly match regex with tree artifact #16949

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -548,11 +548,19 @@ public void finalizeAction(Action action, MetadataHandler metadataHandler) {
inputsToDownload.add(output);
}

for (Pattern pattern : patternsToDownload) {
if (pattern.matcher(output.getExecPathString()).matches()) {
outputsToDownload.add(output);
break;
if (output.isTreeArtifact()) {
var children = metadataHandler.getTreeArtifactChildren((SpecialArtifact) output);
if (outputMatchesPattern(output)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the pattern has a $, would this incorrectly match tree artifact paths?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a concrete example?

If the pattern matches tree root, all files inside the tree are downloaded. (See newly added test cases)

Copy link
Contributor

@brentleyjones brentleyjones Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that seems like a bug to me, since if we weren't using a tree artifact we wouldn't download everything under a/path with the pattern a/path$ (for example, if we had an output of a/path/and/more/lib.a).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is guarded by output.isTreeArtifact(). File output a/path/and/more/lib.a won't match pattern a/path$ so it won't be downloaded. Added another test case to illustrate that.

Copy link
Contributor

@brentleyjones brentleyjones Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I'm saying that the behavior shouldn't differ between if it's a tree artifact or not. We shouldn't match on the tree artifact itself if it has children, if using $ in the pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If I change the lib.a example from a normal output to a tree artifact output, the download behavior of any patterns shouldn't change.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Good catch! Updated the code.

outputsToDownload.addAll(children);
} else {
for (var file : children) {
if (outputMatchesPattern(file)) {
outputsToDownload.add(file);
}
}
}
} else if (outputMatchesPattern(output)) {
outputsToDownload.add(output);
}
}

Expand All @@ -565,6 +573,15 @@ public void finalizeAction(Action action, MetadataHandler metadataHandler) {
}
}

private boolean outputMatchesPattern(Artifact output) {
for (var pattern : patternsToDownload) {
if (pattern.matcher(output.getExecPathString()).matches()) {
return true;
}
}
return false;
}

public void flushOutputTree() throws InterruptedException {
downloadCache.awaitInProgressTasks();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,86 @@ public void downloadOutputsWithRegex() throws Exception {
assertOutputsDoNotExist("//:foobar");
}

@Test
public void downloadOutputsWithRegex_treeOutput_regexMatchesTreeFile() throws Exception {
// Disable on Windows since it fails for unknown reasons.
// TODO(chiwang): Enable it on windows.
if (OS.getCurrent() == OS.WINDOWS) {
return;
}

writeOutputDirRule();
write(
"BUILD",
"load(':output_dir.bzl', 'output_dir')",
"output_dir(",
" name = 'foo',",
" manifest = ':manifest',",
")");
write("manifest", "file-1", "file-2", "file-3");
addOptions("--experimental_remote_download_regex=.*foo/file-2$");

buildTarget("//:foo");
waitDownloads();

assertValidOutputFile("foo/file-2", "file-2\n");
assertOutputDoesNotExist("foo/file-1");
assertOutputDoesNotExist("foo/file-3");
}

@Test
public void downloadOutputsWithRegex_treeOutput_regexMatchesTreeRoot() throws Exception {
writeOutputDirRule();
write(
"BUILD",
"load(':output_dir.bzl', 'output_dir')",
"output_dir(",
" name = 'foo',",
" manifest = ':manifest',",
")");
write("manifest", "file-1", "file-2", "file-3");
addOptions("--experimental_remote_download_regex=.*foo$");

buildTarget("//:foo");
waitDownloads();

assertValidOutputFile("foo/file-1", "file-1\n");
assertValidOutputFile("foo/file-2", "file-2\n");
assertValidOutputFile("foo/file-3", "file-3\n");
}

@Test
public void downloadOutputsWithRegex_regexMatchParentPath_filesNotDownloaded() throws Exception {
write(
"BUILD",
"genrule(",
" name = 'file-1',",
" srcs = [],",
" outs = ['foo/file-1'],",
" cmd = 'echo file-1 > $@',",
")",
"genrule(",
" name = 'file-2',",
" srcs = [],",
" outs = ['foo/file-2'],",
" cmd = 'echo file-2 > $@',",
")",
"genrule(",
" name = 'file-3',",
" srcs = [],",
" outs = ['foo/file-3'],",
" cmd = 'echo file-3 > $@',",
")");
addOptions("--experimental_remote_download_regex=.*foo$");

buildTarget("//:file-1", "//:file-2", "//:file-3");
waitDownloads();

assertOutputDoesNotExist("foo/file-1");
assertOutputDoesNotExist("foo/file-2");
assertOutputDoesNotExist("foo/file-3");
}

@Test
public void intermediateOutputsAreInputForLocalActions_prefetchIntermediateOutputs()
throws Exception {
Expand Down