Refactoring: include exec_properties as a separate field in Spawn and change the code to use that instead of the platform.

Googler · copybara-github · commit 0dc53a2217f3 · 2019-08-25T22:10:45.000-07:00
This is in preparation for target level execution properties (See https://docs.google.com/document/d/1w3fu8zu_sRw_gK1dFAvkY2suhbQQ82tc0zdjet-dpCI/edit#heading=h.5mcn15i0e1ch) RELNOTES: None PiperOrigin-RevId: 265386594
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -16,6 +16,7 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
@@ -549,6 +550,11 @@ public SkylarkDict<String, String> getEnv() {
     return SkylarkDict.copyOf(null, env.getFixedEnv().toMap());
   }
 
+  @Override
+  public ImmutableMap<String, String> getExecProperties() {
+    return getOwner().getExecProperties();
+  }
+
   @Override
   @Nullable
   public PlatformInfo getExecutionPlatform() {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import javax.annotation.Nullable;
@@ -250,6 +251,9 @@ default boolean hasLooseHeaders() {
     return false;
   }
 
+  /** Returns a String to String map containing the execution properties of this action. */
+  ImmutableMap<String, String> getExecProperties();
+
   /**
    * Returns the {@link PlatformInfo} platform this action should be executed on. If the execution
    * platform is {@code null}, then the host platform is assumed.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java b/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java
@@ -40,6 +40,8 @@ public final String getKey(ActionKeyContext actionKeyContext) {
               getExecutionPlatform().addTo(fp);
             }
 
+            fp.addStringMap(getExecProperties());
+
             // Compute the actual key and store it.
             cachedKey = fp.hexDigestAndReset();
           } catch (CommandLineExpansionException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java b/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java
@@ -16,6 +16,7 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -46,6 +47,7 @@ public abstract class ActionOwner {
           "system",
           null,
           null,
+          ImmutableMap.<String, String>of(),
           null);
 
   @AutoCodec.Instantiator
@@ -58,6 +60,7 @@ public static ActionOwner create(
       String configurationChecksum,
       @Nullable BuildConfigurationEvent configuration,
       @Nullable String additionalProgressInfo,
+      ImmutableMap<String, String> execProperties,
       @Nullable PlatformInfo executionPlatform) {
     return new AutoValue_ActionOwner(
         location,
@@ -68,6 +71,7 @@ public static ActionOwner create(
         configuration,
         targetKind,
         additionalProgressInfo,
+        execProperties,
         executionPlatform);
   }
 
@@ -111,6 +115,9 @@ public static ActionOwner create(
   @Nullable
   abstract String getAdditionalProgressInfo();
 
+  /** Returns a String to String map containing the execution properties of this action. */
+  abstract ImmutableMap<String, String> getExecProperties();
+
   /**
    * Returns the {@link PlatformInfo} platform this action should be executed on. If the execution
    * platform is {@code null}, then the host platform is assumed.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionTemplate.java b/src/main/java/com/google/devtools/build/lib/actions/ActionTemplate.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
@@ -88,6 +89,11 @@ Iterable<T> generateActionForInputArtifacts(
   /** Returns the output TreeArtifact. */
   Artifact getOutputTreeArtifact();
 
+  @Override
+  default ImmutableMap<String, String> getExecProperties() {
+    return ImmutableMap.<String, String>of();
+  }
+
   @Override
   @Nullable
   default PlatformInfo getExecutionPlatform() {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java
@@ -150,6 +150,11 @@ public String getMnemonic() {
     return action.getMnemonic();
   }
 
+  @Override
+  public ImmutableMap<String, String> getCombinedExecProperties() {
+    return action.getOwner().getExecProperties();
+  }
+
   @Override
   @Nullable
   public PlatformInfo getExecutionPlatform() {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java
@@ -87,6 +87,11 @@ public String getMnemonic() {
     return spawn.getMnemonic();
   }
 
+  @Override
+  public ImmutableMap<String, String> getCombinedExecProperties() {
+    return spawn.getCombinedExecProperties();
+  }
+
   @Override
   @Nullable
   public PlatformInfo getExecutionPlatform() {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java
@@ -139,6 +139,11 @@ public String getMnemonic() {
     return owner.getMnemonic();
   }
 
+  @Override
+  public ImmutableMap<String, String> getCombinedExecProperties() {
+    return owner.getExecProperties();
+  }
+
   @Override
   @Nullable
   public PlatformInfo getExecutionPlatform() {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java
@@ -111,6 +111,14 @@ public interface Spawn {
    */
   String getMnemonic();
 
+  /**
+   * Returns execution properties related to this spawn.
+   *
+   * <p>Note that this includes data from the execution platform's exec_properties as well as
+   * target-level exec_properties. TODO(agoulti): implement target-level exec_properties.
+   */
+  ImmutableMap<String, String> getCombinedExecProperties();
+
   @Nullable
   PlatformInfo getExecutionPlatform();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -514,6 +514,14 @@ public static ActionOwner createActionOwner(
       ImmutableList<AspectDescriptor> aspectDescriptors,
       BuildConfiguration configuration,
       @Nullable PlatformInfo executionPlatform) {
+    ImmutableMap<String, String> execProperties;
+    if (executionPlatform != null) {
+      execProperties = executionPlatform.execProperties();
+    } else {
+      execProperties = ImmutableMap.of();
+    }
+    // TODO(agoulti): Insert logic to include per-target execution properties
+
     return ActionOwner.create(
         rule.getLabel(),
         aspectDescriptors,
@@ -523,6 +531,7 @@ public static ActionOwner createActionOwner(
         configuration.checksum(),
         configuration.toBuildEvent(),
         configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null,
+        execProperties,
         executionPlatform);
   }
 
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
@@ -19,6 +19,7 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Ordering;
+import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.protobuf.TextFormat;
@@ -33,35 +34,36 @@
 public final class PlatformUtils {
 
   @Nullable
-  public static Platform getPlatformProto(
-      @Nullable PlatformInfo executionPlatform, @Nullable RemoteOptions remoteOptions)
+  public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions remoteOptions)
       throws UserExecException {
     SortedMap<String, String> defaultExecProperties =
         remoteOptions != null
             ? remoteOptions.getRemoteDefaultExecProperties()
             : ImmutableSortedMap.of();
 
-    if (executionPlatform == null && defaultExecProperties.isEmpty()) {
+    if (spawn.getExecutionPlatform() == null
+        && spawn.getCombinedExecProperties().isEmpty()
+        && defaultExecProperties.isEmpty()) {
       return null;
     }
 
     Platform.Builder platformBuilder = Platform.newBuilder();
 
-    if (executionPlatform != null && !executionPlatform.execProperties().isEmpty()) {
-      for (Map.Entry<String, String> entry : executionPlatform.execProperties().entrySet()) {
+    if (!spawn.getCombinedExecProperties().isEmpty()) {
+      for (Map.Entry<String, String> entry : spawn.getCombinedExecProperties().entrySet()) {
         platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue());
       }
-    } else if (executionPlatform != null
-        && !Strings.isNullOrEmpty(executionPlatform.remoteExecutionProperties())) {
+    } else if (spawn.getExecutionPlatform() != null
+        && !Strings.isNullOrEmpty(spawn.getExecutionPlatform().remoteExecutionProperties())) {
       // Try and get the platform info from the execution properties.
       try {
         TextFormat.getParser()
-            .merge(executionPlatform.remoteExecutionProperties(), platformBuilder);
+            .merge(spawn.getExecutionPlatform().remoteExecutionProperties(), platformBuilder);
       } catch (ParseException e) {
         throw new UserExecException(
             String.format(
                 "Failed to parse remote_execution_properties from platform %s",
-                executionPlatform.label()),
+                spawn.getExecutionPlatform().label()),
             e);
       }
     } else {
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java
@@ -129,8 +129,7 @@ public void logSpawn(
     }
     builder.setRemotable(Spawns.mayBeExecutedRemotely(spawn));
 
-    Platform execPlatform =
-        PlatformUtils.getPlatformProto(spawn.getExecutionPlatform(), remoteOptions);
+    Platform execPlatform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
     if (execPlatform != null) {
       builder.setPlatform(buildPlatform(execPlatform));
     }
diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java
@@ -194,6 +194,11 @@ public RunfilesSupplier getRunfilesSupplier() {
       throw new UnsupportedOperationException();
     }
 
+    @Override
+    public ImmutableMap<String, String> getExecProperties() {
+      return actionExecutionMetadata.getExecProperties();
+    }
+
     @Override
     @Nullable
     public PlatformInfo getExecutionPlatform() {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -132,7 +132,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
     Digest merkleTreeRoot = merkleTree.getRootDigest();
 
     // Get the remote platform properties.
-    Platform platform = PlatformUtils.getPlatformProto(spawn.getExecutionPlatform(), options);
+    Platform platform = PlatformUtils.getPlatformProto(spawn, options);
 
     Command command =
         RemoteSpawnRunner.buildCommand(
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -169,7 +169,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
     maybeWriteParamFilesLocally(spawn);
 
     // Get the remote platform properties.
-    Platform platform = PlatformUtils.getPlatformProto(spawn.getExecutionPlatform(), remoteOptions);
+    Platform platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
 
     Command command =
         buildCommand(
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java
@@ -377,8 +377,7 @@ private String executeCommand(List<String> cmdLine, InputStream stdIn) throws Us
 
   private Optional<String> dockerContainerFromSpawn(Spawn spawn) throws ExecException {
     Platform platform =
-        PlatformUtils.getPlatformProto(
-            spawn.getExecutionPlatform(), cmdEnv.getOptions().getOptions(RemoteOptions.class));
+        PlatformUtils.getPlatformProto(spawn, cmdEnv.getOptions().getOptions(RemoteOptions.class));
 
     if (platform != null) {
       try {
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
@@ -17,6 +17,7 @@
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ResourceManager.ResourceHandle;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
@@ -547,6 +548,11 @@ public MiddlemanType getActionType() {
       throw new IllegalStateException();
     }
 
+    @Override
+    public ImmutableMap<String, String> getExecProperties() {
+      throw new IllegalStateException();
+    }
+
     @Nullable
     @Override
     public PlatformInfo getExecutionPlatform() {
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
@@ -346,6 +346,7 @@ public Label getLabel() {
           "dummy-configuration",
           null,
           null,
+          ImmutableMap.<String, String>of(),
           null);
 
   @AutoCodec
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD
@@ -23,6 +23,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib:packages-internal",
         "//src/main/java/com/google/devtools/build/lib:syntax",
         "//src/main/java/com/google/devtools/build/lib:util",
+        "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils",
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
diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java
diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/UiStateTrackerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/UiStateTrackerTest.java

Original file line number	Diff line number	Diff line change
`@@ -40,6 +40,8 @@ public final String getKey(ActionKeyContext actionKeyContext) {`
`40`	`40`	`getExecutionPlatform().addTo(fp);`
`41`	`41`	`}`
`42`	`42`
	`43`	`+ fp.addStringMap(getExecProperties());`
	`44`	`+`
`43`	`45`	`// Compute the actual key and store it.`
`44`	`46`	`cachedKey = fp.hexDigestAndReset();`
`45`	`47`	`} catch (CommandLineExpansionException e) {`