commit 3a5b3606a6f5433467a5b49f0188c41411684bf5
author Chi Wang <chiwang@google.com> Fri Oct 29 05:21:26 2021 -0700
committer Copybara-Service <copybara-worker@google.com> Fri Oct 29 05:22:42 2021 -0700
tree af7b090e5875f79240fb08e548a2f8470559f42c
parent eacb32f463536ecbc42cbdfcf2c091449840302d

Remote: Merge target-level exec_properties with --remote_default_exec_properties

Per-target `exec_properties` was introduced by 0dc53a2217f32da737410883158d42c41b6d1d61 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 https://github.com/bazelbuild/bazel/issues/10252.

Closes #14193.

PiperOrigin-RevId: 406337649

src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java

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 7922735..503b649 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
@@ -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;
@@ -78,7 +79,19 @@
     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

src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java
index 0b013fb..fc15ae5 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java
@@ -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;
@@ -45,8 +47,9 @@
 
   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;
   }
 
@@ -93,7 +96,7 @@
             .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
@@ -115,4 +118,28 @@
     // 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);
+  }
 }