From aa45f5f3301c5005294df084cdf99c45d6f95d53 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 25 Oct 2022 10:00:07 -0700 Subject: [PATCH] Move integration tests for BwoB to a base class and add more tests there. Remove the test parameters since it adds a lot of test time but for most tests we don't need to test these combinations. If we care a specific case (e.g. toplevel, remote cache, etc.), we should have a test case. PiperOrigin-RevId: 483705367 Change-Id: If4a166d2545bd7aea6fb63ec901a0ec889e88ca0 --- .../build/lib/actions/ActionCacheChecker.java | 8 +- .../lib/skyframe/SkyframeActionExecutor.java | 11 +- .../google/devtools/build/lib/remote/BUILD | 29 +- .../BuildWithoutTheBytesIntegrationTest.java | 356 ++--------- ...ildWithoutTheBytesIntegrationTestBase.java | 567 ++++++++++++++++++ 5 files changed, 641 insertions(+), 330 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 77ac27bf126010..6f4943026810ed 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -431,7 +431,7 @@ public Token getTokenIfNeedToExecute( MetadataHandler metadataHandler, ArtifactExpander artifactExpander, Map remoteDefaultPlatformProperties, - boolean isRemoteCacheEnabled) + boolean loadCachedOutputMetadata) throws InterruptedException { // TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that // produce only symlinks we should not check whether inputs are valid at all - all that matters @@ -476,7 +476,7 @@ public Token getTokenIfNeedToExecute( if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata() - && isRemoteCacheEnabled) { + && loadCachedOutputMetadata) { // load remote metadata from action cache cachedOutputMetadata = loadCachedOutputMetadata(action, entry, metadataHandler); } @@ -780,7 +780,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit( MetadataHandler metadataHandler, ArtifactExpander artifactExpander, Map remoteDefaultPlatformProperties, - boolean isRemoteCacheEnabled) + boolean loadCachedOutputMetadata) throws InterruptedException { if (action != null) { removeCacheEntry(action); @@ -793,7 +793,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit( metadataHandler, artifactExpander, remoteDefaultPlatformProperties, - isRemoteCacheEnabled); + loadCachedOutputMetadata); } /** Returns an action key. It is always set to the first output exec path string. */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 65777cec7c6458..6329c467285eb3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -603,15 +603,16 @@ Token checkActionCache( RemoteOptions remoteOptions; SortedMap remoteDefaultProperties; EventHandler handler; - boolean isRemoteCacheEnabled; + boolean loadCachedOutputMetadata; try (SilentCloseable c = profiler.profile(ProfilerTask.ACTION_CHECK, action.describe())) { remoteOptions = this.options.getOptions(RemoteOptions.class); remoteDefaultProperties = remoteOptions != null ? remoteOptions.getRemoteDefaultExecProperties() : ImmutableSortedMap.of(); - isRemoteCacheEnabled = - (remoteOptions != null && remoteOptions.isRemoteCacheEnabled()) || outputService != null; + loadCachedOutputMetadata = + outputService + != null; // Only load cached output metadata if remote output service is available handler = options.getOptions(BuildRequestOptions.class).explanationPath != null ? reporter : null; token = @@ -623,7 +624,7 @@ Token checkActionCache( metadataHandler, artifactExpander, remoteDefaultProperties, - isRemoteCacheEnabled); + loadCachedOutputMetadata); } catch (UserExecException e) { throw ActionExecutionException.fromExecException(e, action); } @@ -681,7 +682,7 @@ public T getContext(Class type) { metadataHandler, artifactExpander, remoteDefaultProperties, - isRemoteCacheEnabled); + loadCachedOutputMetadata); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 9d0411ebcc121b..830c481ea452da 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -37,7 +37,10 @@ java_test( size = "small", srcs = glob( ["**/*.java"], - exclude = NATIVE_SSL_TEST + ["BuildWithoutTheBytesIntegrationTest.java"], + exclude = NATIVE_SSL_TEST + [ + "BuildWithoutTheBytesIntegrationTest.java", + "BuildWithoutTheBytesIntegrationTestBase.java", + ], ) + NATIVE_SSL_TEST_MAYBE, test_class = "com.google.devtools.build.lib.AllTests", deps = [ @@ -119,6 +122,24 @@ java_test( ], ) +java_library( + name = "build_without_the_bytes_integration_test_base", + srcs = [ + "BuildWithoutTheBytesIntegrationTestBase.java", + ], + deps = [ + "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/util:os", + "//src/main/java/com/google/devtools/build/lib/util/io", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/test/java/com/google/devtools/build/lib/buildtool/util", + "//third_party:guava", + "//third_party:junit4", + "//third_party:truth", + ], +) + java_test( name = "BuildWithoutTheBytesIntegrationTest", size = "large", @@ -128,14 +149,14 @@ java_test( "//third_party/grpc-java:grpc-jar", ], deps = [ + ":build_without_the_bytes_integration_test_base", "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/dynamic", "//src/main/java/com/google/devtools/build/lib/remote", - "//src/main/java/com/google/devtools/build/lib/sandbox", + "//src/main/java/com/google/devtools/build/lib/standalone", "//src/main/java/com/google/devtools/build/lib/util:os", - "//src/main/java/com/google/devtools/build/lib/vfs", - "//src/test/java/com/google/devtools/build/lib/buildtool/util", "//src/test/java/com/google/devtools/build/lib/remote/util:integration_test_utils", "//third_party:guava", "//third_party:junit4", diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index e06b866f7b549d..2db4ea11d070a3 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java @@ -13,76 +13,57 @@ // limitations under the License. package com.google.devtools.build.lib.remote; -import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth.assertWithMessage; import static com.google.devtools.build.lib.remote.util.IntegrationTestUtils.startWorker; -import static com.google.devtools.build.lib.vfs.FileSystemUtils.readContent; -import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; -import com.google.common.eventbus.Subscribe; -import com.google.devtools.build.lib.actions.ActionExecutedEvent; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.BuildFailedException; -import com.google.devtools.build.lib.actions.CachedActionEvent; -import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase; +import com.google.devtools.build.lib.dynamic.DynamicExecutionModule; import com.google.devtools.build.lib.remote.util.IntegrationTestUtils.WorkerInstance; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.BlockWaitingModule; import com.google.devtools.build.lib.runtime.BuildSummaryStatsModule; -import com.google.devtools.build.lib.sandbox.SandboxModule; +import com.google.devtools.build.lib.standalone.StandaloneModule; import com.google.devtools.build.lib.util.OS; import java.io.IOException; -import java.util.ArrayList; -import java.util.List; import org.junit.After; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; +import org.junit.runners.JUnit4; /** Integration tests for Build without the Bytes. */ -@RunWith(Parameterized.class) -public class BuildWithoutTheBytesIntegrationTest extends BuildIntegrationTestCase { - public enum RemoteMode { - REMOTE_EXECUTION, - REMOTE_CACHE; - - public boolean executeRemotely() { - return this == REMOTE_EXECUTION; - } - } +@RunWith(JUnit4.class) +public class BuildWithoutTheBytesIntegrationTest extends BuildWithoutTheBytesIntegrationTestBase { + private WorkerInstance worker; - public enum OutputMode { - DOWNLOAD_TOPLEVEL, - DOWNLOAD_MINIMAL; + @Override + protected void setupOptions() throws Exception { + super.setupOptions(); - public boolean minimal() { - return this == DOWNLOAD_MINIMAL; + if (worker == null) { + worker = startWorker(); } + + addOptions( + "--remote_executor=grpc://localhost:" + worker.getPort(), + "--remote_download_minimal", + "--experimental_action_cache_store_output_metadata", + "--dynamic_local_strategy=standalone", + "--dynamic_remote_strategy=remote"); } - private WorkerInstance worker; - private final RemoteMode remoteMode; - private final OutputMode outputMode; - - @Parameterized.Parameters(name = "{0}-{1}") - public static List configs() { - ArrayList params = new ArrayList<>(); - for (RemoteMode remoteMode : RemoteMode.values()) { - for (OutputMode outputMode : OutputMode.values()) { - params.add(new Object[] {remoteMode, outputMode}); - } - } - return params; + @Override + protected void setDownloadToplevel() { + addOptions("--remote_download_outputs=toplevel"); } - public BuildWithoutTheBytesIntegrationTest(RemoteMode remoteMode, OutputMode outputMode) { - this.remoteMode = remoteMode; - this.outputMode = outputMode; + @Override + protected void setDownloadAll() { + addOptions("--remote_download_outputs=all"); } @Override @@ -96,104 +77,32 @@ protected BlazeRuntime.Builder getRuntimeBuilder() throws Exception { protected ImmutableList getSpawnModules() { return ImmutableList.builder() .addAll(super.getSpawnModules()) - .add(new SandboxModule()) + .add(new StandaloneModule()) .add(new RemoteModule()) + .add(new DynamicExecutionModule()) .build(); } - @After - public void tearDown() throws IOException { - if (worker != null) { - worker.stop(); - } - } - - private void addRemoteModeOptions() throws IOException, InterruptedException { - addRemoteModeOptions(remoteMode); - } - - private void addRemoteModeOptions(RemoteMode remoteMode) - throws IOException, InterruptedException { - if (worker == null) { - worker = startWorker(); - } - - switch (remoteMode) { - case REMOTE_EXECUTION: - addOptions("--remote_executor=grpc://localhost:" + worker.getPort()); - break; - case REMOTE_CACHE: - addOptions("--remote_cache=grpc://localhost:" + worker.getPort()); - break; - } - } - - private void addOutputModeOptions() { - switch (outputMode) { - case DOWNLOAD_TOPLEVEL: - addOptions("--remote_download_toplevel"); - break; - case DOWNLOAD_MINIMAL: - addOptions("--remote_download_minimal"); - break; - } + @Override + protected void assertOutputEquals(String realContent, String expectedContent) throws Exception { + assertThat(realContent).isEqualTo(expectedContent); } - private String getSpawnStrategy() { - if (remoteMode.executeRemotely()) { - return "remote"; - } else { - OS currentOS = OS.getCurrent(); - switch (currentOS) { - case LINUX: - return "linux-sandbox"; - case DARWIN: - return "processwrapper-sandbox"; - default: - return "local"; - } - } + @Override + protected void assertOutputContains(String content, String contains) throws Exception { + assertThat(content).contains(contains); } - @Test - public void unnecessaryOutputsAreNotDownloaded() throws Exception { - write( - "a/BUILD", - "genrule(", - " name = 'foo',", - " srcs = [],", - " outs = ['foo.txt'],", - " cmd = 'echo foo > $@',", - ")", - "", - "genrule(", - " name = 'foobar',", - " srcs = [':foo'],", - " outs = ['foobar.txt'],", - " cmd = 'cat $(location :foo) > $@ && echo bar > $@',", - ")"); - if (!remoteMode.executeRemotely()) { - warmUpRemoteCache("//a:foobar"); - } - addRemoteModeOptions(); - addOutputModeOptions(); - - buildTarget("//a:foobar"); - - assertOutputsDoNotExist("//a:foo"); - if (outputMode.minimal()) { - assertOutputsDoNotExist("//a:foobar"); + @After + public void tearDown() throws IOException { + if (worker != null) { + worker.stop(); } } @Test public void executeRemotely_actionFails_outputsAreAvailableLocallyForDebuggingPurpose() throws Exception { - if (!remoteMode.executeRemotely()) { - return; - } - addRemoteModeOptions(); - addOutputModeOptions(); write( "a/BUILD", "genrule(", @@ -208,43 +117,6 @@ public void executeRemotely_actionFails_outputsAreAvailableLocallyForDebuggingPu assertOnlyOutputContent("//a:fail", "fail.txt", "foo\n"); } - @Test - public void intermediateOutputsAreInputForLocalActions_prefetchIntermediateOutputs() - throws Exception { - // Test that a remote-only output that's an input to a local action is downloaded lazily before - // executing the local action. - write( - "a/BUILD", - "genrule(", - " name = 'remote',", - " srcs = [],", - " outs = ['remote.txt'],", - " cmd = 'echo -n remote > $@',", - ")", - "", - "genrule(", - " name = 'local',", - " srcs = [':remote'],", - " outs = ['local.txt'],", - " cmd = 'cat $(location :remote) > $@ && echo -n local >> $@',", - " tags = ['no-remote'],", - ")"); - if (!remoteMode.executeRemotely()) { - warmUpRemoteCache("//a:local"); - } - addRemoteModeOptions(); - addOutputModeOptions(); - - if (outputMode.minimal()) { - buildTarget("//a:remote"); - assertOutputsDoNotExist("//a:remote"); - } - buildTarget("//a:local"); - - assertOnlyOutputContent("//a:remote", "remote.txt", "remote"); - assertOnlyOutputContent("//a:local", "local.txt", "remotelocal"); - } - @Test public void intermediateOutputsAreInputForInternalActions_prefetchIntermediateOutputs() throws Exception { @@ -291,21 +163,12 @@ public void intermediateOutputsAreInputForInternalActions_prefetchIntermediateOu " username = 'buchgr',", " template = ':generate-template',", ")"); - if (!remoteMode.executeRemotely()) { - warmUpRemoteCache("//a:substitute-buchgr"); - } - addRemoteModeOptions(); - addOutputModeOptions(); buildTarget("//a:substitute-buchgr"); // The genrule //a:generate-template should run remotely and //a:substitute-buchgr should be a // internal action running locally. - if (remoteMode.executeRemotely()) { - events.assertContainsInfo("3 processes: 2 internal, 1 remote"); - } else { - events.assertContainsInfo("3 processes: 1 remote cache hit, 2 internal"); - } + events.assertContainsInfo("3 processes: 2 internal, 1 remote"); Artifact intermediateOutput = getOnlyElement(getArtifacts("//a:generate-template")); assertThat(intermediateOutput.getPath().exists()).isTrue(); assertOnlyOutputContent("//a:substitute-buchgr", "substitute-buchgr.txt", "Hello buchgr!"); @@ -313,8 +176,6 @@ public void intermediateOutputsAreInputForInternalActions_prefetchIntermediateOu @Test public void changeOutputMode_invalidateActions() throws Exception { - addRemoteModeOptions(); - addOutputModeOptions(); write( "a/BUILD", "genrule(", @@ -336,88 +197,19 @@ public void changeOutputMode_invalidateActions() throws Exception { // 3 = workspace status action + //:foo + //:foobar assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(3); actionEventCollector.clear(); - events.assertContainsInfo("3 processes: 1 internal, 2 " + getSpawnStrategy()); events.clear(); - addOptions("--remote_download_outputs=all"); + setDownloadAll(); buildTarget("//a:foobar"); // Changing output mode should invalidate SkyFrame's in-memory caching and make it re-evaluate // the action nodes. assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(3); - if (remoteMode.executeRemotely()) { - // The dummy workspace status action does not re-executed, so wouldn't be displayed here - if (outputMode.minimal()) { - events.assertContainsInfo("2 processes: 2 remote cache hit"); - } else { - // output of toplevel target are on the local disk, so the action can hit the action cache, - // hence not shown here - events.assertContainsInfo("1 process: 1 remote cache hit"); - } - } else { - // all actions can hit action cache since all outputs are on the local disk due to they were - // executed locally. - events.assertContainsInfo("0 processes"); - } - } - - @Test - public void symlinkToSourceFile() throws Exception { - addRemoteModeOptions(); - addOutputModeOptions(); - - write( - "a/defs.bzl", - "def _impl(ctx):", - " if ctx.attr.chain_length < 1:", - " fail('chain_length must be > 0')", - "", - " file = ctx.file.target", - "", - " for i in range(ctx.attr.chain_length):", - " sym = ctx.actions.declare_file(ctx.label.name + '.sym' + str(i))", - " ctx.actions.symlink(output = sym, target_file = file)", - " file = sym", - "", - " out = ctx.actions.declare_file(ctx.label.name + '.out')", - " ctx.actions.run_shell(", - " inputs = [sym],", - " outputs = [out],", - " command = '[[ hello == $(cat $1) ]] && touch $2',", - " arguments = [sym.path, out.path],", - " execution_requirements = {'no-remote': ''} if ctx.attr.local else {},", - " )", - "", - " return DefaultInfo(files = depset([out]))", - "", - "my_rule = rule(", - " implementation = _impl,", - " attrs = {", - " 'target': attr.label(allow_single_file = True),", - " 'chain_length': attr.int(),", - " 'local': attr.bool(),", - " },", - ")"); - - write( - "a/BUILD", - "load(':defs.bzl', 'my_rule')", - "", - "my_rule(name = 'one_local', target = 'src.txt', local = True, chain_length = 1)", - "my_rule(name = 'two_local', target = 'src.txt', local = True, chain_length = 2)", - "my_rule(name = 'one_remote', target = 'src.txt', local = False, chain_length = 1)", - "my_rule(name = 'two_remote', target = 'src.txt', local = False, chain_length = 2)"); - - write("a/src.txt", "hello"); - - buildTarget("//a:one_local", "//a:two_local", "//a:one_remote", "//a:two_remote"); + events.assertContainsInfo("2 processes: 2 remote cache hit"); } @Test public void symlinkToGeneratedFile() throws Exception { - addRemoteModeOptions(); - addOutputModeOptions(); - write( "a/defs.bzl", "def _impl(ctx):", @@ -470,9 +262,6 @@ public void symlinkToGeneratedFile() throws Exception { @Test public void symlinkToDirectory() throws Exception { - addRemoteModeOptions(); - addOutputModeOptions(); - write( "a/defs.bzl", "def _impl(ctx):", @@ -524,9 +313,6 @@ public void symlinkToDirectory() throws Exception { @Test public void symlinkToNestedFile() throws Exception { - addRemoteModeOptions(); - addOutputModeOptions(); - write( "a/defs.bzl", "def _impl(ctx):", @@ -579,9 +365,6 @@ public void symlinkToNestedFile() throws Exception { @Test public void symlinkToNestedDirectory() throws Exception { - addRemoteModeOptions(); - addOutputModeOptions(); - write( "a/defs.bzl", "def _impl(ctx):", @@ -631,65 +414,4 @@ public void symlinkToNestedDirectory() throws Exception { buildTarget("//a:one_local", "//a:two_local", "//a:one_remote", "//a:two_remote"); } - - private static class ActionEventCollector { - private final List actionExecutedEvents = new ArrayList<>(); - private final List cachedActionEvents = new ArrayList<>(); - - @Subscribe - public void onActionExecuted(ActionExecutedEvent event) { - actionExecutedEvents.add(event); - } - - @Subscribe - public void onCachedAction(CachedActionEvent event) { - cachedActionEvents.add(event); - } - - public int getNumActionNodesEvaluated() { - return getActionExecutedEvents().size() + getCachedActionEvents().size(); - } - - public void clear() { - this.actionExecutedEvents.clear(); - this.cachedActionEvents.clear(); - } - - public List getActionExecutedEvents() { - return actionExecutedEvents; - } - - public List getCachedActionEvents() { - return cachedActionEvents; - } - } - - private void assertOutputsDoNotExist(String target) throws Exception { - for (Artifact output : getArtifacts(target)) { - assertWithMessage( - "output %s for target %s should not exist", output.getExecPathString(), target) - .that(output.getPath().exists()) - .isFalse(); - } - } - - private void assertOnlyOutputContent(String target, String filename, String content) - throws Exception { - Artifact output = getOnlyElement(getArtifacts(target)); - assertThat(output.getFilename()).isEqualTo(filename); - assertThat(output.getPath().exists()).isTrue(); - assertThat(readContent(output.getPath(), UTF_8)).isEqualTo(content); - } - - private void warmUpRemoteCache(String target) throws Exception { - checkState(worker == null, "Call 'warmUpRemoteCache' before 'addRemoteModeOptions'"); - addRemoteModeOptions(RemoteMode.REMOTE_EXECUTION); - ImmutableList originalOptions = getRuntimeWrapper().getOptions(); - - buildTarget(target); - - getDirectories().getOutputPath(getWorkspace().getBaseName()).deleteTreesBelow(); - resetOptions(); - addOptions(originalOptions); - } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java new file mode 100644 index 00000000000000..cf1dc3ffac39e6 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java @@ -0,0 +1,567 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.remote; + +import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.devtools.build.lib.vfs.FileSystemUtils.readContent; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertThrows; + +import com.google.common.eventbus.Subscribe; +import com.google.devtools.build.lib.actions.ActionExecutedEvent; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.BuildFailedException; +import com.google.devtools.build.lib.actions.CachedActionEvent; +import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase; +import com.google.devtools.build.lib.util.OS; +import com.google.devtools.build.lib.util.io.RecordingOutErr; +import com.google.devtools.build.lib.vfs.Path; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.junit.Test; + +/** Base class for integration tests for BwoB. */ +public abstract class BuildWithoutTheBytesIntegrationTestBase extends BuildIntegrationTestCase { + // Concrete implementations should by default set the necessary flags to download minimal outputs. + // These methods should override the necessary flags to download top-level outputs or all outputs. + protected abstract void setDownloadToplevel(); + + protected abstract void setDownloadAll(); + + protected abstract void assertOutputEquals(String realContent, String expectedContent) + throws Exception; + + protected abstract void assertOutputContains(String content, String contains) throws Exception; + + protected void waitDownloads() throws Exception { + // Trigger afterCommand of modules so that downloads are waited. + runtimeWrapper.newCommand(); + } + + @Test + public void outputsAreNotDownloaded() throws Exception { + write( + "BUILD", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['out/foo.txt'],", + " cmd = 'echo foo > $@',", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',", + ")"); + + buildTarget("//:foobar"); + waitDownloads(); + + assertOutputsDoNotExist("//:foo"); + assertOutputsDoNotExist("//:foobar"); + } + + @Test + public void downloadOutputsWithRegex() throws Exception { + write( + "BUILD", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['out/foo.txt'],", + " cmd = 'echo foo > $@',", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',", + ")"); + addOptions("--experimental_remote_download_regex=.*foo\\.txt$"); + + buildTarget("//:foobar"); + waitDownloads(); + + assertValidOutputFile("out/foo.txt", "foo\n"); + assertOutputsDoNotExist("//:foobar"); + } + + @Test + public void intermediateOutputsAreInputForLocalActions_prefetchIntermediateOutputs() + throws Exception { + // Test that a remote-only output that's an input to a local action is downloaded lazily before + // executing the local action. + write( + "a/BUILD", + "genrule(", + " name = 'remote',", + " srcs = [],", + " outs = ['remote.txt'],", + " cmd = 'echo -n remote > $@',", + ")", + "", + "genrule(", + " name = 'local',", + " srcs = [':remote'],", + " outs = ['local.txt'],", + " cmd = 'cat $(location :remote) > $@ && echo -n local >> $@',", + " tags = ['no-remote'],", + ")"); + + buildTarget("//a:remote"); + waitDownloads(); + assertOutputsDoNotExist("//a:remote"); + buildTarget("//a:local"); + waitDownloads(); + + assertOnlyOutputContent("//a:remote", "remote.txt", "remote"); + assertOnlyOutputContent("//a:local", "local.txt", "remotelocal"); + } + + @Test + public void symlinkToSourceFile() throws Exception { + write( + "a/defs.bzl", + "def _impl(ctx):", + " if ctx.attr.chain_length < 1:", + " fail('chain_length must be > 0')", + "", + " file = ctx.file.target", + "", + " for i in range(ctx.attr.chain_length):", + " sym = ctx.actions.declare_file(ctx.label.name + '.sym' + str(i))", + " ctx.actions.symlink(output = sym, target_file = file)", + " file = sym", + "", + " out = ctx.actions.declare_file(ctx.label.name + '.out')", + " ctx.actions.run_shell(", + " inputs = [sym],", + " outputs = [out],", + " command = '[[ hello == $(cat $1) ]] && touch $2',", + " arguments = [sym.path, out.path],", + " execution_requirements = {'no-remote': ''} if ctx.attr.local else {},", + " )", + "", + " return DefaultInfo(files = depset([out]))", + "", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'target': attr.label(allow_single_file = True),", + " 'chain_length': attr.int(),", + " 'local': attr.bool(),", + " },", + ")"); + + write( + "a/BUILD", + "load(':defs.bzl', 'my_rule')", + "", + "my_rule(name = 'one_local', target = 'src.txt', local = True, chain_length = 1)", + "my_rule(name = 'two_local', target = 'src.txt', local = True, chain_length = 2)", + "my_rule(name = 'one_remote', target = 'src.txt', local = False, chain_length = 1)", + "my_rule(name = 'two_remote', target = 'src.txt', local = False, chain_length = 2)"); + + write("a/src.txt", "hello"); + + buildTarget("//a:one_local", "//a:two_local", "//a:one_remote", "//a:two_remote"); + } + + @Test + public void dynamicExecution_stdoutIsReported() throws Exception { + // Disable on Windows since it fails for unknown reasons. + // TODO(chiwang): Enable it on windows. + if (OS.getCurrent() == OS.WINDOWS) { + return; + } + + addOptions("--internal_spawn_scheduler"); + addOptions("--strategy=Genrule=dynamic"); + addOptions("--experimental_local_execution_delay=9999999"); + write( + "BUILD", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['out/foo.txt'],", + " cmd = 'echo my-output-message > $@',", + " tags = ['no-local'],", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo) && touch $@',", + ")"); + RecordingOutErr outErr = new RecordingOutErr(); + this.outErr = outErr; + + buildTarget("//:foobar"); + waitDownloads(); + + assertOutputContains(outErr.outAsLatin1(), "my-output-message"); + } + + @Test + public void dynamicExecution_stderrIsReported() throws Exception { + // Disable on Windows since it fails for unknown reasons. + // TODO(chiwang): Enable it on windows. + if (OS.getCurrent() == OS.WINDOWS) { + return; + } + addOptions("--internal_spawn_scheduler"); + addOptions("--strategy=Genrule=dynamic"); + addOptions("--experimental_local_execution_delay=9999999"); + write( + "BUILD", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['out/foo.txt'],", + " cmd = 'echo my-error-message > $@',", + " tags = ['no-local'],", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo) >&2 && exit 1',", + ")"); + RecordingOutErr outErr = new RecordingOutErr(); + this.outErr = outErr; + + assertThrows(BuildFailedException.class, () -> buildTarget("//:foobar")); + + assertOutputContains(outErr.errAsLatin1(), "my-error-message"); + } + + @Test + public void downloadToplevel_outputsFromImportantOutputGroupAreDownloaded() throws Exception { + setDownloadToplevel(); + write( + "rules.bzl", + "def _gen_impl(ctx):", + " output = ctx.actions.declare_file(ctx.attr.name)", + " ctx.actions.run_shell(", + " outputs = [output],", + " arguments = [ctx.attr.content, output.path],", + " command = 'echo $1 > $2',", + " )", + " extra1 = ctx.actions.declare_file(ctx.attr.name + '1')", + " ctx.actions.run_shell(", + " outputs = [extra1],", + " arguments = [ctx.attr.content, extra1.path],", + " command = 'echo $1 > $2',", + " )", + " extra2 = ctx.actions.declare_file(ctx.attr.name + '2')", + " ctx.actions.run_shell(", + " outputs = [extra2],", + " arguments = [ctx.attr.content, extra2.path],", + " command = 'echo $1 > $2',", + " )", + " return [", + " DefaultInfo(files = depset([output])),", + " OutputGroupInfo(", + " extra1_files = depset([extra1]),", + " extra2_files = depset([extra2]),", + " ),", + " ]", + "", + "gen = rule(", + " implementation = _gen_impl,", + " attrs = {", + " 'content': attr.string(mandatory = True),", + " }", + ")"); + write( + "BUILD", + "load(':rules.bzl', 'gen')", + "gen(", + " name = 'foo',", + " content = 'foo-content',", + ")"); + addOptions("--output_groups=+extra1_files"); + + buildTarget("//:foo"); + waitDownloads(); + + assertValidOutputFile("foo", "foo-content\n"); + assertValidOutputFile("foo1", "foo-content\n"); + assertOutputDoesNotExist("foo2"); + } + + @Test + public void downloadToplevel_outputsFromHiddenOutputGroupAreNotDownloaded() throws Exception { + setDownloadToplevel(); + write( + "rules.bzl", + "def _gen_impl(ctx):", + " output = ctx.actions.declare_file(ctx.attr.name)", + " ctx.actions.run_shell(", + " outputs = [output],", + " arguments = [ctx.attr.content, output.path],", + " command = 'echo $1 > $2',", + " )", + " validation_file = ctx.actions.declare_file(ctx.attr.name + '.validation')", + " ctx.actions.run_shell(", + " outputs = [validation_file],", + " arguments = [ctx.attr.content, validation_file.path],", + " command = 'echo $1 > $2',", + " )", + " return [", + " DefaultInfo(files = depset([output])),", + " OutputGroupInfo(", + " _validation = depset([validation_file]),", + " ),", + " ]", + "", + "gen = rule(", + " implementation = _gen_impl,", + " attrs = {", + " 'content': attr.string(mandatory = True),", + " }", + ")"); + write( + "BUILD", + "load(':rules.bzl', 'gen')", + "gen(", + " name = 'foo',", + " content = 'foo-content',", + ")"); + addOptions("--output_groups=+_validation"); + + buildTarget("//:foo"); + waitDownloads(); + + assertValidOutputFile("foo", "foo-content\n"); + assertOutputDoesNotExist("foo.validation"); + } + + @Test + public void downloadToplevel_treeArtifacts() throws Exception { + // Disable on Windows since it fails for unknown reasons. + // TODO(chiwang): Enable it on windows. + if (OS.getCurrent() == OS.WINDOWS) { + return; + } + setDownloadToplevel(); + writeOutputDirRule(); + write( + "BUILD", + "load(':output_dir.bzl', 'output_dir')", + "output_dir(", + " name = 'foo',", + " manifest = ':manifest',", + ")"); + write("manifest", "file-1", "file-2", "file-3"); + + 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 incrementalBuild_deleteOutputsInUnwritableParentDirectory() throws Exception { + write( + "BUILD", + "genrule(", + " name = 'unwritable',", + " srcs = ['file.in'],", + " outs = ['unwritable/somefile.out'],", + " cmd = 'cat $(SRCS) > $@; chmod a-w $$(dirname $@)',", + " local = True,", + ")"); + write("file.in", "content"); + buildTarget("//:unwritable"); + + write("file.in", "updated content"); + + buildTarget("//:unwritable"); + } + + @Test + public void incrementalBuild_treeArtifacts_correctlyProducesNewTree() 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"); + setDownloadToplevel(); + buildTarget("//:foo"); + waitDownloads(); + + write("manifest", "file-1", "file-4"); + restartServer(); + setDownloadToplevel(); + buildTarget("//:foo"); + waitDownloads(); + + assertValidOutputFile("foo/file-1", "file-1\n"); + assertValidOutputFile("foo/file-4", "file-4\n"); + assertOutputDoesNotExist("foo/file-2"); + assertOutputDoesNotExist("foo/file-3"); + } + + @Test + public void incrementalBuild_restartServer_hitActionCache() throws Exception { + // Prepare workspace + write( + "BUILD", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['out/foo.txt'],", + " cmd = 'echo foo > $@',", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',", + ")"); + ActionEventCollector actionEventCollector = new ActionEventCollector(); + getRuntimeWrapper().registerSubscriber(actionEventCollector); + + // Clean build + buildTarget("//:foobar"); + + // all action should be executed + assertThat(actionEventCollector.getActionExecutedEvents()).hasSize(3); + // no outputs are staged + assertOutputsDoNotExist("//:foobar"); + + restartServer(); + actionEventCollector = new ActionEventCollector(); + getRuntimeWrapper().registerSubscriber(actionEventCollector); + + // Incremental build + buildTarget("//:foobar"); + + // all actions should hit the action cache. + assertThat(actionEventCollector.getActionExecutedEvents()).isEmpty(); + // no outputs are staged + assertOutputsDoNotExist("//:foobar"); + } + + protected void assertOutputsDoNotExist(String target) throws Exception { + for (Artifact output : getArtifacts(target)) { + assertWithMessage( + "output %s for target %s should not exist", output.getExecPathString(), target) + .that(output.getPath().exists()) + .isFalse(); + } + } + + protected Path getOutputPath(String binRelativePath) { + return getDirectories() + .getWorkspace() + .getRelative(getDirectories().getProductName() + "-bin") + .getRelative(binRelativePath); + } + + protected void assertOutputDoesNotExist(String binRelativePath) { + Path output = getOutputPath(binRelativePath); + assertThat(output.exists()).isFalse(); + } + + protected void assertOnlyOutputContent(String target, String filename, String content) + throws Exception { + Artifact output = getOnlyElement(getArtifacts(target)); + assertThat(output.getFilename()).isEqualTo(filename); + assertThat(output.getPath().exists()).isTrue(); + assertOutputEquals(readContent(output.getPath(), UTF_8), content); + } + + protected void assertValidOutputFile(String binRelativePath, String content) throws Exception { + Path output = getOutputPath(binRelativePath); + assertOutputEquals(readContent(output, UTF_8), content); + assertThat(output.isReadable()).isTrue(); + assertThat(output.isWritable()).isFalse(); + assertThat(output.isExecutable()).isTrue(); + } + + protected void writeOutputDirRule() throws IOException { + write( + "output_dir.bzl", + "def _output_dir_impl(ctx):", + " output_dir = ctx.actions.declare_directory(ctx.attr.name)", + " ctx.actions.run_shell(", + " inputs = [ctx.file.manifest],", + " outputs = [output_dir],", + " arguments = [ctx.file.manifest.path, output_dir.path],", + " command = 'while read -r line; do echo $line > $2/$line; done < $1',", + " )", + " return [DefaultInfo(files = depset([output_dir]))]", + "", + "output_dir = rule(", + " implementation = _output_dir_impl,", + " attrs = {", + " 'manifest': attr.label(mandatory = True, allow_single_file = True),", + " }", + ")"); + } + + protected static class ActionEventCollector { + private final List actionExecutedEvents = new ArrayList<>(); + private final List cachedActionEvents = new ArrayList<>(); + + @Subscribe + public void onActionExecuted(ActionExecutedEvent event) { + actionExecutedEvents.add(event); + } + + @Subscribe + public void onCachedAction(CachedActionEvent event) { + cachedActionEvents.add(event); + } + + public int getNumActionNodesEvaluated() { + return getActionExecutedEvents().size() + getCachedActionEvents().size(); + } + + public void clear() { + this.actionExecutedEvents.clear(); + this.cachedActionEvents.clear(); + } + + public List getActionExecutedEvents() { + return actionExecutedEvents; + } + + public List getCachedActionEvents() { + return cachedActionEvents; + } + } + + protected void restartServer() throws Exception { + // Simulates a server restart + createRuntimeWrapper(); + } +}