Skip to content

Commit

Permalink
fix: Use execProperties when remoteExecutionProperties is empty
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@stevebarrau @copybara-github
stevebarrau authored and copybara-github committed Oct 13, 2023
1 parent 19c72e3 commit 6f24fa5
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 28 deletions.
40 changes: 22 additions & 18 deletions src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -84,38 +83,43 @@ public static Platform getPlatformProto(
return null;
}

Map<String, String> properties;
Map<String, String> 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;
}

Expand Down
95 changes: 85 additions & 10 deletions src/test/shell/bazel/remote/remote_execution_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -1352,40 +1352,40 @@ 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.
expect_log "1 process: 1 internal"

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"}' \
Expand All @@ -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
Expand Down

0 comments on commit 6f24fa5

Please sign in to comment.