Skip to content

Commit

Permalink
Remote: Merge target-level exec_properties with --remote_default_exec…
Browse files Browse the repository at this point in the history
…_properties

Per-target `exec_properties` was introduced by 0dc53a2 but it didn't take into account of `--remote_default_exec_properties` which provides default exec properties for the execution platform if it doesn't already set with `exec_properties`. So the per-target `exec_properties` overrides `--remote_default_exec_properties` instead of merging with them which is wrong.

Fixes bazelbuild#10252.

Closes bazelbuild#14193.

PiperOrigin-RevId: 406337649
  • Loading branch information
coeuvre authored and copybara-github committed Oct 29, 2021
1 parent eacb32f commit 3a5b360
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.protobuf.TextFormat;
import com.google.protobuf.TextFormat.ParseException;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.SortedMap;
Expand Down Expand Up @@ -78,7 +79,19 @@ public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions rem
Platform.Builder platformBuilder = Platform.newBuilder();

if (!spawn.getCombinedExecProperties().isEmpty()) {
for (Map.Entry<String, String> entry : spawn.getCombinedExecProperties().entrySet()) {
Map<String, String> combinedExecProperties;
// Apply default exec properties if the execution platform does not already set
// exec_properties
if (spawn.getExecutionPlatform() == null
|| spawn.getExecutionPlatform().execProperties().isEmpty()) {
combinedExecProperties = new HashMap<>();
combinedExecProperties.putAll(defaultExecProperties);
combinedExecProperties.putAll(spawn.getCombinedExecProperties());
} else {
combinedExecProperties = spawn.getCombinedExecProperties();
}

for (Map.Entry<String, String> entry : combinedExecProperties.entrySet()) {
platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue());
}
} else if (spawn.getExecutionPlatform() != null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@
import static com.google.common.truth.Truth.assertThat;

import build.bazel.remote.execution.v2.Platform;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.exec.util.SpawnBuilder;
import com.google.devtools.build.lib.remote.options.RemoteOptions;
import com.google.devtools.common.options.Options;
import java.util.AbstractMap;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -45,8 +47,9 @@ private static String platformOptionsString() {

private static RemoteOptions remoteOptions() {
RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class);
remoteOptions.remoteDefaultPlatformProperties = platformOptionsString();

remoteOptions.remoteDefaultExecProperties =
ImmutableList.of(
new AbstractMap.SimpleEntry<>("b", "2"), new AbstractMap.SimpleEntry<>("a", "1"));
return remoteOptions;
}

Expand Down Expand Up @@ -93,7 +96,7 @@ public void testParsePlatformSortsProperties_execProperties() throws Exception {
.addProperties(Platform.Property.newBuilder().setName("zz").setValue("66"))
.build();
// execProperties are sorted by key
assertThat(PlatformUtils.getPlatformProto(s, remoteOptions())).isEqualTo(expected);
assertThat(PlatformUtils.getPlatformProto(s, null)).isEqualTo(expected);
}

@Test
Expand All @@ -115,4 +118,28 @@ public void testGetPlatformProto_differentiateWorkspace() throws Exception {
// execProperties are sorted by key
assertThat(PlatformUtils.getPlatformProto(s, remoteOptions())).isEqualTo(expected);
}

@Test
public void getPlatformProto_mergeTargetExecPropertiesWithPlatform() throws Exception {
Spawn spawn = new SpawnBuilder("dummy").withExecProperties(ImmutableMap.of("c", "3")).build();
Platform expected =
Platform.newBuilder()
.addProperties(Platform.Property.newBuilder().setName("a").setValue("1"))
.addProperties(Platform.Property.newBuilder().setName("b").setValue("2"))
.addProperties(Platform.Property.newBuilder().setName("c").setValue("3"))
.build();
assertThat(PlatformUtils.getPlatformProto(spawn, remoteOptions())).isEqualTo(expected);
}

@Test
public void getPlatformProto_targetExecPropertiesConflictWithPlatform_override()
throws Exception {
Spawn spawn = new SpawnBuilder("dummy").withExecProperties(ImmutableMap.of("b", "3")).build();
Platform expected =
Platform.newBuilder()
.addProperties(Platform.Property.newBuilder().setName("a").setValue("1"))
.addProperties(Platform.Property.newBuilder().setName("b").setValue("3"))
.build();
assertThat(PlatformUtils.getPlatformProto(spawn, remoteOptions())).isEqualTo(expected);
}
}

0 comments on commit 3a5b360

Please sign in to comment.