Skip to content

Commit

Permalink
SpawnAction.setShellCommand: expect shell path
Browse files Browse the repository at this point in the history
SpawnAction.setShellCommand(String) now expects
the shell interpreter's path as an argument. This
change enables two things:
- rules can report an error if the shell is
  missing
- SpawnAction no longer has to know about a
  default shell

The new ShToolchain class will later also be
responsible for retrieving the active shell
toolchain (added in 81ed3ad).

This change brings Bazel closer to not depend on
the shell unless it has to (e.g. to run shell
scripts).

See #4319

RELNOTES: none
PiperOrigin-RevId: 193885943
  • Loading branch information
laszlocsomor authored and Copybara-Service committed Apr 23, 2018
1 parent d91974e commit 84d3097
Show file tree
Hide file tree
Showing 12 changed files with 155 additions and 86 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,6 @@ private PathFragment shellPath(Map<String, String> executionInfo) {
// Use vanilla /bin/bash for actions running on mac machines.
return executionInfo.containsKey(ExecutionRequirements.REQUIRES_DARWIN)
? PathFragment.create("/bin/bash")
: ruleContext.getConfiguration().getFragment(ShellConfiguration.class).getShellExecutable();
: ShToolchain.getPathOrError(ruleContext);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright 2018 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.analysis;

import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.vfs.PathFragment;

/** Class to work with the shell toolchain, e.g. get the shell interpreter's path. */
public final class ShToolchain {

/**
* Returns the shell executable's path, or an empty path if not set.
*
* <p>This method checks the configuration's {@link ShellConfiguration} fragment.
*/
public static PathFragment getPath(BuildConfiguration config) {
PathFragment result = PathFragment.EMPTY_FRAGMENT;

ShellConfiguration configFragment =
(ShellConfiguration) config.getFragment(ShellConfiguration.class);
if (configFragment != null) {
PathFragment path = configFragment.getShellExecutable();
if (path != null) {
result = path;
}
}

return result;
}

/**
* Returns the shell executable's path, or reports a rule error if the path is empty.
*
* <p>This method checks the rule's configuration's {@link ShellConfiguration} fragment for the
* shell executable's path. If null or empty, the method reports an error against the rule.
*/
public static PathFragment getPathOrError(RuleContext ctx) {
PathFragment result = getPath(ctx.getConfiguration());

if (result.isEmpty()) {
ctx.ruleError(
"This rule needs a shell interpreter. Use the --shell_executable=<path> flag to specify"
+ " the interpreter's path, e.g. --shell_executable=/usr/local/bin/bash");
}

return result;
}

private ShToolchain() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@
import com.google.devtools.build.lib.actions.extra.SpawnInfo;
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.analysis.ShellConfiguration;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand Down Expand Up @@ -680,8 +679,7 @@ private CommandLine buildCommandLine(
AnalysisEnvironment analysisEnvironment,
BuildConfiguration configuration,
List<Action> paramFileActions) {
ImmutableList<String> executableArgs = buildExecutableArgs(
configuration.getFragment(ShellConfiguration.class).getShellExecutable());
ImmutableList<String> executableArgs = buildExecutableArgs();
boolean hasConditionalParamFile =
commandLines.stream().anyMatch(c -> c.paramFileInfo != null && !c.paramFileInfo.always());
boolean spillToParamFiles = false;
Expand Down Expand Up @@ -812,7 +810,7 @@ SpawnAction buildSpawnAction(
*/
CommandLine buildCommandLineWithoutParamsFiles() {
SpawnActionCommandLine.Builder result = new SpawnActionCommandLine.Builder();
ImmutableList<String> executableArgs = buildExecutableArgs(null);
ImmutableList<String> executableArgs = buildExecutableArgs();
result.addExecutableArguments(executableArgs);
for (CommandLineAndParamFileInfo commandLineAndParamFileInfo : commandLines) {
result.addCommandLine(commandLineAndParamFileInfo.commandLine);
Expand Down Expand Up @@ -851,12 +849,7 @@ protected SpawnAction createSpawnAction(
extraActionInfoSupplier);
}

private ImmutableList<String> buildExecutableArgs(
@Nullable PathFragment defaultShellExecutable) {
if (isShellCommand && executable == null) {
Preconditions.checkNotNull(defaultShellExecutable);
executable = defaultShellExecutable;
}
private ImmutableList<String> buildExecutableArgs() {
Preconditions.checkNotNull(executable);
Preconditions.checkNotNull(executableArgs);

Expand Down Expand Up @@ -1119,18 +1112,16 @@ public Builder setJarExecutable(PathFragment javaExecutable,
}

/**
* Sets the executable to be the shell and adds the given command as the
* command to be executed.
* Sets the executable to be the shell and adds the given command as the command to be executed.
*
* <p>Note that this will not clear the arguments, so any arguments will
* be passed in addition to the command given here.
* <p>Note that this will not clear the arguments, so any arguments will be passed in addition
* to the command given here.
*
* <p>Calling this method overrides any previous values set via calls to
* {@link #setExecutable(Artifact)}, {@link #setJavaExecutable}, or
* {@link #setShellCommand(String)}.
* <p>Calling this method overrides any previous values set via calls to {@link
* #setExecutable(Artifact)}, {@link #setJavaExecutable}, or {@link #setShellCommand(String)}.
*/
public Builder setShellCommand(String command) {
this.executable = null;
public Builder setShellCommand(PathFragment shExecutable, String command) {
this.executable = shExecutable;
// 0=shell command switch, 1=command
this.executableArgs = Lists.newArrayList("-c", command);
this.isShellCommand = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.analysis.PseudoAction;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.ShToolchain;
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction;
Expand Down Expand Up @@ -654,10 +655,11 @@ public void runShell(
Artifact helperScript =
CommandHelper.shellCommandHelperScriptMaybe(
ruleContext, command, helperScriptSuffix, executionInfo);
PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
if (helperScript == null) {
builder.setShellCommand(command);
builder.setShellCommand(shExecutable, command);
} else {
builder.setShellCommand(helperScript.getExecPathString());
builder.setShellCommand(shExecutable, helperScript.getExecPathString());
builder.addInput(helperScript);
FilesToRunProvider provider = context.getExecutableRunfiles(helperScript);
if (provider != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.RunfilesSupport;
import com.google.devtools.build.lib.analysis.ShToolchain;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
Expand Down Expand Up @@ -306,14 +307,25 @@ private TestParams createTestAction(int shards) {
coverageArtifacts.add(coverageArtifact);
}

env.registerAction(new TestRunnerAction(
ruleContext.getActionOwner(), inputs,
testSetupScript, collectCoverageScript,
testLog, cacheStatus,
coverageArtifact,
testProperties, extraTestEnv, executionSettings,
shard, run, config, ruleContext.getWorkspaceName(),
useTestRunner));
PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
env.registerAction(
new TestRunnerAction(
ruleContext.getActionOwner(),
inputs,
testSetupScript,
collectCoverageScript,
testLog,
cacheStatus,
coverageArtifact,
testProperties,
extraTestEnv,
executionSettings,
shard,
run,
config,
ruleContext.getWorkspaceName(),
shExecutable,
useTestRunner));
results.add(cacheStatus);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl;
import com.google.devtools.build.lib.analysis.ShellConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.RunUnder;
import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
Expand Down Expand Up @@ -80,6 +79,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
private final Artifact cacheStatus;
private final PathFragment testWarningsPath;
private final PathFragment unusedRunfilesLogPath;
private final PathFragment shExecutable;
private final PathFragment splitLogsPath;
private final PathFragment splitLogsDir;
private final PathFragment undeclaredOutputsDir;
Expand Down Expand Up @@ -125,18 +125,18 @@ private static ImmutableList<Artifact> list(Artifact... artifacts) {
}

/**
* Create new TestRunnerAction instance. Should not be called directly.
* Use {@link TestActionBuilder} instead.
* Create new TestRunnerAction instance. Should not be called directly. Use {@link
* TestActionBuilder} instead.
*
* @param shardNum The shard number. Must be 0 if totalShards == 0
* (no sharding). Otherwise, must be >= 0 and < totalShards.
* @param shardNum The shard number. Must be 0 if totalShards == 0 (no sharding). Otherwise, must
* be >= 0 and < totalShards.
* @param runNumber test run number
*/
TestRunnerAction(
ActionOwner owner,
Iterable<Artifact> inputs,
Artifact testSetupScript, // Must be in inputs
@Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
Artifact testSetupScript, // Must be in inputs
@Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
Artifact testLog,
Artifact cacheStatus,
Artifact coverageArtifact,
Expand All @@ -147,6 +147,7 @@ private static ImmutableList<Artifact> list(Artifact... artifacts) {
int runNumber,
BuildConfiguration configuration,
String workspaceName,
PathFragment shExecutable,
boolean useTestRunner) {
super(
owner,
Expand Down Expand Up @@ -182,6 +183,7 @@ private static ImmutableList<Artifact> list(Artifact... artifacts) {
this.testWarningsPath = baseDir.getChild("test.warnings");
this.unusedRunfilesLogPath = baseDir.getChild("test.unused_runfiles_log");
this.testStderr = baseDir.getChild("test.err");
this.shExecutable = shExecutable;
this.splitLogsDir = baseDir.getChild("test.raw_splitlogs");
// See note in {@link #getSplitLogsPath} on the choice of file name.
this.splitLogsPath = splitLogsDir.getChild("test.splitlogs");
Expand Down Expand Up @@ -756,7 +758,7 @@ public Artifact getTestSetupScript() {
}

public PathFragment getShExecutable() {
return configuration.getFragment(ShellConfiguration.class).getShellExecutable();
return shExecutable;
}

public ImmutableMap<String, String> getLocalShellEnvironment() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.ShellConfiguration;
import com.google.devtools.build.lib.analysis.ShToolchain;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction;
Expand Down Expand Up @@ -395,25 +395,17 @@ public Artifact createStubAction(
if (OS.getCurrent() == OS.WINDOWS) {
Artifact newExecutable =
ruleContext.getImplicitOutputArtifact(ruleContext.getTarget().getName() + ".cmd");
PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
ruleContext.registerAction(
new TemplateExpansionAction(
ruleContext.getActionOwner(),
newExecutable,
STUB_SCRIPT_WINDOWS,
ImmutableList.of(
Substitution.of(
"%bash_exe_path%",
ruleContext
.getFragment(ShellConfiguration.class)
.getShellExecutable()
.getPathString()),
Substitution.of("%bash_exe_path%", shExecutable.getPathString()),
Substitution.of(
"%cygpath_exe_path%",
ruleContext
.getFragment(ShellConfiguration.class)
.getShellExecutable()
.replaceName("cygpath.exe")
.getPathString())),
shExecutable.replaceName("cygpath.exe").getPathString())),
true));
return newExecutable;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.devtools.build.lib.analysis.Runfiles.Builder;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.RunfilesSupport;
import com.google.devtools.build.lib.analysis.ShToolchain;
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction;
import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction.LaunchInfo;
Expand Down Expand Up @@ -169,11 +170,13 @@ public Artifact createExecutable(
true));

if (OS.getCurrent() != OS.WINDOWS) {
PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
ruleContext.registerAction(
new SpawnAction.Builder()
.addInput(zipFile)
.addOutput(executable)
.setShellCommand(
shExecutable,
"echo '#!/usr/bin/env python' | cat - "
+ zipFile.getExecPathString()
+ " > "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.RunfilesSupport;
import com.google.devtools.build.lib.analysis.ShellConfiguration;
import com.google.devtools.build.lib.analysis.ShToolchain;
import com.google.devtools.build.lib.analysis.actions.ExecutableSymlinkAction;
import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction;
import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction.LaunchInfo;
Expand All @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.PathFragment;

/**
* Implementation for the sh_binary rule.
Expand Down Expand Up @@ -101,21 +102,16 @@ private static boolean isWindowsExecutable(Artifact artifact) {
|| artifact.getExtension().equals("bat");
}

private static Artifact createWindowsExeLauncher(RuleContext ruleContext)
throws RuleErrorException {
private static Artifact createWindowsExeLauncher(
RuleContext ruleContext, PathFragment shExecutable) throws RuleErrorException {
Artifact bashLauncher =
ruleContext.getImplicitOutputArtifact(ruleContext.getTarget().getName() + ".exe");

LaunchInfo launchInfo =
LaunchInfo.builder()
.addKeyValuePair("binary_type", "Bash")
.addKeyValuePair("workspace_name", ruleContext.getWorkspaceName())
.addKeyValuePair(
"bash_bin_path",
ruleContext
.getFragment(ShellConfiguration.class)
.getShellExecutable()
.getPathString())
.addKeyValuePair("bash_bin_path", shExecutable.getPathString())
.build();

LauncherFileWriteAction.createAndRegister(ruleContext, bashLauncher, launchInfo);
Expand All @@ -138,8 +134,9 @@ private static Artifact launcherForWindows(
}
}

PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
if (ruleContext.getConfiguration().enableWindowsExeLauncher()) {
return createWindowsExeLauncher(ruleContext);
return createWindowsExeLauncher(ruleContext, shExecutable);
}

Artifact wrapper =
Expand All @@ -149,13 +146,7 @@ private static Artifact launcherForWindows(
ruleContext.getActionOwner(),
wrapper,
STUB_SCRIPT_WINDOWS,
ImmutableList.of(
Substitution.of(
"%bash_exe_path%",
ruleContext
.getFragment(ShellConfiguration.class)
.getShellExecutable()
.getPathString())),
ImmutableList.of(Substitution.of("%bash_exe_path%", shExecutable.getPathString())),
true));
return wrapper;
}
Expand Down
Loading

0 comments on commit 84d3097

Please sign in to comment.