From 6f24fa5de1a376ea93e95458cf7bc2cfcae9ce17 Mon Sep 17 00:00:00 2001 From: Steve Barrau <98589981+stevebarrau@users.noreply.github.com> Date: Fri, 13 Oct 2023 11:39:53 -0700 Subject: [PATCH] fix: Use execProperties when remoteExecutionProperties is empty When defining a platform (for RBE); [remote_execution_properties](https://bazel.build/reference/be/platforms-and-toolchains#platform.remote_execution_properties) is marked as deprecated, thus we do not set it. If it is not set getPlatformProto does not read the execProperties instead, before defaulting to defaultExecProperties. Closes #19792. PiperOrigin-RevId: 573278209 Change-Id: Ifadf043fe728ce602904db576785e1a4473b0037 --- .../lib/analysis/platform/PlatformUtils.java | 40 ++++---- .../bazel/remote/remote_execution_test.sh | 95 +++++++++++++++++-- 2 files changed, 107 insertions(+), 28 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java index 3fef4f8616a970..3366b6b3ab39fe 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java @@ -16,7 +16,6 @@ import build.bazel.remote.execution.v2.Platform; import build.bazel.remote.execution.v2.Platform.Property; -import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Ordering; @@ -84,38 +83,43 @@ public static Platform getPlatformProto( return null; } - Map properties; + Map properties = new HashMap<>(); if (!spawn.getCombinedExecProperties().isEmpty()) { // Apply default exec properties if the execution platform does not already set // exec_properties if (spawn.getExecutionPlatform() == null || spawn.getExecutionPlatform().execProperties().isEmpty()) { - properties = new HashMap<>(); properties.putAll(defaultExecProperties); properties.putAll(spawn.getCombinedExecProperties()); } else { properties = spawn.getCombinedExecProperties(); } - } else if (spawn.getExecutionPlatform() != null - && !Strings.isNullOrEmpty(spawn.getExecutionPlatform().remoteExecutionProperties())) { - properties = new HashMap<>(); - // Try and get the platform info from the execution properties. - try { + } else if (spawn.getExecutionPlatform() != null) { + String remoteExecutionProperties = spawn.getExecutionPlatform().remoteExecutionProperties(); + if (!remoteExecutionProperties.isEmpty()) { Platform.Builder platformBuilder = Platform.newBuilder(); - TextFormat.getParser() - .merge(spawn.getExecutionPlatform().remoteExecutionProperties(), platformBuilder); + // Try and get the platform info from the execution properties. + try { + TextFormat.getParser() + .merge(spawn.getExecutionPlatform().remoteExecutionProperties(), platformBuilder); + } catch (ParseException e) { + String message = + String.format( + "Failed to parse remote_execution_properties from platform %s", + spawn.getExecutionPlatform().label()); + throw new UserExecException( + e, createFailureDetail(message, Code.INVALID_REMOTE_EXECUTION_PROPERTIES)); + } + for (Property property : platformBuilder.getPropertiesList()) { properties.put(property.getName(), property.getValue()); } - } catch (ParseException e) { - String message = - String.format( - "Failed to parse remote_execution_properties from platform %s", - spawn.getExecutionPlatform().label()); - throw new UserExecException( - e, createFailureDetail(message, Code.INVALID_REMOTE_EXECUTION_PROPERTIES)); + } else { + properties.putAll(spawn.getExecutionPlatform().execProperties()); } - } else { + } + + if (properties.isEmpty()) { properties = defaultExecProperties; } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 798add16d0066e..9c3e29533bd6cd 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1338,8 +1338,8 @@ EOF function test_platform_default_properties_invalidation() { # Test that when changing values of --remote_default_platform_properties all actions are - # invalidated. -mkdir -p test + # invalidated if no platform is used. + mkdir -p test cat > test/BUILD << 'EOF' genrule( name = "test", @@ -1352,23 +1352,23 @@ EOF bazel build \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=1234" \ - //test:test >& $TEST_log || fail "Failed to build //a:remote" + //test:test >& $TEST_log || fail "Failed to build //test:test" expect_log "2 processes: 1 internal, 1 remote" bazel build \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=88888" \ - //test:test >& $TEST_log || fail "Failed to build //a:remote" + //test:test >& $TEST_log || fail "Failed to build //test:test" # Changing --remote_default_platform_properties value should invalidate SkyFrames in-memory # caching and make it re-run the action. expect_log "2 processes: 1 internal, 1 remote" - bazel build \ + bazel build \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=88888" \ - //test:test >& $TEST_log || fail "Failed to build //a:remote" + //test:test >& $TEST_log || fail "Failed to build //test:test" # The same value of --remote_default_platform_properties should NOT invalidate SkyFrames in-memory cache # and make the action should not be re-run. @@ -1376,16 +1376,16 @@ EOF bazel shutdown - bazel build \ + bazel build \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=88888" \ - //test:test >& $TEST_log || fail "Failed to build //a:remote" + //test:test >& $TEST_log || fail "Failed to build //test:test" - # The same value of --remote_default_platform_properties should NOT invalidate SkyFrames od-disk cache + # The same value of --remote_default_platform_properties should NOT invalidate SkyFrames on-disk cache # and the action should not be re-run. expect_log "1 process: 1 internal" - bazel build\ + bazel build \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=88888" \ --remote_default_platform_properties='properties:{name:"build" value:"1234"}' \ @@ -1397,6 +1397,81 @@ EOF expect_log "Setting both --remote_default_platform_properties and --remote_default_exec_properties is not allowed" } +function test_platform_default_properties_invalidation_with_platform_exec_properties() { + # Test that when changing values of --remote_default_platform_properties all actions are + # invalidated. + mkdir -p test + cat > test/BUILD << 'EOF' +platform( + name = "platform_with_exec_properties", + exec_properties = { + "foo": "bar", + }, +) + +genrule( + name = "test", + srcs = [], + outs = ["output.txt"], + cmd = "echo \"foo\" > \"$@\"" +) +EOF + + bazel build \ + --extra_execution_platforms=//test:platform_with_exec_properties \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_default_exec_properties="build=1234" \ + //test:test >& $TEST_log || fail "Failed to build //test:test" + + expect_log "2 processes: 1 internal, 1 remote" + + bazel build \ + --extra_execution_platforms=//test:platform_with_exec_properties \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_default_exec_properties="build=88888" \ + //test:test >& $TEST_log || fail "Failed to build //test:test" + + # Changing --remote_default_platform_properties value does not invalidate SkyFrames + # given its is superseded by the platform exec_properties. + expect_log "1 process: 1 internal." +} + +function test_platform_default_properties_invalidation_with_platform_remote_execution_properties() { + # Test that when changing values of --remote_default_platform_properties all actions are + # invalidated. + mkdir -p test + cat > test/BUILD << 'EOF' +platform( + name = "platform_with_remote_execution_properties", + remote_execution_properties = """properties: {name: "foo" value: "baz"}""", +) + +genrule( + name = "test", + srcs = [], + outs = ["output.txt"], + cmd = "echo \"foo\" > \"$@\"" +) +EOF + + bazel build \ + --extra_execution_platforms=//test:platform_with_remote_execution_properties \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_default_exec_properties="build=1234" \ + //test:test >& $TEST_log || fail "Failed to build //test:test" + + expect_log "2 processes: 1 internal, 1 remote" + + bazel build \ + --extra_execution_platforms=//test:platform_with_remote_execution_properties \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_default_exec_properties="build=88888" \ + //test:test >& $TEST_log || fail "Failed to build //test:test" + + # Changing --remote_default_platform_properties value does not invalidate SkyFrames + # given its is superseded by the platform remote_execution_properties. + expect_log "2 processes: 1 remote cache hit, 1 internal" +} function test_combined_disk_remote_exec_with_flag_combinations() { rm -f ${TEST_TMPDIR}/test_expected