From 1cc9731c50ac545168085e3734653171951ead57 Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Wed, 8 Jun 2022 02:43:32 -0500 Subject: [PATCH 01/10] Add a flag to force Bazel to download certain artifacts When using --remote_download_minimal Bazel only downloads to the client the minimum number of artifacts necessary for the build to succeed. This can be sometimes problematic when local development (i.e. IDE) requires some artifacts to do local work (e.g. syntax completion). We add a flag --experimental_force_downloads_regex that allow specifying a regular expression that will force certain artifacts to be forced to downloaded. Tested locally that syntax errors disappear in IntelliJ when compiling Bazel itself with --remote_download_minimal and --experimental_force_downloads_regex=".*jar$", also included an integration test with several cases. Signed-off-by: Luis Fernando Pino Duque --- .../lib/remote/RemoteExecutionService.java | 79 +++++++++++++++---- .../lib/remote/options/RemoteOptions.java | 10 +++ .../bazel/remote/remote_execution_test.sh | 57 +++++++++++++ 3 files changed, 129 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 27132d528c1961..cafabd02944d9b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -147,6 +147,9 @@ import java.util.concurrent.Executor; import java.util.concurrent.Phaser; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Predicate; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.annotation.Nullable; /** @@ -174,6 +177,8 @@ public class RemoteExecutionService { private final AtomicBoolean shutdown = new AtomicBoolean(false); private final AtomicBoolean buildInterrupted = new AtomicBoolean(false); + private final boolean shouldForceDownloads; + private final Predicate shouldForceDownloadPredicate; public RemoteExecutionService( Executor executor, @@ -215,6 +220,16 @@ public RemoteExecutionService( this.captureCorruptedOutputsDir = captureCorruptedOutputsDir; this.scheduler = Schedulers.from(executor, /*interruptibleWorker=*/ true); + + String regex = remoteOptions.experimentalForceDownloadsRegex; + // TODO(bazel-team): Consider adding a warning or more validation if the experimentalForceDownloadsRegex is used + // without RemoteOutputsMode.MINIMAL + this.shouldForceDownloads = !regex.isEmpty() && remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL; + Pattern pattern = Pattern.compile(regex); + this.shouldForceDownloadPredicate = path -> { + Matcher m = pattern.matcher(path); + return m.matches(); + }; } static Command buildCommand( @@ -1021,24 +1036,9 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re /* exitCode = */ result.getExitCode(), hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload)); + ImmutableList.Builder> forcedDownloadsBuilder = ImmutableList.builder(); if (downloadOutputs) { - HashSet queuedFilePaths = new HashSet<>(); - - for (FileMetadata file : metadata.files()) { - PathFragment filePath = file.path().asFragment(); - if (queuedFilePaths.add(filePath)) { - downloadsBuilder.add(downloadFile(action, file)); - } - } - - for (Map.Entry entry : metadata.directories()) { - for (FileMetadata file : entry.getValue().files()) { - PathFragment filePath = file.path().asFragment(); - if (queuedFilePaths.add(filePath)) { - downloadsBuilder.add(downloadFile(action, file)); - } - } - } + downloadsBuilder.addAll(buildFilesToDownload(metadata, action)); } else { checkState( result.getExitCode() == 0, @@ -1049,6 +1049,9 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re "Symlinks in action outputs are not yet supported by " + "--experimental_remote_download_outputs=minimal"); } + if (shouldForceDownloads) { + forcedDownloadsBuilder.addAll(buildFilesToDownloadWithPredicate(metadata, action, shouldForceDownloadPredicate)); + } } FileOutErr tmpOutErr = outErr.childOutErr(); @@ -1068,6 +1071,15 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re throw e; } + ImmutableList> forcedDownloads = forcedDownloadsBuilder.build(); + try (SilentCloseable c = Profiler.instance().profile("Remote.forcedDownload")) { + waitForBulkTransfer(forcedDownloads, /* cancelRemainingOnInterrupt= */ true); + } catch (Exception e) { + captureCorruptedOutputs(e); + deletePartialDownloadedOutputs(result, tmpOutErr, e); + throw e; + } + FileOutErr.dump(tmpOutErr, outErr); // Ensure that we are the only ones writing to the output files when using the dynamic spawn @@ -1079,6 +1091,10 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re tmpOutErr.clearOut(); tmpOutErr.clearErr(); + if (!forcedDownloads.isEmpty()) { + moveOutputsToFinalLocation(forcedDownloads); + } + if (downloadOutputs) { moveOutputsToFinalLocation(downloads); @@ -1133,6 +1149,35 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } + private ImmutableList> buildFilesToDownload( + ActionResultMetadata metadata, RemoteAction action) { + Predicate alwaysTrue = unused -> true; + return buildFilesToDownloadWithPredicate(metadata, action, alwaysTrue); + } + + private ImmutableList> buildFilesToDownloadWithPredicate( + ActionResultMetadata metadata, RemoteAction action, Predicate predicate) { + HashSet queuedFilePaths = new HashSet<>(); + ImmutableList.Builder> builder = new ImmutableList.Builder<>(); + for (FileMetadata file : metadata.files()) { + PathFragment filePath = file.path().asFragment(); + if (queuedFilePaths.add(filePath) && predicate.test(file.path.toString())) { + builder.add(downloadFile(action, file)); + } + } + + for (Map.Entry entry : metadata.directories()) { + for (FileMetadata file : entry.getValue().files()) { + PathFragment filePath = file.path().asFragment(); + if (queuedFilePaths.add(filePath) && predicate.test(file.path.toString())) { + builder.add(downloadFile(action, file)); + } + } + } + + return builder.build(); + } + private static String prettyPrint(ActionInput actionInput) { if (actionInput instanceof Artifact) { return ((Artifact) actionInput).prettyPrint(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index cf5621ba9b5604..483e9fc9c481f3 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -574,6 +574,16 @@ public RemoteOutputsStrategyConverter() { help = "Maximum number of open files allowed during BEP artifact upload.") public int maximumOpenFiles; + @Option( + name = "experimental_force_downloads_regex", + defaultValue = "", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + help = "Force Bazel to download the artifacts that match the given regexp. To be used in conjunction with" + + "--remote_download_minimal to allow the client to request certain artifacts that might be needed" + + "locally (e.g. IDE support)") + public String experimentalForceDownloadsRegex; + // The below options are not configurable by users, only tests. // This is part of the effort to reduce the overall number of flags. diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 30a4b6913d7155..79b6447baf3b40 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3347,6 +3347,63 @@ EOF expect_log "2 processes: 1 internal, 1 remote" } +function test_forced_downloads() { + mkdir -p a + + cat > a/BUILD <<'EOF' +java_library( + name = "lib", + srcs = ["Library.java"], +) +java_test( + name = "test", + srcs = ["JavaTest.java"], + test_class = "JavaTest", + deps = [":lib"], +) +EOF + + cat > a/Library.java <<'EOF' +public class Library { + public static boolean TEST = true; +} +EOF + + cat > a/JavaTest.java <<'EOF' +import org.junit.Assert; +import org.junit.Test; +public class JavaTest { + @Test + public void test() { Assert.assertTrue(Library.TEST); } +} +EOF + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //a:test >& $TEST_log || fail "Failed to build" + + [[ ! -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar shouldn't exist" + [[ ! -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps shouldn't exist" + + bazel clean && bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + --experimental_force_downloads_regex=".*" \ + //a:test >& $TEST_log || fail "Failed to build" + + [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!" + [[ -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps file does not exist!" + + bazel clean && bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + --experimental_force_downloads_regex=".*jar$" \ + //a:test >& $TEST_log || fail "Failed to build" + + [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!" + [[ ! -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps shouldn't exist" +} + function test_grpc_connection_errors_are_propagated() { # Test that errors when creating grpc connection are propagated instead of crashing Bazel. # https://github.com/bazelbuild/bazel/issues/13724 From 1fa3b54ff5654af374d2f9aba442597b5c4f6d6c Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Tue, 14 Jun 2022 13:38:51 -0500 Subject: [PATCH 02/10] Address most comments Signed-off-by: Luis Fernando Pino Duque --- .../lib/remote/RemoteExecutionService.java | 21 +++++++++++-------- .../lib/remote/options/RemoteOptions.java | 10 ++++----- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index cafabd02944d9b..e47971c4b8e2f0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -221,10 +221,13 @@ public RemoteExecutionService( this.scheduler = Schedulers.from(executor, /*interruptibleWorker=*/ true); - String regex = remoteOptions.experimentalForceDownloadsRegex; + String regex = remoteOptions.remoteDownloadRegex; // TODO(bazel-team): Consider adding a warning or more validation if the experimentalForceDownloadsRegex is used - // without RemoteOutputsMode.MINIMAL - this.shouldForceDownloads = !regex.isEmpty() && remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL; + // without RemoteOutputsMode.MINIMAL or RemoteOutputsMode.TOPLEVEL + this.shouldForceDownloads = + !regex.isEmpty() + && (remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL + || remoteOptions.remoteOutputsMode == RemoteOutputsMode.TOPLEVEL); Pattern pattern = Pattern.compile(regex); this.shouldForceDownloadPredicate = path -> { Matcher m = pattern.matcher(path); @@ -1041,13 +1044,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re downloadsBuilder.addAll(buildFilesToDownload(metadata, action)); } else { checkState( - result.getExitCode() == 0, - "injecting remote metadata is only supported for successful actions (exit code 0)."); + result.getExitCode() == 0, + "injecting remote metadata is only supported for successful actions (exit code 0)."); if (!metadata.symlinks().isEmpty()) { throw new IOException( - "Symlinks in action outputs are not yet supported by " - + "--experimental_remote_download_outputs=minimal"); + "Symlinks in action outputs are not yet supported by " + + "--experimental_remote_download_outputs=minimal"); } if (shouldForceDownloads) { forcedDownloadsBuilder.addAll(buildFilesToDownloadWithPredicate(metadata, action, shouldForceDownloadPredicate)); @@ -1150,13 +1153,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } private ImmutableList> buildFilesToDownload( - ActionResultMetadata metadata, RemoteAction action) { + ActionResultMetadata metadata, com.google.devtools.build.lib.remote.RemoteAction action) { Predicate alwaysTrue = unused -> true; return buildFilesToDownloadWithPredicate(metadata, action, alwaysTrue); } private ImmutableList> buildFilesToDownloadWithPredicate( - ActionResultMetadata metadata, RemoteAction action, Predicate predicate) { + ActionResultMetadata metadata, com.google.devtools.build.lib.remote.RemoteAction action, Predicate predicate) { HashSet queuedFilePaths = new HashSet<>(); ImmutableList.Builder> builder = new ImmutableList.Builder<>(); for (FileMetadata file : metadata.files()) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 483e9fc9c481f3..2f5a0873ac5b34 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -575,14 +575,14 @@ public RemoteOutputsStrategyConverter() { public int maximumOpenFiles; @Option( - name = "experimental_force_downloads_regex", + name = "experimental_remote_download_regex", defaultValue = "", documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, - help = "Force Bazel to download the artifacts that match the given regexp. To be used in conjunction with" + - "--remote_download_minimal to allow the client to request certain artifacts that might be needed" + - "locally (e.g. IDE support)") - public String experimentalForceDownloadsRegex; + help = "Force Bazel to download the artifacts that match the given regexp. To be used in conjunction with " + + "--remote_download_minimal or --remote_download_toplevel to allow the client to request certain " + + "artifacts that might be needed locally (e.g. IDE support)") + public String remoteDownloadRegex; // The below options are not configurable by users, only tests. // This is part of the effort to reduce the overall number of flags. From 1e6ea205e2ea817c0d5690fb750df2b82fb9514a Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Tue, 14 Jun 2022 15:36:45 -0500 Subject: [PATCH 03/10] Unify builders Signed-off-by: Luis Fernando Pino Duque --- .../lib/remote/RemoteExecutionService.java | 35 ++++++------------- 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index e47971c4b8e2f0..5bded4c7bef016 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -223,11 +223,10 @@ public RemoteExecutionService( String regex = remoteOptions.remoteDownloadRegex; // TODO(bazel-team): Consider adding a warning or more validation if the experimentalForceDownloadsRegex is used - // without RemoteOutputsMode.MINIMAL or RemoteOutputsMode.TOPLEVEL + // without RemoteOutputsMode.MINIMAL this.shouldForceDownloads = !regex.isEmpty() - && (remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL - || remoteOptions.remoteOutputsMode == RemoteOutputsMode.TOPLEVEL); + && remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL; Pattern pattern = Pattern.compile(regex); this.shouldForceDownloadPredicate = path -> { Matcher m = pattern.matcher(path); @@ -1039,21 +1038,20 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re /* exitCode = */ result.getExitCode(), hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload)); - ImmutableList.Builder> forcedDownloadsBuilder = ImmutableList.builder(); if (downloadOutputs) { downloadsBuilder.addAll(buildFilesToDownload(metadata, action)); } else { checkState( - result.getExitCode() == 0, - "injecting remote metadata is only supported for successful actions (exit code 0)."); + result.getExitCode() == 0, + "injecting remote metadata is only supported for successful actions (exit code 0)."); if (!metadata.symlinks().isEmpty()) { throw new IOException( - "Symlinks in action outputs are not yet supported by " - + "--experimental_remote_download_outputs=minimal"); + "Symlinks in action outputs are not yet supported by " + + "--experimental_remote_download_outputs=minimal"); } if (shouldForceDownloads) { - forcedDownloadsBuilder.addAll(buildFilesToDownloadWithPredicate(metadata, action, shouldForceDownloadPredicate)); + downloadsBuilder.addAll(buildFilesToDownloadWithPredicate(metadata, action, shouldForceDownloadPredicate)); } } @@ -1074,15 +1072,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re throw e; } - ImmutableList> forcedDownloads = forcedDownloadsBuilder.build(); - try (SilentCloseable c = Profiler.instance().profile("Remote.forcedDownload")) { - waitForBulkTransfer(forcedDownloads, /* cancelRemainingOnInterrupt= */ true); - } catch (Exception e) { - captureCorruptedOutputs(e); - deletePartialDownloadedOutputs(result, tmpOutErr, e); - throw e; - } - FileOutErr.dump(tmpOutErr, outErr); // Ensure that we are the only ones writing to the output files when using the dynamic spawn @@ -1094,10 +1083,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re tmpOutErr.clearOut(); tmpOutErr.clearErr(); - if (!forcedDownloads.isEmpty()) { - moveOutputsToFinalLocation(forcedDownloads); - } - if (downloadOutputs) { moveOutputsToFinalLocation(downloads); @@ -1114,6 +1099,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // Create the symbolic links after all downloads are finished, because dangling symlinks // might not be supported on all platforms createSymlinks(symlinks); + } else if (!downloads.isEmpty()) { + moveOutputsToFinalLocation(downloads); } else { ActionInput inMemoryOutput = null; Digest inMemoryOutputDigest = null; @@ -1153,13 +1140,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } private ImmutableList> buildFilesToDownload( - ActionResultMetadata metadata, com.google.devtools.build.lib.remote.RemoteAction action) { + ActionResultMetadata metadata, RemoteAction action) { Predicate alwaysTrue = unused -> true; return buildFilesToDownloadWithPredicate(metadata, action, alwaysTrue); } private ImmutableList> buildFilesToDownloadWithPredicate( - ActionResultMetadata metadata, com.google.devtools.build.lib.remote.RemoteAction action, Predicate predicate) { + ActionResultMetadata metadata, RemoteAction action, Predicate predicate) { HashSet queuedFilePaths = new HashSet<>(); ImmutableList.Builder> builder = new ImmutableList.Builder<>(); for (FileMetadata file : metadata.files()) { From 01a26a42dafc376d731a7bcc2d33777d667ad4c0 Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Tue, 14 Jun 2022 16:05:36 -0500 Subject: [PATCH 04/10] Revert and push green version Signed-off-by: Luis Fernando Pino Duque --- .../lib/remote/RemoteExecutionService.java | 22 +++++++++++++++---- .../bazel/remote/remote_execution_test.sh | 6 ++--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 5bded4c7bef016..2d5a295ef89196 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -226,7 +226,8 @@ public RemoteExecutionService( // without RemoteOutputsMode.MINIMAL this.shouldForceDownloads = !regex.isEmpty() - && remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL; + && (remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL + || remoteOptions.remoteOutputsMode == RemoteOutputsMode.TOPLEVEL); Pattern pattern = Pattern.compile(regex); this.shouldForceDownloadPredicate = path -> { Matcher m = pattern.matcher(path); @@ -1038,6 +1039,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re /* exitCode = */ result.getExitCode(), hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload)); + ImmutableList.Builder> forcedDownloadsBuilder = ImmutableList.builder(); + if (downloadOutputs) { downloadsBuilder.addAll(buildFilesToDownload(metadata, action)); } else { @@ -1051,7 +1054,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re + "--experimental_remote_download_outputs=minimal"); } if (shouldForceDownloads) { - downloadsBuilder.addAll(buildFilesToDownloadWithPredicate(metadata, action, shouldForceDownloadPredicate)); + forcedDownloadsBuilder.addAll(buildFilesToDownloadWithPredicate(metadata, action, shouldForceDownloadPredicate)); } } @@ -1072,6 +1075,15 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re throw e; } + ImmutableList> forcedDownloads = forcedDownloadsBuilder.build(); + try (SilentCloseable c = Profiler.instance().profile("Remote.forcedDownload")) { + waitForBulkTransfer(forcedDownloads, /* cancelRemainingOnInterrupt= */ true); + } catch (Exception e) { + captureCorruptedOutputs(e); + deletePartialDownloadedOutputs(result, tmpOutErr, e); + throw e; + } + FileOutErr.dump(tmpOutErr, outErr); // Ensure that we are the only ones writing to the output files when using the dynamic spawn @@ -1083,6 +1095,10 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re tmpOutErr.clearOut(); tmpOutErr.clearErr(); + if (!forcedDownloads.isEmpty()) { + moveOutputsToFinalLocation(forcedDownloads); + } + if (downloadOutputs) { moveOutputsToFinalLocation(downloads); @@ -1099,8 +1115,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // Create the symbolic links after all downloads are finished, because dangling symlinks // might not be supported on all platforms createSymlinks(symlinks); - } else if (!downloads.isEmpty()) { - moveOutputsToFinalLocation(downloads); } else { ActionInput inMemoryOutput = null; Digest inMemoryOutputDigest = null; diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 79b6447baf3b40..7591cdcae0e5b8 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3347,7 +3347,7 @@ EOF expect_log "2 processes: 1 internal, 1 remote" } -function test_forced_downloads() { +function test_remote_download_regex() { mkdir -p a cat > a/BUILD <<'EOF' @@ -3388,7 +3388,7 @@ EOF bazel clean && bazel test \ --remote_executor=grpc://localhost:${worker_port} \ --remote_download_minimal \ - --experimental_force_downloads_regex=".*" \ + --experimental_remote_download_regex=".*" \ //a:test >& $TEST_log || fail "Failed to build" [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!" @@ -3397,7 +3397,7 @@ EOF bazel clean && bazel test \ --remote_executor=grpc://localhost:${worker_port} \ --remote_download_minimal \ - --experimental_force_downloads_regex=".*jar$" \ + --experimental_remote_download_regex=".*jar$" \ //a:test >& $TEST_log || fail "Failed to build" [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!" From ecc33b92e9708e163ebce2a511e8749bbc38cf16 Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Tue, 5 Jul 2022 16:44:44 -0500 Subject: [PATCH 05/10] Move tests Signed-off-by: Luis Fernando Pino Duque --- .../remote/build_without_the_bytes_test.sh | 57 ++++++ .../bazel/remote/remote_execution_test.sh | 179 ------------------ 2 files changed, 57 insertions(+), 179 deletions(-) diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh index 4cdecf01ca71d7..4528ab18e3d167 100755 --- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh +++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh @@ -1354,4 +1354,61 @@ EOF expect_log "START.*: \[.*\] Executing genrule //a:dep" } +function test_remote_download_regex() { + mkdir -p a + + cat > a/BUILD <<'EOF' +java_library( + name = "lib", + srcs = ["Library.java"], +) +java_test( + name = "test", + srcs = ["JavaTest.java"], + test_class = "JavaTest", + deps = [":lib"], +) +EOF + + cat > a/Library.java <<'EOF' +public class Library { + public static boolean TEST = true; +} +EOF + + cat > a/JavaTest.java <<'EOF' +import org.junit.Assert; +import org.junit.Test; +public class JavaTest { + @Test + public void test() { Assert.assertTrue(Library.TEST); } +} +EOF + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //a:test >& $TEST_log || fail "Failed to build" + + [[ ! -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar shouldn't exist" + [[ ! -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps shouldn't exist" + + bazel clean && bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + --experimental_remote_download_regex=".*" \ + //a:test >& $TEST_log || fail "Failed to build" + + [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!" + [[ -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps file does not exist!" + + bazel clean && bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + --experimental_remote_download_regex=".*jar$" \ + //a:test >& $TEST_log || fail "Failed to build" + + [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!" + [[ ! -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps shouldn't exist" +} + run_suite "Build without the Bytes tests" diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 5e195b24e177c5..854d866a75cefe 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -2290,117 +2290,6 @@ EOF fi } -# Test that when testing with --remote_download_minimal, Bazel doesn't -# regenerate the test.xml if the action actually produced it. See -# https://github.com/bazelbuild/bazel/issues/12554 -function test_remote_download_minimal_with_test_xml_generation() { - mkdir -p a - - cat > a/BUILD <<'EOF' -sh_test( - name = "test0", - srcs = ["test.sh"], -) - -java_test( - name = "test1", - srcs = ["JavaTest.java"], - test_class = "JavaTest", -) -EOF - - cat > a/test.sh <<'EOF' -#!/bin/bash -echo 'Hello' -EOF - chmod a+x a/test.sh - - cat > a/JavaTest.java <<'EOF' -import org.junit.Test; - -public class JavaTest { - @Test - public void test() {} -} -EOF - - bazel build \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_minimal \ - //a:test0 //a:test1 >& $TEST_log || fail "Failed to build" - - bazel test \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_minimal \ - //a:test0 >& $TEST_log || fail "Failed to test" - # 2 remote spawns: 1 for executing the test, 1 for generating the test.xml - expect_log "2 processes: 2 remote" - - bazel test \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_minimal \ - //a:test1 >& $TEST_log || fail "Failed to test" - # only 1 remote spawn: test.xml is generated by junit - expect_log "2 processes: 1 internal, 1 remote" -} - -function test_remote_download_regex() { - mkdir -p a - - cat > a/BUILD <<'EOF' -java_library( - name = "lib", - srcs = ["Library.java"], -) -java_test( - name = "test", - srcs = ["JavaTest.java"], - test_class = "JavaTest", - deps = [":lib"], -) -EOF - - cat > a/Library.java <<'EOF' -public class Library { - public static boolean TEST = true; -} -EOF - - cat > a/JavaTest.java <<'EOF' -import org.junit.Assert; -import org.junit.Test; -public class JavaTest { - @Test - public void test() { Assert.assertTrue(Library.TEST); } -} -EOF - bazel test \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_minimal \ - //a:test >& $TEST_log || fail "Failed to build" - - [[ ! -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar shouldn't exist" - [[ ! -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps shouldn't exist" - - bazel clean && bazel test \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_minimal \ - --experimental_remote_download_regex=".*" \ - //a:test >& $TEST_log || fail "Failed to build" - - [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!" - [[ -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps file does not exist!" - - bazel clean && bazel test \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_minimal \ - --experimental_remote_download_regex=".*jar$" \ - //a:test >& $TEST_log || fail "Failed to build" - - [[ -e "bazel-bin/a/liblib.jar" ]] || fail "bazel-bin/a/liblib.jar file does not exist!" - [[ ! -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps shouldn't exist" -} - function test_grpc_connection_errors_are_propagated() { # Test that errors when creating grpc connection are propagated instead of crashing Bazel. # https://github.com/bazelbuild/bazel/issues/13724 @@ -2422,74 +2311,6 @@ EOF expect_log "ERROR: Failed to query remote execution capabilities: Failed to init TLS infrastructure using '/nope' as root certificate: File does not contain valid certificates: /nope" } -function test_output_file_permission() { - # Test that permission of output files are always 0555 - - mkdir -p a - cat > a/BUILD <& $TEST_log || fail "Failed to build" - - ls -l bazel-bin/a/bar >& $TEST_log - expect_log "-r-xr-xr-x" - - ls -l bazel-bin/a/foo >& $TEST_log - expect_log "-r-xr-xr-x" - - cat bazel-bin/a/bar >& $TEST_log - expect_log "-r-xr-xr-x" - - bazel clean >& $TEST_log || fail "Failed to clean" - - # normal remote execution - bazel build \ - --remote_executor=grpc://localhost:${worker_port} \ - //a:bar >& $TEST_log || fail "Failed to build" - - ls -l bazel-bin/a/bar >& $TEST_log - expect_log "-r-xr-xr-x" - - ls -l bazel-bin/a/foo >& $TEST_log - expect_log "-r-xr-xr-x" - - cat bazel-bin/a/bar >& $TEST_log - expect_log "-r-xr-xr-x" - - bazel clean >& $TEST_log || fail "Failed to clean" - - # build without bytes - bazel build \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_minimal \ - //a:bar >& $TEST_log || fail "Failed to build" - - ls -l bazel-bin/a/bar >& $TEST_log - expect_log "-r-xr-xr-x" - - ls -l bazel-bin/a/foo >& $TEST_log - expect_log "-r-xr-xr-x" - - cat bazel-bin/a/bar >& $TEST_log - expect_log "-r-xr-xr-x" -} - function test_async_upload_works_for_flaky_tests() { mkdir -p a cat > a/BUILD < Date: Tue, 5 Jul 2022 17:04:18 -0500 Subject: [PATCH 06/10] Address comments Signed-off-by: Luis Fernando Pino Duque --- .../lib/remote/RemoteExecutionService.java | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 2d5a295ef89196..b9b1d694aa7818 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -222,7 +222,7 @@ public RemoteExecutionService( this.scheduler = Schedulers.from(executor, /*interruptibleWorker=*/ true); String regex = remoteOptions.remoteDownloadRegex; - // TODO(bazel-team): Consider adding a warning or more validation if the experimentalForceDownloadsRegex is used + // TODO(bazel-team): Consider adding a warning or more validation if the remoteDownloadRegex is used // without RemoteOutputsMode.MINIMAL this.shouldForceDownloads = !regex.isEmpty() @@ -1067,20 +1067,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } ImmutableList> downloads = downloadsBuilder.build(); - try (SilentCloseable c = Profiler.instance().profile("Remote.download")) { - waitForBulkTransfer(downloads, /* cancelRemainingOnInterrupt= */ true); - } catch (Exception e) { - captureCorruptedOutputs(e); - deletePartialDownloadedOutputs(result, tmpOutErr, e); - throw e; - } - ImmutableList> forcedDownloads = forcedDownloadsBuilder.build(); - try (SilentCloseable c = Profiler.instance().profile("Remote.forcedDownload")) { - waitForBulkTransfer(forcedDownloads, /* cancelRemainingOnInterrupt= */ true); - } catch (Exception e) { - captureCorruptedOutputs(e); - deletePartialDownloadedOutputs(result, tmpOutErr, e); + + try { + waitForDownloads(forcedDownloads, result, tmpOutErr, "Remote.download"); + waitForDownloads(forcedDownloads, result, tmpOutErr, "Remote.forcedDownload"); + } catch (BulkTransferException | InterruptedException | ExecException e) { + // TODO(bazel-team): Consider adding better case-by-case exception handling instead of just rethrowing throw e; } @@ -1153,6 +1146,18 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } + private void waitForDownloads(ImmutableList> downloads, + RemoteActionResult result, FileOutErr tmpOutErr, String profileDescriptor) + throws BulkTransferException, InterruptedException, ExecException { + try (SilentCloseable c = Profiler.instance().profile(profileDescriptor)) { + waitForBulkTransfer(downloads, /* cancelRemainingOnInterrupt= */ true); + } catch (BulkTransferException | InterruptedException e) { + captureCorruptedOutputs(e); + deletePartialDownloadedOutputs(result, tmpOutErr, e); + throw e; + } + } + private ImmutableList> buildFilesToDownload( ActionResultMetadata metadata, RemoteAction action) { Predicate alwaysTrue = unused -> true; From 987d3874129eac366fe111f481b5d0a1593dd467 Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Tue, 5 Jul 2022 17:07:01 -0500 Subject: [PATCH 07/10] Address comments #2 Signed-off-by: Luis Fernando Pino Duque --- .../build/lib/remote/RemoteExecutionService.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index b9b1d694aa7818..71e7d2b976ef26 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1088,10 +1088,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re tmpOutErr.clearOut(); tmpOutErr.clearErr(); - if (!forcedDownloads.isEmpty()) { - moveOutputsToFinalLocation(forcedDownloads); - } - if (downloadOutputs) { moveOutputsToFinalLocation(downloads); @@ -1109,6 +1105,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // might not be supported on all platforms createSymlinks(symlinks); } else { + // TODO(bazel-team): We should unify this if-block to rely on downloadOutputs above but, as of 2022-07-05, + // downloadOuputs' semantics isn't exactly the same as build-without-the-bytes which is necessary for using + // remoteDownloadRegex. + if (!forcedDownloads.isEmpty()) { + moveOutputsToFinalLocation(forcedDownloads); + } + ActionInput inMemoryOutput = null; Digest inMemoryOutputDigest = null; PathFragment inMemoryOutputPath = getInMemoryOutputPath(action.getSpawn()); From fdf3b25296d336de609c33c35c7e7a587931c455 Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Thu, 7 Jul 2022 21:09:54 -0500 Subject: [PATCH 08/10] Debug Signed-off-by: Luis Fernando Pino Duque --- .../build/lib/remote/RemoteExecutionService.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 71e7d2b976ef26..aa16d6c5ed3eb5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1088,6 +1088,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re tmpOutErr.clearOut(); tmpOutErr.clearErr(); + // TODO(bazel-team): We should unify this if-block to rely on downloadOutputs above but, as of 2022-07-05, + // downloadOuputs' semantics isn't exactly the same as build-without-the-bytes which is necessary for using + // remoteDownloadRegex. + if (!forcedDownloads.isEmpty()) { + moveOutputsToFinalLocation(forcedDownloads); + } + if (downloadOutputs) { moveOutputsToFinalLocation(downloads); @@ -1105,13 +1112,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // might not be supported on all platforms createSymlinks(symlinks); } else { - // TODO(bazel-team): We should unify this if-block to rely on downloadOutputs above but, as of 2022-07-05, - // downloadOuputs' semantics isn't exactly the same as build-without-the-bytes which is necessary for using - // remoteDownloadRegex. - if (!forcedDownloads.isEmpty()) { - moveOutputsToFinalLocation(forcedDownloads); - } - ActionInput inMemoryOutput = null; Digest inMemoryOutputDigest = null; PathFragment inMemoryOutputPath = getInMemoryOutputPath(action.getSpawn()); From df6de240f818912e7bd7770f4a6fd7c3ff458c17 Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Fri, 8 Jul 2022 17:51:46 -0500 Subject: [PATCH 09/10] Address comments #3 Signed-off-by: Luis Fernando Pino Duque --- .../lib/remote/RemoteExecutionService.java | 56 +++++++++++-------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index aa16d6c5ed3eb5..75837b8d9a2b3c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1067,13 +1067,23 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } ImmutableList> downloads = downloadsBuilder.build(); - ImmutableList> forcedDownloads = forcedDownloadsBuilder.build(); + try (SilentCloseable c = Profiler.instance().profile("Remote.download")) { + waitForBulkTransfer(downloads, /* cancelRemainingOnInterrupt= */ true); + } catch (Exception e) { + // TODO(bazel-team): Consider adding better case-by-case exception handling instead of just rethrowing + captureCorruptedOutputs(e); + deletePartialDownloadedOutputs(result, tmpOutErr, e); + throw e; + } - try { - waitForDownloads(forcedDownloads, result, tmpOutErr, "Remote.download"); - waitForDownloads(forcedDownloads, result, tmpOutErr, "Remote.forcedDownload"); - } catch (BulkTransferException | InterruptedException | ExecException e) { + ImmutableList> forcedDownloads = forcedDownloadsBuilder.build(); + // TODO(bazel-team): Unify this block with the equivalent block above. + try (SilentCloseable c = Profiler.instance().profile("Remote.forcedDownload")) { + waitForBulkTransfer(forcedDownloads, /* cancelRemainingOnInterrupt= */ true); + } catch (Exception e) { // TODO(bazel-team): Consider adding better case-by-case exception handling instead of just rethrowing + captureCorruptedOutputs(e); + deletePartialDownloadedOutputs(result, tmpOutErr, e); throw e; } @@ -1088,13 +1098,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re tmpOutErr.clearOut(); tmpOutErr.clearErr(); - // TODO(bazel-team): We should unify this if-block to rely on downloadOutputs above but, as of 2022-07-05, - // downloadOuputs' semantics isn't exactly the same as build-without-the-bytes which is necessary for using - // remoteDownloadRegex. - if (!forcedDownloads.isEmpty()) { - moveOutputsToFinalLocation(forcedDownloads); - } - if (downloadOutputs) { moveOutputsToFinalLocation(downloads); @@ -1112,6 +1115,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // might not be supported on all platforms createSymlinks(symlinks); } else { + // TODO(bazel-team): We should unify this if-block to rely on downloadOutputs above but, as of 2022-07-05, + // downloadOuputs' semantics isn't exactly the same as build-without-the-bytes which is necessary for using + // remoteDownloadRegex. + if (!forcedDownloads.isEmpty()) { + moveOutputsToFinalLocation(forcedDownloads); + } + ActionInput inMemoryOutput = null; Digest inMemoryOutputDigest = null; PathFragment inMemoryOutputPath = getInMemoryOutputPath(action.getSpawn()); @@ -1149,17 +1159,17 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } - private void waitForDownloads(ImmutableList> downloads, - RemoteActionResult result, FileOutErr tmpOutErr, String profileDescriptor) - throws BulkTransferException, InterruptedException, ExecException { - try (SilentCloseable c = Profiler.instance().profile(profileDescriptor)) { - waitForBulkTransfer(downloads, /* cancelRemainingOnInterrupt= */ true); - } catch (BulkTransferException | InterruptedException e) { - captureCorruptedOutputs(e); - deletePartialDownloadedOutputs(result, tmpOutErr, e); - throw e; - } - } +// private void waitForDownloads(ImmutableList> downloads, +// RemoteActionResult result, FileOutErr tmpOutErr, String profileDescriptor) +// throws Exception { +// try (SilentCloseable c = Profiler.instance().profile(profileDescriptor)) { +// waitForBulkTransfer(downloads, /* cancelRemainingOnInterrupt= */ true); +// } catch (Exception e) { +// captureCorruptedOutputs(e); +// deletePartialDownloadedOutputs(result, tmpOutErr, e); +// throw e; +// } +// } private ImmutableList> buildFilesToDownload( ActionResultMetadata metadata, RemoteAction action) { From 73fd1921bc14c745511d04642a5d706dd2389b6f Mon Sep 17 00:00:00 2001 From: Luis Fernando Pino Duque Date: Tue, 12 Jul 2022 08:42:13 -0500 Subject: [PATCH 10/10] Address comments Signed-off-by: Luis Fernando Pino Duque --- .../build/lib/remote/RemoteExecutionService.java | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 75837b8d9a2b3c..346182c4df39cf 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1159,18 +1159,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } -// private void waitForDownloads(ImmutableList> downloads, -// RemoteActionResult result, FileOutErr tmpOutErr, String profileDescriptor) -// throws Exception { -// try (SilentCloseable c = Profiler.instance().profile(profileDescriptor)) { -// waitForBulkTransfer(downloads, /* cancelRemainingOnInterrupt= */ true); -// } catch (Exception e) { -// captureCorruptedOutputs(e); -// deletePartialDownloadedOutputs(result, tmpOutErr, e); -// throw e; -// } -// } - private ImmutableList> buildFilesToDownload( ActionResultMetadata metadata, RemoteAction action) { Predicate alwaysTrue = unused -> true;