From 5091e1da56db5fff0d17214717a3604b48dd6a74 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 19 Jan 2025 16:32:35 +0100 Subject: [PATCH 1/2] Forward `exec_properties` to main test spawn's execution info This makes `exec_properties` such as `no-remote-exec` effective for the main test spawn, where it previously only affected post-processing spawns such as test XML generation. --- .../lib/exec/StandaloneTestStrategy.java | 3 +- .../lib/analysis/StarlarkExecGroupTest.java | 161 ++++++++++++++++++ src/test/shell/bazel/remote/BUILD | 1 + .../bazel/remote/remote_execution_test.sh | 63 ++++--- 4 files changed, 206 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index f412cbee769e91..d05eb857e6ecfb 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -112,8 +112,7 @@ public TestRunnerSpawn createTestRunnerSpawn( createEnvironment( actionExecutionContext, action, tmpDirRoot, executionOptions.splitXmlGeneration); - Map executionInfo = - new TreeMap<>(action.getTestProperties().getExecutionInfo()); + Map executionInfo = new TreeMap<>(action.getExecutionInfo()); if (!action.shouldAcceptCachedResult()) { // TODO(tjgq): We want to reject a previously cached result, but not prevent the result of the // current execution from being uploaded. We should introduce a separate execution requirement diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java index a6e1cf216813e9..6004ca96941c4c 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java @@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME; import static com.google.devtools.build.lib.skyframe.BzlLoadValue.keyForBuild; +import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; @@ -707,4 +708,164 @@ public void ruleOverridePlatformExecGroupExecProperty() throws Exception { assertThat(getGeneratingAction(target, "test/out.txt").getOwner().getExecProperties()) .containsExactly("ripeness", "unknown"); } + + @Test + public void testRuleExecGroup() throws Exception { + scratch.file( + "rule/my_toolchain.bzl", + """ + def _impl(ctx): + return [platform_common.ToolchainInfo(label = ctx.label)] + + my_toolchain = rule( + implementation = _impl, + ) + """); + scratch.file( + "rule/BUILD", + """ + toolchain_type(name = "toolchain_type") + """); + scratch.file( + "toolchain/BUILD", + """ + load("//rule:my_toolchain.bzl", "my_toolchain") + + my_toolchain( + name = "target_target", + ) + + toolchain( + name = "target_target_toolchain", + exec_compatible_with = ["@platforms//os:linux"], + target_compatible_with = ["@platforms//os:linux"], + toolchain = ":target_target", + toolchain_type = "//rule:toolchain_type", + ) + + my_toolchain( + name = "exec_target", + ) + + toolchain( + name = "exec_target_toolchain", + exec_compatible_with = ["@platforms//os:macos"], + target_compatible_with = ["@platforms//os:linux"], + toolchain = ":exec_target", + toolchain_type = "//rule:toolchain_type", + ) + """); + + scratch.overwriteFile( + "platform/BUILD", + """ + constraint_setting( + name = "fast_cpu", + default_constraint_value = ":no_fast_cpu", + ) + + constraint_value( + name = "no_fast_cpu", + constraint_setting = ":fast_cpu", + ) + + constraint_value( + name = "has_fast_cpu", + constraint_setting = ":fast_cpu", + ) + + platform( + name = "target_platform", + constraint_values = [ + "@platforms//os:linux", + ], + ) + + platform( + name = "fast_cpu_platform", + constraint_values = [ + "@platforms//os:macos", + ":has_fast_cpu", + ], + exec_properties = { + "require_fast_cpu": "true", + }, + ) + """); + + scratch.file( + "test/defs.bzl", + """ + MyInfo = provider(fields = ["toolchain_label"]) + + def _impl(ctx): + executable = ctx.actions.declare_file(ctx.label.name) + ctx.actions.run_shell( + outputs = [executable], + command = "touch $1", + arguments = [executable.path], + ) + return [ + DefaultInfo( + executable = executable, + ), + MyInfo( + toolchain_label = ctx.toolchains["//rule:toolchain_type"].label, + ), + ] + + my_cc_test = rule( + implementation = _impl, + test = True, + toolchains = ["//rule:toolchain_type"], + ) + """); + + scratch.file( + "test/BUILD", + """ + load("//test:defs.bzl", "my_cc_test") + + my_cc_test( + name = "my_test", + exec_compatible_with = [ + "//platform:has_fast_cpu", + ], + exec_properties = { + "test.require_gpu": "true", + }, + ) + """); + + useConfiguration( + "--extra_toolchains=//toolchain:target_target_toolchain,//toolchain:exec_target_toolchain", + "--platforms=//platform:target_platform", + "--extra_execution_platforms=//platform:target_platform,//platform:fast_cpu_platform"); + + ConfiguredTarget target = getConfiguredTarget("//test:my_test"); + + Provider.Key key = + new StarlarkProvider.Key(keyForBuild(Label.parseCanonical("//test:defs.bzl")), "MyInfo"); + Label toolchainLabel = (Label) ((StructImpl) target.get(key)).getValue("toolchain_label"); + assertThat(toolchainLabel).isEqualTo(Label.parseCanonicalUnchecked("//toolchain:exec_target")); + + Action compileAction = getGeneratingAction(target, "test/my_test"); + assertThat(compileAction.getExecutionPlatform().label()) + .isEqualTo(Label.parseCanonicalUnchecked("//platform:fast_cpu_platform")); + assertThat(compileAction.getExecProperties()).containsExactly("require_fast_cpu", "true"); + + Action testAction = + getActions("//test:my_test").stream() + .filter(action -> action.getMnemonic().equals("TestRunner")) + .findFirst() + .orElseThrow(); + // This is NOT the desired behavior: the test action should not be affected by + // the exec_compatible_with constraint and thus select the target platform. + // TODO: Change this as soon as exec_group_compatible_with is available, which provides an + // explicit way to specify additional constraints for the test exec group. + assertThat(testAction.getExecutionPlatform().label()) + .isEqualTo(Label.parseCanonicalUnchecked("//platform:fast_cpu_platform")); + assertThat(testAction.getExecProperties()) + .containsExactly("require_fast_cpu", "true", "require_gpu", "true"); + } } diff --git a/src/test/shell/bazel/remote/BUILD b/src/test/shell/bazel/remote/BUILD index f6e2a899c24a6b..d4ac780aa5829f 100644 --- a/src/test/shell/bazel/remote/BUILD +++ b/src/test/shell/bazel/remote/BUILD @@ -28,6 +28,7 @@ sh_test( "//src/test/shell/bazel:test-deps", "//src/tools/remote:worker", "@bazel_tools//tools/bash/runfiles", + "@local_jdk//:jdk", # for remote_helpers setup_localjdk_javabase ], shard_count = 5, tags = [ diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 7c28e3fdb707fa..b6b66bca5e9370 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3449,7 +3449,29 @@ function test_platform_no_remote_exec_test_action() { exit 0 EOF chmod 755 a/test.sh + cat > a/rule.bzl <<'EOF' +def _my_test(ctx): + script = ctx.actions.declare_file(ctx.label.name) + ctx.actions.run_shell( + outputs = [script], + command = """ +cat > $1 <<'EOF2' +#!/usr/bin/env sh +exit 0 +EOF2 +chmod +x $1 +""", + arguments = [script.path], + ) + return [DefaultInfo(executable = script)] +my_test = rule( + implementation = _my_test, + test = True, +) +EOF cat > a/BUILD <<'EOF' +load(":rule.bzl", "my_test") + constraint_setting(name = "foo") constraint_value( @@ -3462,7 +3484,7 @@ platform( name = "host", constraint_values = [":has_foo"], exec_properties = { - "no-remote-exec": "1", + "test.no-remote-exec": "1", }, parents = ["@bazel_tools//tools:host_platform"], visibility = ["//visibility:public"], @@ -3480,44 +3502,45 @@ platform( }, ) -sh_test( +my_test( name = "test", - srcs = ["test.sh"], + # TODO: This uses a hack by setting test.no-remote-exec on the exec platform + # forced by this constraint for both the build and the test action. Instead, + # use exec_group_compatible_with = {"test": [":has_foo"]} once it is + # implemented. exec_compatible_with = [":has_foo"], ) -sh_test( +my_test( name = "test2", - srcs = ["test.sh"], - exec_compatible_with = [":has_foo"], - target_compatible_with = [":has_foo"], + exec_properties = { + "test.no-remote-exec": "1", + }, ) EOF - # A test target includes 2 actions: 1 build action (a) and 1 test action (b) - # This test currently demonstrates that: - # - (b) would always be executed on Bazel's target platform, set by "--platforms=" flag. - # - Regardless of 'no-remote-exec' set on (b)'s platform, (b) would still be executed remotely. - # The remote test action will be sent with `"no-remote-exec": "1"` in it's platform. - # - # TODO: Make this test's result consistent with 'test_platform_no_remote_exec'. - # Test action (b) should be executed locally instead of remotely in this setup. - + # A my_test target includes 2 actions: 1 build action (a) and 1 test action (b), + # with (b) running two spawns (test execution, test XML generation). + # The genrule spawn runs remotely, both test spawns run locally. bazel test \ --extra_execution_platforms=//a:remote,//a:host \ --platforms=//a:remote \ --spawn_strategy=remote,local \ --remote_executor=grpc://localhost:${worker_port} \ //a:test >& $TEST_log || fail "Failed to test //a:test" - expect_log "1 local, 1 remote" + expect_log "2 local, 1 remote" + # Note: While the test spawns are executed locally, they still select the + # remote platform as it is the first registered execution platform and there + # are no constraints to force a different one. This is not desired behavior, + # but it isn't covered by this test. bazel test \ --extra_execution_platforms=//a:remote,//a:host \ --platforms=//a:host \ --spawn_strategy=remote,local \ --remote_executor=grpc://localhost:${worker_port} \ //a:test2 >& $TEST_log || fail "Failed to test //a:test2" - expect_log "1 local, 1 remote" + expect_log "2 local, 1 remote" bazel clean @@ -3527,7 +3550,7 @@ EOF --spawn_strategy=remote,local \ --remote_executor=grpc://localhost:${worker_port} \ //a:test >& $TEST_log || fail "Failed to test //a:test" - expect_log "2 remote cache hit" + expect_log "3 remote cache hit" bazel test \ --extra_execution_platforms=//a:remote,//a:host \ @@ -3535,7 +3558,7 @@ EOF --spawn_strategy=remote,local \ --remote_executor=grpc://localhost:${worker_port} \ //a:test2 >& $TEST_log || fail "Failed to test //a:test2" - expect_log "2 remote cache hit" + expect_log "3 remote cache hit" } function setup_inlined_outputs() { From 9160b4ecedf75ccd00adf8d2028c5eaf4e7e43ed Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 21 Jan 2025 15:25:29 +0100 Subject: [PATCH 2/2] Add references --- .../devtools/build/lib/analysis/StarlarkExecGroupTest.java | 1 + src/test/shell/bazel/remote/remote_execution_test.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java index 6004ca96941c4c..d46ae7451a2de3 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java @@ -863,6 +863,7 @@ def _impl(ctx): // the exec_compatible_with constraint and thus select the target platform. // TODO: Change this as soon as exec_group_compatible_with is available, which provides an // explicit way to specify additional constraints for the test exec group. + // https://github.com/bazelbuild/bazel/issues/23802 assertThat(testAction.getExecutionPlatform().label()) .isEqualTo(Label.parseCanonicalUnchecked("//platform:fast_cpu_platform")); assertThat(testAction.getExecProperties()) diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index b6b66bca5e9370..43a4130315251b 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3508,6 +3508,7 @@ my_test( # forced by this constraint for both the build and the test action. Instead, # use exec_group_compatible_with = {"test": [":has_foo"]} once it is # implemented. + # https://github.com/bazelbuild/bazel/issues/23802 exec_compatible_with = [":has_foo"], )