diff options
author | ulfjack <ulfjack@google.com> | 2017-11-29 03:37:04 -0800 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2017-11-29 03:38:22 -0800 |
commit | 4d7f8f7846960ffc111cf1aef2a5efb094114442 (patch) | |
tree | d91dbe1ea83226eadb96eda6562c9a70fa0cd18d /src | |
parent | 91420101cf977990166acec2a0e46e1787389512 (diff) |
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
Diffstat (limited to 'src')
20 files changed, 286 insertions, 145 deletions
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 @@ -69,16 +69,6 @@ public class BaseSpawn implements Spawn { } @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 @@ -36,16 +36,6 @@ public class DelegateSpawn implements Spawn { } @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 @@ -82,16 +82,6 @@ public final class SimpleSpawn implements Spawn { } @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; - } } diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java new file mode 100644 index 0000000000..3baee79ccd --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java @@ -0,0 +1,67 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.exec; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.ActionExecutionMetadata; +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; +import com.google.devtools.build.lib.actions.ExecutionRequirements; +import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.analysis.config.BinTools; +import com.google.devtools.build.lib.exec.util.FakeOwner; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link SymlinkTreeHelper}. */ +@RunWith(JUnit4.class) +public final class SymlinkTreeHelperTest { + private final FileSystem fs = new InMemoryFileSystem(); + + @Test + public void checkCreatedSpawn() { + ActionExecutionMetadata owner = new FakeOwner("SymlinkTree", "Creating it"); + Path execRoot = fs.getPath("/my/workspace"); + Path inputManifestPath = execRoot.getRelative("input_manifest"); + ActionInput inputManifest = ActionInputHelper.fromPath(inputManifestPath.asFragment()); + Spawn spawn = + new SymlinkTreeHelper( + inputManifestPath, + fs.getPath("/my/workspace/output/MANIFEST"), + false) + .createSpawn( + owner, + execRoot, + BinTools.forUnitTesting(execRoot, ImmutableList.of(SymlinkTreeHelper.BUILD_RUNFILES)), + ImmutableMap.of(), + inputManifest); + assertThat(spawn.getResourceOwner()).isSameAs(owner); + assertThat(spawn.getEnvironment()).isEmpty(); + assertThat(spawn.getExecutionInfo()).containsExactly( + ExecutionRequirements.LOCAL, "", + ExecutionRequirements.NO_CACHE, "", + ExecutionRequirements.NO_SANDBOX, ""); + assertThat(spawn.getInputFiles()).containsExactly(inputManifest); + // At this time, the spawn does not declare any output files. + assertThat(spawn.getOutputFiles()).isEmpty(); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java b/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java index 6d49de9024..eb49366cf2 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.base.Predicate; import com.google.common.collect.Lists; import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase; +import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -75,4 +76,24 @@ public class TargetUtilsTest extends PackageLoadingTestCase { assertThat(tagFilter.apply(tag2)).isTrue(); assertThat(tagFilter.apply(tag1b)).isFalse(); } + + @Test + public void testExecutionInfo() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'tag1', srcs=['sh.sh'], tags=['supports-workers', 'no-cache'])", + "sh_binary(name = 'tag2', srcs=['sh.sh'], tags=['disable-local-prefetch'])", + "sh_binary(name = 'tag1b', srcs=['sh.sh'], tags=['local', 'cpu:4'])"); + + Rule tag1 = (Rule) getTarget("//tests:tag1"); + Rule tag2 = (Rule) getTarget("//tests:tag2"); + Rule tag1b = (Rule) getTarget("//tests:tag1b"); + + Map<String, String> execInfo = TargetUtils.getExecutionInfo(tag1); + assertThat(execInfo).containsExactly("supports-workers", "", "no-cache", ""); + execInfo = TargetUtils.getExecutionInfo(tag2); + assertThat(execInfo).containsExactly("disable-local-prefetch", ""); + execInfo = TargetUtils.getExecutionInfo(tag1b); + assertThat(execInfo).containsExactly("local", "", "cpu:4", ""); + } } diff --git a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java index 342680b700..d2497ff313 100644 --- a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java @@ -26,10 +26,10 @@ import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionInputPrefetcher; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; -import com.google.devtools.build.lib.actions.BaseSpawn; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ResourceManager; 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.SpawnActionContext; import com.google.devtools.build.lib.actions.SpawnResult; @@ -58,7 +58,6 @@ import com.google.devtools.build.lib.vfs.util.FileSystems; import com.google.devtools.common.options.Options; import com.google.devtools.common.options.OptionsParser; import java.io.IOException; -import java.util.Arrays; import java.util.Collection; import java.util.List; import org.junit.Before; @@ -148,10 +147,13 @@ public class StandaloneSpawnStrategyTest { } private Spawn createSpawn(String... arguments) { - return new BaseSpawn.Local( - Arrays.asList(arguments), - ImmutableMap.<String, String>of(), + return new SimpleSpawn( new ActionsTestUtil.NullAction(), + ImmutableList.copyOf(arguments), + /*environment=*/ ImmutableMap.of(), + /*executionInfo=*/ ImmutableMap.of(), + /*inputs=*/ ImmutableList.of(), + /*outputs=*/ ImmutableList.of(), ResourceSet.ZERO); } @@ -230,10 +232,13 @@ public class StandaloneSpawnStrategyTest { // down where that env var is coming from. return; } - Spawn spawn = new BaseSpawn.Local( - Arrays.asList("/usr/bin/env"), - ImmutableMap.of("foo", "bar", "baz", "boo"), + Spawn spawn = new SimpleSpawn( new ActionsTestUtil.NullAction(), + ImmutableList.of("/usr/bin/env"), + /*environment=*/ ImmutableMap.of("foo", "bar", "baz", "boo"), + /*executionInfo=*/ ImmutableMap.of(), + /*inputs=*/ ImmutableList.of(), + /*outputs=*/ ImmutableList.of(), ResourceSet.ZERO); run(spawn); assertThat(Sets.newHashSet(out().split("\n"))).isEqualTo(Sets.newHashSet("foo=bar", "baz=boo")); |