From 4d7f8f7846960ffc111cf1aef2a5efb094114442 Mon Sep 17 00:00:00 2001
From: ulfjack <ulfjack@google.com>
Date: Wed, 29 Nov 2017 03:37:04 -0800
Subject: Clean up ExecutionRequirements

- remove BaseSpawn.Local; instead, all callers pass in the full set of
  execution requirements they want to set
- disable caching and sandboxing for the symlink tree action - it does not
  declare outputs, so it can't be cached or sandboxed (fixes #4041)
- centralize the existing execution requirements in the ExecutionRequirements
  class
- centralize checking for execution requirements in the Spawn class
  (it's possible that we may need a more decentralized, extensible design in
  the future, but for now having them in a single place is simple and
  effective)
- update the documentation
- forward the relevant tags to execution requirements in TargetUtils (progress
  on #3960)
- this also contributes to #4153

PiperOrigin-RevId: 177288598
---
 .../docgen/templates/attributes/common/tags.html   | 17 ++++-
 .../devtools/build/lib/actions/BaseSpawn.java      | 36 ---------
 .../devtools/build/lib/actions/DelegateSpawn.java  | 10 ---
 .../build/lib/actions/ExecutionRequirements.java   | 36 ++++++++-
 .../devtools/build/lib/actions/SimpleSpawn.java    | 10 ---
 .../google/devtools/build/lib/actions/Spawn.java   | 11 ---
 .../google/devtools/build/lib/actions/Spawns.java  | 15 ++++
 .../build/lib/analysis/config/BinTools.java        | 36 +++++++--
 .../devtools/build/lib/exec/SymlinkTreeHelper.java | 88 +++++++++++++---------
 .../build/lib/exec/SymlinkTreeStrategy.java        | 17 +++--
 .../devtools/build/lib/exec/TestStrategy.java      |  7 +-
 .../devtools/build/lib/packages/TargetUtils.java   | 11 ++-
 .../build/lib/remote/RemoteSpawnRunner.java        |  2 +-
 .../lib/sandbox/DarwinSandboxedSpawnRunner.java    |  3 +-
 .../lib/sandbox/LinuxSandboxedSpawnRunner.java     |  6 +-
 .../lib/sandbox/SandboxActionContextProvider.java  |  3 +-
 .../devtools/build/lib/sandbox/SandboxHelpers.java | 14 +---
 17 files changed, 185 insertions(+), 137 deletions(-)

(limited to 'src/main')

diff --git a/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html b/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html
index 5ccc56ba33..55b73d2560 100644
--- a/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html
+++ b/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html
@@ -21,6 +21,20 @@
 </p>
 
 <ul>
+  <li><code>no-sandbox</code> keyword results in the action or test never being
+    <a href="../user-manual.html#sandboxing">sandboxed</a>; it can still be
+    cached or run remotely - use <code>no-cache</code> or <code>no-remote</code>
+    to prevent either or both of those.
+  </li>
+
+  <li><code>no-cache</code> keyword results in the action or test never being
+    cached.
+  </li>
+
+  <li><code>no-remote</code> keyword results in the action or test never being
+    executed remotely (but it may be cached remotely).
+  </li>
+
   <li><code>local</code> keyword results in the action or test never being
     run remotely or inside the
     <a href="../user-manual.html#sandboxing">sandbox</a>.
@@ -30,7 +44,8 @@
 
   <li><code>block-network</code> keyword blocks access to the external
     network from inside the sandbox. In this case, only communication
-    with localhost is allowed.
+    with localhost is allowed. This tag only has an effect if sandboxing is
+    enabled.
   </li>
 
   <li><code>requires-fakeroot</code> runs the test or action as uid and gid 0 (i.e., the root
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
index 901b225afe..bc52055207 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java
@@ -68,16 +68,6 @@ public class BaseSpawn implements Spawn {
     return pathFragment.getParentDirectory().getChild(pathFragment.getBaseName() + ".runfiles");
   }
 
-  @Override
-  public boolean hasNoSandbox() {
-    return executionInfo.containsKey("nosandbox");
-  }
-
-  @Override
-  public boolean isRemotable() {
-    return !executionInfo.containsKey("local");
-  }
-
   @Override
   public final ImmutableMap<String, String> getExecutionInfo() {
     return executionInfo;
@@ -157,30 +147,4 @@ public class BaseSpawn implements Spawn {
   public String getMnemonic() {
     return action.getMnemonic();
   }
-
-  /** A local spawn. */
-  public static class Local extends BaseSpawn {
-    public Local(
-        List<String> arguments, Map<String, String> environment, ActionExecutionMetadata action,
-        ResourceSet localResources) {
-      this(arguments, environment, ImmutableMap.<String, String>of(), action, localResources);
-    }
-
-    public Local(
-        List<String> arguments,
-        Map<String, String> environment,
-        Map<String, String> executionInfo,
-        ActionExecutionMetadata action,
-        ResourceSet localResources) {
-      super(arguments, environment, buildExecutionInfo(executionInfo), action, localResources);
-    }
-
-    private static ImmutableMap<String, String> buildExecutionInfo(
-        Map<String, String> additionalExecutionInfo) {
-      ImmutableMap.Builder<String, String> executionInfo = ImmutableMap.builder();
-      executionInfo.putAll(additionalExecutionInfo);
-      executionInfo.put("local", "");
-      return executionInfo.build();
-    }
-  }
 }
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
index 8341301053..fd548b66cd 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java
@@ -35,16 +35,6 @@ public class DelegateSpawn implements Spawn {
     return spawn.getExecutionInfo();
   }
 
-  @Override
-  public boolean isRemotable() {
-    return spawn.isRemotable();
-  }
-
-  @Override
-  public boolean hasNoSandbox() {
-    return spawn.hasNoSandbox();
-  }
-
   @Override
   public ImmutableList<Artifact> getFilesetManifests() {
     return spawn.getFilesetManifests();
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java
index 22242de04c..fced3688b0 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java
@@ -145,8 +145,40 @@ public class ExecutionRequirements {
       ImmutableMap.of(SUPPORTS_WORKERS, "1");
 
   /**
-   * Whether we should disable remote caching of an action. This can be set to force a rerun of an
-   * action even if there is a cache entry for it.
+   * Requires local execution without sandboxing for a spawn.
+   *
+   * <p>This tag is deprecated; use no-cache, no-remote, or no-sandbox instead.
+   */
+  public static final String LOCAL = "local";
+
+  /**
+   * Disables local and remote caching for a spawn, but note that the local action cache may still
+   * apply.
+   *
+   * <p>This tag can also be set on an action, in which case it completely disables all caching for
+   * that action, but note that action-generated spawns may still be cached, unless they also carry
+   * this tag.
    */
   public static final String NO_CACHE = "no-cache";
+
+  /** Disables local sandboxing of a spawn. */
+  public static final String LEGACY_NOSANDBOX = "nosandbox";
+
+  /** Disables local sandboxing of a spawn. */
+  public static final String NO_SANDBOX = "no-sandbox";
+
+  /** Disables remote execution of a spawn. */
+  public static final String NO_REMOTE = "no-remote";
+
+  /**
+   * Disables networking for a spawn if possible (only if sandboxing is enabled and if the sandbox
+   * supports it).
+   */
+  public static final String BLOCK_NETWORK = "block-network";
+
+  /**
+   * On linux, if sandboxing is enabled, ensures that a spawn is run with uid 0, i.e., root. Has no
+   * effect otherwise.
+   */
+  public static final String REQUIRES_FAKEROOT = "requires-fakeroot";
 }
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
index 564460e726..3857783cb5 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java
@@ -81,16 +81,6 @@ public final class SimpleSpawn implements Spawn {
         localResources);
   }
 
-  @Override
-  public boolean hasNoSandbox() {
-    return executionInfo.containsKey("nosandbox");
-  }
-
-  @Override
-  public boolean isRemotable() {
-    return !executionInfo.containsKey("local");
-  }
-
   @Override
   public final ImmutableMap<String, String> getExecutionInfo() {
     return executionInfo;
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
index 6963b016d9..3a59d9ffb3 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java
@@ -25,17 +25,6 @@ import java.util.Collection;
  * of files it is expected to read and write.
  */
 public interface Spawn {
-
-  /**
-   * Returns true iff this command may be executed remotely.
-   */
-  boolean isRemotable();
-
-  /**
-   * Returns true iff this command should be executed without a sandbox.
-   */
-  boolean hasNoSandbox();
-
   /**
    * Out-of-band data for this spawn. This can be used to signal hints (hardware requirements,
    * local vs. remote) to the execution subsystem.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java
index e1156e9745..4448ea8f7b 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java
@@ -32,6 +32,21 @@ public final class Spawns {
     return !spawn.getExecutionInfo().containsKey(ExecutionRequirements.NO_CACHE);
   }
 
+  public static boolean mayBeSandboxed(Spawn spawn) {
+    return !spawn.getExecutionInfo().containsKey(ExecutionRequirements.LEGACY_NOSANDBOX)
+        && !spawn.getExecutionInfo().containsKey(ExecutionRequirements.NO_SANDBOX)
+        && !spawn.getExecutionInfo().containsKey(ExecutionRequirements.LOCAL);
+  }
+
+  public static boolean requiresNetwork(Spawn spawn) {
+    return !spawn.getExecutionInfo().containsKey(ExecutionRequirements.BLOCK_NETWORK);
+  }
+
+  public static boolean mayBeExecutedRemotely(Spawn spawn) {
+    return !spawn.getExecutionInfo().containsKey(ExecutionRequirements.LOCAL)
+        && !spawn.getExecutionInfo().containsKey(ExecutionRequirements.NO_REMOTE);
+  }
+
   /**
    * Parse the timeout key in the spawn execution info, if it exists. Otherwise, return -1.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BinTools.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BinTools.java
index deb987b771..dc52388d83 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BinTools.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BinTools.java
@@ -33,15 +33,22 @@ import java.io.IOException;
  * using relative paths from the execution root.
  */
 public final class BinTools {
-  private final BlazeDirectories directories;
+  private final Path embeddedBinariesRoot;
   private final Path execrootParent;
   private final ImmutableList<String> embeddedTools;
 
   private Path binDir;  // the working bin directory under execRoot
 
   private BinTools(BlazeDirectories directories, ImmutableList<String> tools) {
-    this.directories = directories;
-    this.execrootParent = directories.getExecRoot().getParentDirectory();
+    this(
+        directories.getEmbeddedBinariesRoot(),
+        directories.getExecRoot().getParentDirectory(),
+        tools);
+  }
+
+  private BinTools(Path embeddedBinariesRoot, Path execrootParent, ImmutableList<String> tools) {
+    this.embeddedBinariesRoot = embeddedBinariesRoot;
+    this.execrootParent = execrootParent;
     ImmutableList.Builder<String> builder = ImmutableList.builder();
     // Files under embedded_tools shouldn't be copied to under _bin dir
     // They won't be used during action execution time.
@@ -69,8 +76,8 @@ public final class BinTools {
    */
   @VisibleForTesting
   public static BinTools empty(BlazeDirectories directories) {
-    return new BinTools(directories, ImmutableList.<String>of()).setBinDir(
-        directories.getWorkspace().getBaseName());
+    return new BinTools(directories, ImmutableList.<String>of())
+        .setBinDir(directories.getWorkspace().getBaseName());
   }
 
   /**
@@ -80,8 +87,21 @@ public final class BinTools {
    */
   @VisibleForTesting
   public static BinTools forUnitTesting(BlazeDirectories directories, Iterable<String> tools) {
-    return new BinTools(directories, ImmutableList.copyOf(tools)).setBinDir(
-        directories.getWorkspace().getBaseName());
+    return new BinTools(directories, ImmutableList.copyOf(tools))
+        .setBinDir(directories.getWorkspace().getBaseName());
+  }
+
+  /**
+   * Creates an instance for testing without actually symlinking the tools.
+   *
+   * <p>Used for tests that need a set of embedded tools to be present, but not the actual files.
+   */
+  @VisibleForTesting
+  public static BinTools forUnitTesting(Path execroot, Iterable<String> tools) {
+    return new BinTools(
+        execroot.getRelative("/fake/embedded/tools"),
+        execroot.getParentDirectory(),
+        ImmutableList.copyOf(tools)).setBinDir(execroot.getBaseName());
   }
 
   /**
@@ -168,7 +188,7 @@ public final class BinTools {
 
   private void setupTool(String embeddedPath) throws ExecException {
     Preconditions.checkNotNull(binDir);
-    Path sourcePath = directories.getEmbeddedBinariesRoot().getRelative(embeddedPath);
+    Path sourcePath = embeddedBinariesRoot.getRelative(embeddedPath);
     Path linkPath = binDir.getRelative(PathFragment.create(embeddedPath).getBaseName());
     linkTool(sourcePath, linkPath);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
index b5f4b92024..b9a055b238 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
@@ -18,11 +18,15 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
-import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
-import com.google.devtools.build.lib.actions.BaseSpawn;
+import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
+import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.ResourceSet;
+import com.google.devtools.build.lib.actions.SimpleSpawn;
+import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.analysis.config.BinTools;
@@ -37,18 +41,17 @@ import java.io.IOException;
 import java.util.List;
 
 /**
- * Helper class responsible for the symlink tree creation.
- * Used to generate runfiles and fileset symlink farms.
+ * Helper class responsible for the symlink tree creation. Used to generate runfiles and fileset
+ * symlink farms.
  */
 public final class SymlinkTreeHelper {
   @VisibleForTesting
   public static final String BUILD_RUNFILES = "build-runfiles" + OsUtils.executableExtension();
 
   /**
-   * These actions run faster overall when serialized, because most of their
-   * cost is in the ext2 block allocator, and there's less seeking required if
-   * their directory creations get non-interleaved allocations. So we give them
-   * a huge resource cost.
+   * These actions run faster overall when serialized, because most of their cost is in the ext2
+   * block allocator, and there's less seeking required if their directory creations get
+   * non-interleaved allocations. So we give them a huge resource cost.
    */
   public static final ResourceSet RESOURCE_SET = ResourceSet.createWithRamCpuIo(1000, 0.5, 0.75);
 
@@ -57,16 +60,14 @@ public final class SymlinkTreeHelper {
   private final boolean filesetTree;
 
   /**
-   * Creates SymlinkTreeHelper instance. Can be used independently of
-   * SymlinkTreeAction.
+   * Creates SymlinkTreeHelper instance. Can be used independently of SymlinkTreeAction.
    *
    * @param inputManifest exec path to the input runfiles manifest
    * @param symlinkTreeRoot the root of the symlink tree to be created
    * @param filesetTree true if this is fileset symlink tree,
    *                    false if this is a runfiles symlink tree.
    */
-  public SymlinkTreeHelper(Path inputManifest, Path symlinkTreeRoot,
-      boolean filesetTree) {
+  public SymlinkTreeHelper(Path inputManifest, Path symlinkTreeRoot, boolean filesetTree) {
     this.inputManifest = inputManifest;
     this.symlinkTreeRoot = symlinkTreeRoot;
     this.filesetTree = filesetTree;
@@ -77,20 +78,19 @@ public final class SymlinkTreeHelper {
   }
 
   /**
-   * Creates a symlink tree using a CommandBuilder. This means that the symlink
-   * tree will always be present on the developer's workstation. Useful when
-   * running commands locally.
+   * Creates a symlink tree using a CommandBuilder. This means that the symlink tree will always be
+   * present on the developer's workstation. Useful when running commands locally.
    *
-   * Warning: this method REALLY executes the command on the box Blaze was
-   * run on, without any kind of synchronization, locking, or anything else.
+   * Warning: this method REALLY executes the command on the box Bazel is running on, without any
+   * kind of synchronization, locking, or anything else.
    *
    * @param config the configuration that is used for creating the symlink tree.
    * @throws CommandException
    */
-  public void createSymlinksUsingCommand(Path execRoot,
-      BuildConfiguration config, BinTools binTools) throws CommandException {
+  public void createSymlinksUsingCommand(
+      Path execRoot, BuildConfiguration config, BinTools binTools)
+          throws CommandException {
     List<String> argv = getSpawnArgumentList(execRoot, binTools);
-
     CommandBuilder builder = new CommandBuilder();
     builder.addArgs(argv);
     builder.setWorkingDir(execRoot);
@@ -101,30 +101,30 @@ public final class SymlinkTreeHelper {
    * Creates symlink tree using appropriate method. At this time tree always created using
    * build-runfiles helper application.
    *
-   * <p>Note: method may try to acquire resources - meaning that it would block for undetermined
-   * period of time. If it is interrupted during that wait, ExecException will be thrown but
-   * interrupted bit will be preserved.
-   *
-   * @param action action instance that requested symlink tree creation
+   * @param owner action instance that requested symlink tree creation
    * @param actionExecutionContext Services that are in the scope of the action.
    * @param enableRunfiles
    * @return a list of SpawnResults created during symlink creation, if any
    */
   public List<SpawnResult> createSymlinks(
-      AbstractAction action,
+      ActionExecutionMetadata owner,
       ActionExecutionContext actionExecutionContext,
       BinTools binTools,
       ImmutableMap<String, String> shellEnvironment,
+      Artifact inputManifestArtifact,
       boolean enableRunfiles)
-      throws ExecException, InterruptedException {
+          throws ExecException, InterruptedException {
+    Preconditions.checkState(inputManifestArtifact.getPath().equals(inputManifest));
     if (enableRunfiles) {
-      List<String> args =
-          getSpawnArgumentList(
-              actionExecutionContext.getExecRoot(), binTools);
       return actionExecutionContext
-          .getSpawnActionContext(action.getMnemonic())
+          .getSpawnActionContext(owner.getMnemonic())
           .exec(
-              new BaseSpawn.Local(args, shellEnvironment, action, RESOURCE_SET),
+              createSpawn(
+                  owner,
+                  actionExecutionContext.getExecRoot(),
+                  binTools,
+                  shellEnvironment,
+                  inputManifestArtifact),
               actionExecutionContext);
     } else {
       // Pretend we created the runfiles tree by copying the manifest
@@ -138,10 +138,30 @@ public final class SymlinkTreeHelper {
     }
   }
 
+  @VisibleForTesting
+  Spawn createSpawn(
+      ActionExecutionMetadata owner,
+      Path execRoot,
+      BinTools binTools,
+      ImmutableMap<String, String> environment,
+      ActionInput inputManifestArtifact) {
+    return new SimpleSpawn(
+        owner,
+        getSpawnArgumentList(execRoot, binTools),
+        environment,
+        ImmutableMap.of(
+            ExecutionRequirements.LOCAL, "",
+            ExecutionRequirements.NO_CACHE, "",
+            ExecutionRequirements.NO_SANDBOX, ""),
+        ImmutableList.of(inputManifestArtifact),
+        /*outputs=*/ ImmutableList.of(),
+        RESOURCE_SET);
+  }
+
   /**
    * Returns the complete argument list build-runfiles has to be called with.
    */
-  private List<String> getSpawnArgumentList(Path execRoot, BinTools binTools) {
+  private ImmutableList<String> getSpawnArgumentList(Path execRoot, BinTools binTools) {
     PathFragment path = binTools.getExecPath(BUILD_RUNFILES);
     Preconditions.checkNotNull(path, BUILD_RUNFILES + " not found in embedded tools");
 
@@ -156,6 +176,6 @@ public final class SymlinkTreeHelper {
     args.add(inputManifest.relativeTo(execRoot).getPathString());
     args.add(symlinkTreeRoot.relativeTo(execRoot).getPathString());
 
-    return args;
+    return ImmutableList.copyOf(args);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
index a0f87d4da0..14802acd99 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
@@ -55,18 +55,25 @@ public final class SymlinkTreeStrategy implements SymlinkTreeActionContext {
         AutoProfiler.logged(
             "running " + action.prettyPrint(), logger, /*minTimeForLoggingInMilliseconds=*/ 100)) {
       try {
-        SymlinkTreeHelper helper = new SymlinkTreeHelper(
-            action.getInputManifest().getPath(),
-            action.getOutputManifest().getPath().getParentDirectory(), action.isFilesetTree());
         if (outputService != null && outputService.canCreateSymlinkTree()) {
-          outputService.createSymlinkTree(action.getInputManifest().getPath(),
+          outputService.createSymlinkTree(
+              action.getInputManifest().getPath(),
               action.getOutputManifest().getPath(),
               action.isFilesetTree(),
               action.getOutputManifest().getExecPath().getParentDirectory());
           return ImmutableList.of();
         } else {
+          SymlinkTreeHelper helper = new SymlinkTreeHelper(
+              action.getInputManifest().getPath(),
+              action.getOutputManifest().getPath().getParentDirectory(),
+              action.isFilesetTree());
           return helper.createSymlinks(
-              action, actionExecutionContext, binTools, shellEnvironment, enableRunfiles);
+              action,
+              actionExecutionContext,
+              binTools,
+              shellEnvironment,
+              action.getInputManifest(),
+              enableRunfiles);
         }
       } catch (ExecException e) {
         throw e.toActionExecutionException(
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
index 85d1d1d60d..e251cd60a0 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
@@ -410,7 +410,12 @@ public abstract class TestStrategy implements TestActionContext {
 
     new SymlinkTreeHelper(execSettings.getInputManifest().getPath(), runfilesDir, false)
         .createSymlinks(
-            testAction, actionExecutionContext, binTools, shellEnvironment, enableRunfiles);
+            testAction,
+            actionExecutionContext,
+            binTools,
+            shellEnvironment,
+            execSettings.getInputManifest(),
+            enableRunfiles);
 
     actionExecutionContext.getEventHandler()
         .handle(Event.progress(testAction.getProgressMessage()));
diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java
index 6ec3340465..702b9d6902 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java
@@ -203,16 +203,23 @@ public final class TargetUtils {
   }
 
   /**
-   * Returns the execution info. These include execution requirement
-   * tags ('requires-*' as well as "local") as keys with empty values.
+   * Returns the execution info. These include execution requirement tags ('block-*', 'requires-*',
+   * 'no-*', 'supports-*', 'disable-*', 'local', and 'cpu:*') as keys with empty values.
    */
   public static Map<String, String> getExecutionInfo(Rule rule) {
     // tags may contain duplicate values.
     Map<String, String> map = new HashMap<>();
     for (String tag :
         NonconfigurableAttributeMapper.of(rule).get(CONSTRAINTS_ATTR, Type.STRING_LIST)) {
+      // We don't want to pollute the execution info with random things, and we also need to reserve
+      // some internal tags that we don't allow to be set on targets. We also don't want to
+      // exhaustively enumerate all the legal values here. Right now, only a ~small set of tags is
+      // recognized by Bazel.
       if (tag.startsWith("block-")
           || tag.startsWith("requires-")
+          || tag.startsWith("no-")
+          || tag.startsWith("supports-")
+          || tag.startsWith("disable-")
           || tag.equals("local")
           || tag.startsWith("cpu:")) {
         map.put(tag, "");
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
index 8dae278958..6c4b0a8eda 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -108,7 +108,7 @@ class RemoteSpawnRunner implements SpawnRunner {
   @Override
   public SpawnResult exec(Spawn spawn, SpawnExecutionPolicy policy)
       throws ExecException, InterruptedException, IOException {
-    if (!spawn.isRemotable() || remoteCache == null) {
+    if (!Spawns.mayBeExecutedRemotely(spawn) || remoteCache == null) {
       return fallbackRunner.exec(spawn, policy);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
index 1f6af91075..97fc677503 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.SpawnResult;
+import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.exec.apple.XCodeLocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
@@ -197,7 +198,7 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
     Map<String, String> environment =
         localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir, productName);
 
-    boolean allowNetworkForThisSpawn = allowNetwork || SandboxHelpers.shouldAllowNetwork(spawn);
+    boolean allowNetworkForThisSpawn = allowNetwork || Spawns.requiresNetwork(spawn);
     SandboxedSpawn sandbox = new SymlinkedSandboxedSpawn(
         sandboxPath,
         sandboxExecRoot,
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
index aee389713a..a16497febc 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
@@ -19,8 +19,10 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
+import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
@@ -134,8 +136,8 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
             writableDirs,
             getTmpfsPaths(),
             getReadOnlyBindMounts(blazeDirs, sandboxExecRoot),
-            allowNetwork || SandboxHelpers.shouldAllowNetwork(spawn),
-            spawn.getExecutionInfo().containsKey("requires-fakeroot"));
+            allowNetwork || Spawns.requiresNetwork(spawn),
+            spawn.getExecutionInfo().containsKey(ExecutionRequirements.REQUIRES_FAKEROOT));
     Map<String, String> environment =
         localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir, productName);
 
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
index 53173e4bea..ee496e8e29 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
+import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.exec.ActionContextProvider;
 import com.google.devtools.build.lib.exec.SpawnRunner;
 import com.google.devtools.build.lib.exec.apple.XCodeLocalEnvProvider;
@@ -121,7 +122,7 @@ final class SandboxActionContextProvider extends ActionContextProvider {
     @Override
     public SpawnResult exec(Spawn spawn, SpawnExecutionPolicy policy)
         throws InterruptedException, IOException, ExecException {
-      if (!spawn.isRemotable() || spawn.hasNoSandbox()) {
+      if (!Spawns.mayBeSandboxed(spawn)) {
         return fallbackSpawnRunner.exec(spawn, policy);
       } else {
         return sandboxSpawnRunner.exec(spawn, policy);
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
index 94d1c1111c..58906fddf2 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration;
 import com.google.devtools.build.lib.exec.SpawnInputExpander;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
@@ -115,7 +116,7 @@ public final class SandboxHelpers {
 
   /**
    * Returns true if the build options are set in a way that requires network access for all
-   * actions. This is separate from {@link #shouldAllowNetwork(Spawn)} to avoid having to keep a
+   * actions. This is separate from {@link Spawns#requiresNetwork} to avoid having to keep a
    * reference to the full set of build options (and also for performance, since this only needs to
    * be checked once-per-build).
    */
@@ -128,15 +129,4 @@ public final class SandboxHelpers {
         .testArguments
         .contains("--wrapper_script_flag=--debug");
   }
-
-  /** Returns true if this specific spawn requires network access. */
-  static boolean shouldAllowNetwork(Spawn spawn) {
-    // If the Spawn requests to block network access, do so.
-    if (spawn.getExecutionInfo().containsKey("block-network")) {
-      return false;
-    }
-
-    // Network access is allowed by default.
-    return true;
-  }
 }
-- 
cgit v1.2.3