From 2a846c7a831f89db4065ecf14f0078c441cd4028 Mon Sep 17 00:00:00 2001 From: Steve Barrau <98589981+stevebarrau@users.noreply.github.com> Date: Thu, 12 Oct 2023 18:04:56 +0100 Subject: [PATCH] add tests --- .../lib/analysis/platform/PlatformUtils.java | 12 +-- .../bazel/remote/remote_execution_test.sh | 91 ++++++++++++++++--- 2 files changed, 82 insertions(+), 21 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 57cc9ce915f677..be1fee949cc290 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 @@ -83,13 +83,12 @@ 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 { @@ -97,10 +96,9 @@ public static Platform getPlatformProto( } } else if (spawn.getExecutionPlatform() != null) { properties = new HashMap<>(); - Platform.Builder platformBuilder = Platform.newBuilder(); - String remoteExecutionProperties = spawn.getExecutionPlatform().remoteExecutionProperties(); if (!remoteExecutionProperties.isEmpty()) { + Platform.Builder platformBuilder = Platform.newBuilder(); // Try and get the platform info from the execution properties. try { TextFormat.getParser() @@ -117,10 +115,12 @@ public static Platform getPlatformProto( for (Property property : platformBuilder.getPropertiesList()) { properties.put(property.getName(), property.getValue()); } - } else { + } else if (!spawn.getExecutionPlatform().execProperties().isEmpty()) { 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 c379d98554dbfe..e50f26222042ae 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1336,19 +1336,11 @@ EOF [[ ! -f bazel-bin/test.runfiles/MANIFEST ]] || fail "expected output manifest to exist" } -# FIXME remove this comment -# -# - I run this test with: bazel test //src/test/shell/bazel/remote:remote_execution_test --test_filter=test_platform_default_properties_invalidation -# - I do not understand how to set the platform yet function test_platform_default_properties_invalidation() { # Test that when changing values of --remote_default_platform_properties all actions are - # invalidated. + # invalidated if no platform is used. mkdir -p test cat > test/BUILD << 'EOF' -platform( - name = "platform_without_any_exec_properties", -) - genrule( name = "test", srcs = [], @@ -1358,7 +1350,6 @@ genrule( EOF bazel build \ - --extra_execution_platforms=//test:platform_without_any_exec_properties \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=1234" \ //test:test >& $TEST_log || fail "Failed to build //test:test" @@ -1366,7 +1357,6 @@ EOF expect_log "2 processes: 1 internal, 1 remote" bazel build \ - --extra_execution_platforms=//test:platform_without_any_exec_properties \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=88888" \ //test:test >& $TEST_log || fail "Failed to build //test:test" @@ -1374,10 +1364,8 @@ EOF # 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" - # FIXME the above assert fails bazel build \ - --extra_execution_platforms=//test:platform_without_any_exec_properties \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=88888" \ //test:test >& $TEST_log || fail "Failed to build //test:test" @@ -1389,7 +1377,6 @@ EOF bazel shutdown bazel build \ - --extra_execution_platforms=//test:platform_without_any_exec_properties \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=88888" \ //test:test >& $TEST_log || fail "Failed to build //test:test" @@ -1399,7 +1386,6 @@ EOF expect_log "1 process: 1 internal" bazel build\ - --extra_execution_platforms=//test:platform_without_any_exec_properties \ --remote_executor=grpc://localhost:${worker_port} \ --remote_default_exec_properties="build=88888" \ --remote_default_platform_properties='properties:{name:"build" value:"1234"}' \ @@ -1411,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_without_any_exec_properties", + exec_properties = { + "foo": "bar", + }, +) + +genrule( + name = "test", + srcs = [], + outs = ["output.txt"], + cmd = "echo \"foo\" > \"$@\"" +) +EOF + + bazel build \ + --extra_execution_platforms=//test:platform_without_any_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_without_any_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 super-seeded 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_without_any_exec_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_without_any_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_without_any_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 super-seeded 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