Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Use execProperties when remoteExecutionProperties is empty #19792

Closed
wants to merge 8 commits into from
Closed

fix: Use execProperties when remoteExecutionProperties is empty #19792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b0fd9d5
fix: Use execProperties when remoteExecutionProperties is empty
stevebarrau Oct 11, 2023
252fa0e
reverse if
stevebarrau Oct 11, 2023
12df1e1
test progress
stevebarrau Oct 12, 2023
bd77add
add tests
stevebarrau Oct 12, 2023
c3fa67e
remove recreate
stevebarrau Oct 13, 2023
6caeee6
allow to add an empty map
stevebarrau Oct 13, 2023
30709fd
cleanup tests
stevebarrau Oct 13, 2023
703a830
typo
stevebarrau Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 22 additions & 18 deletions src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java
View file
Open in desktop
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
Open in desktop
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"
stevebarrau marked this conversation as resolved.
Show resolved Hide resolved

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
Loading