Skip to content

Commit

Permalink
bazel run: no longer requires Bash for simple case
Browse files Browse the repository at this point in the history 
Windows: add --incompatible_windows_bashless_run_command

This flag enables Bash-less "bazel run" for simple
cases (no `--run_under`, no `--script_path`).

When enabled, "bazel run //foo:bin" no longer
requires Bash, and works with
`--shell_executable=""`.

Bash is still required with `--run_under="foo"`
and with `--script_path=<path>`.

Fixes #8229
See #8240

Closes #8241.

PiperOrigin-RevId: 247183428
  • Loading branch information
@laszlocsomor @copybara-github
laszlocsomor authored and copybara-github committed May 8, 2019
1 parent 3d88d75 commit e8af81c
Show file tree
Hide file tree
Showing 3 changed files with 189 additions and 36 deletions.
105 changes: 83 additions & 22 deletions src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.runtime.commands;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -64,6 +65,7 @@
import com.google.devtools.build.lib.server.CommandProtos.EnvironmentVariable;
import com.google.devtools.build.lib.server.CommandProtos.ExecRequest;
import com.google.devtools.build.lib.shell.CommandException;
import com.google.devtools.build.lib.shell.ShellUtils;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.CommandDescriptionForm;
import com.google.devtools.build.lib.util.CommandFailureUtils;
Expand All @@ -79,6 +81,7 @@
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
import com.google.devtools.common.options.OptionMetadataTag;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingResult;
Expand Down Expand Up @@ -122,8 +125,28 @@ public static class RunOptions extends OptionsBase {
+ "and the executable is connected to the terminal's stdin."
)
public PathFragment scriptPath;

@Option(
name = "incompatible_windows_bashless_run_command",
documentationCategory = OptionDocumentationCategory.TESTING,
effectTags = {
OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS,
},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES,
},
defaultValue = "false",
help =
"On Windows: if true, the \"run\" command runs the binary directly instead of running "
+ "through Bash; when false, then the binary is ran through Bash. On other "
+ "platforms: no-op.")
public boolean bashlessRun;
}

// Thrown when a method needs Bash but ShToolchain.getPath yields none.
private static final class NoShellFoundException extends Exception {}

@VisibleForTesting
public static final String SINGLE_TARGET_MESSAGE =
"Only a single target can be run. "
Expand Down Expand Up @@ -182,9 +205,15 @@ private List<String> computeArgs(CommandEnvironment env, ConfiguredTarget target
return args;
}

private void constructCommandLine(List<String> cmdLine, List<String> prettyCmdLine,
CommandEnvironment env, PathFragment shellExecutable, ConfiguredTarget targetToRun,
ConfiguredTarget runUnderTarget, List<String> args) {
private void constructCommandLine(
List<String> cmdLine,
List<String> prettyCmdLine,
CommandEnvironment env,
BuildConfiguration configuration,
ConfiguredTarget targetToRun,
ConfiguredTarget runUnderTarget,
List<String> args)
throws NoShellFoundException {
String productName = env.getRuntime().getProductName();
Artifact executable = targetToRun.getProvider(FilesToRunProvider.class).getExecutable();

Expand Down Expand Up @@ -219,6 +248,12 @@ private void constructCommandLine(List<String> cmdLine, List<String> prettyCmdLi
runUnderValue += " " + ShellEscaper.escapeJoinAll(opts);
}
}

PathFragment shellExecutable = ShToolchain.getPath(configuration);
if (shellExecutable.isEmpty()) {
throw new NoShellFoundException();
}

cmdLine.add(shellExecutable.getPathString());
cmdLine.add("-c");
cmdLine.add(runUnderValue + " " + executablePath.getPathString() + " "
Expand Down Expand Up @@ -350,18 +385,6 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
}
}

// TODO(laszlocsomor): change RunCommand to not require a shell and not write a shell script,
// then remove references to ShToolchain. See https://github.com/bazelbuild/bazel/issues/4319
PathFragment shExecutable = ShToolchain.getPath(configuration);
if (shExecutable.isEmpty()) {
env.getReporter()
.handle(
Event.error(
"the \"run\" command needs a shell; use the --shell_executable=<path> flag "
+ "to specify its path, e.g. --shell_executable=/usr/local/bin/bash"));
return BlazeCommandResult.exitCode(ExitCode.COMMAND_LINE_ERROR);
}

Map<String, String> runEnvironment = new TreeMap<>();
List<String> cmdLine = new ArrayList<>();
List<String> prettyCmdLine = new ArrayList<>();
Expand Down Expand Up @@ -419,8 +442,18 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
} else {
workingDir = runfilesDir;
List<String> args = computeArgs(env, targetToRun, commandLineArgs);
constructCommandLine(
cmdLine, prettyCmdLine, env, shExecutable, targetToRun, runUnderTarget, args);
try {
constructCommandLine(
cmdLine, prettyCmdLine, env, configuration, targetToRun, runUnderTarget, args);
} catch (NoShellFoundException e) {
env.getReporter()
.handle(
Event.error(
"the \"run\" command needs a shell with \"--run_under\"; use the"
+ " --shell_executable=<path> flag to specify its path, e.g."
+ " --shell_executable=/bin/bash"));
return BlazeCommandResult.exitCode(ExitCode.COMMAND_LINE_ERROR);
}
}

if (runOptions.scriptPath != null) {
Expand All @@ -430,6 +463,18 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
cmdLine,
runEnvironment,
workingDir.getPathString());

PathFragment shExecutable = ShToolchain.getPath(configuration);
if (shExecutable.isEmpty()) {
env.getReporter()
.handle(
Event.error(
"the \"run\" command needs a shell with \"--script_path\"; use the"
+ " --shell_executable=<path> flag to specify its path, e.g."
+ " --shell_executable=/bin/bash"));
return BlazeCommandResult.exitCode(ExitCode.COMMAND_LINE_ERROR);
}

if (writeScript(env, shExecutable, runOptions.scriptPath, unisolatedCommand)) {
return BlazeCommandResult.exitCode(ExitCode.SUCCESS);
} else {
Expand Down Expand Up @@ -458,12 +503,28 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
.setWorkingDirectory(
ByteString.copyFrom(workingDir.getPathString(), StandardCharsets.ISO_8859_1));

ImmutableList<String> shellCmdLine =
ImmutableList.<String>of(
shExecutable.getPathString(), "-c", ShellEscaper.escapeJoinAll(cmdLine));
if (OS.getCurrent() == OS.WINDOWS && runOptions.bashlessRun) {
String joinedCommands =
Joiner.on(" ").join(Iterables.transform(cmdLine, x -> ShellUtils.windowsEscapeArg(x)));
execDescription.addArgv(ByteString.copyFrom(joinedCommands, StandardCharsets.ISO_8859_1));
} else {
PathFragment shExecutable = ShToolchain.getPath(configuration);
if (shExecutable.isEmpty()) {
env.getReporter()
.handle(
Event.error(
"the \"run\" command needs a shell with; use the --shell_executable=<path> "
+ "flag to specify the shell's path, e.g. --shell_executable=/bin/bash"));
return BlazeCommandResult.exitCode(ExitCode.COMMAND_LINE_ERROR);
}

ImmutableList<String> shellCmdLine =
ImmutableList.<String>of(
shExecutable.getPathString(), "-c", ShellEscaper.escapeJoinAll(cmdLine));

for (String arg : shellCmdLine) {
execDescription.addArgv(ByteString.copyFrom(arg, StandardCharsets.ISO_8859_1));
for (String arg : shellCmdLine) {
execDescription.addArgv(ByteString.copyFrom(arg, StandardCharsets.ISO_8859_1));
}
}

for (Map.Entry<String, String> variable : runEnvironment.entrySet()) {
Expand Down
68 changes: 68 additions & 0 deletions src/test/py/bazel/first_time_use_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,74 @@ def testNoPythonRequirement(self):
if 'python' in line and 'not found on PATH' in line:
self._FailWithOutput(stdout + stderr)

def testNoBashRequiredForSimpleBazelRun(self):
"""Regression test for https://github.com/bazelbuild/bazel/issues/8229."""
self.ScratchFile('WORKSPACE')
self.ScratchFile('foo/BUILD', [
'py_binary(',
' name = "x",'
' srcs = ["x.py"],',
')',
])
self.ScratchFile('foo/x.py', [
'from __future__ import print_function',
'print("hello python")',
])

if test_base.TestBase.IsWindows():
exit_code, stdout, stderr = self.RunBazel([
'run',
'--shell_executable=',
'--noincompatible_windows_bashless_run_command',
'//foo:x',
])
self.AssertNotExitCode(exit_code, 0, stderr)
found_error = False
for line in stdout + stderr:
if 'ERROR' in line and 'needs a shell' in line:
found_error = True
break
if not found_error:
self._FailWithOutput(stdout + stderr)

exit_code, stdout, stderr = self.RunBazel([
'run',
'--shell_executable=',
'--incompatible_windows_bashless_run_command',
'//foo:x',
])
self.AssertExitCode(exit_code, 0, stderr)
found_output = False
for line in stdout + stderr:
if 'ERROR' in line and 'needs a shell' in line:
self._FailWithOutput(stdout + stderr)
if 'hello python' in line:
found_output = True
break
if not found_output:
self._FailWithOutput(stdout + stderr)
else:
# The --incompatible_windows_bashless_run_command should be a no-op on
# platforms other than Windows.
for flag in [
'--incompatible_windows_bashless_run_command',
'--noincompatible_windows_bashless_run_command'
]:
exit_code, stdout, stderr = self.RunBazel([
'run',
'--shell_executable=',
flag,
'//foo:x',
])
self.AssertNotExitCode(exit_code, 0, stderr)
found_error = False
for line in stdout + stderr:
if 'ERROR' in line and 'needs a shell' in line:
found_error = True
break
if not found_error:
self._FailWithOutput(['flag=' + flag] + stdout + stderr)


if __name__ == '__main__':
unittest.main()
52 changes: 38 additions & 14 deletions src/test/py/bazel/test_base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,31 +66,55 @@ def setUp(self):

def tearDown(self):
self.RunBazel(['shutdown'])

def AssertExitCode(self,
actual_exit_code,
expected_exit_code,
stderr_lines,
stdout_lines=None):
super(TestBase, self).tearDown()

def _AssertExitCodeIs(self,
actual_exit_code,
exit_code_pred,
expectation_msg,
stderr_lines,
stdout_lines=None):
"""Assert that `actual_exit_code` == `expected_exit_code`."""
if actual_exit_code != expected_exit_code:
if not exit_code_pred(actual_exit_code):
# If stdout was provided, include it in the output. This is mostly useful
# for tests.
stdout = '\n'.join([
'(start stdout)----------------------------------------',
] + stdout_lines + [
'(end stdout)------------------------------------------',
]) if stdout_lines is not None else ''
stdout = ''
if stdout_lines:
stdout = '\n'.join([
'(start stdout)----------------------------------------',
] + stdout_lines + [
'(end stdout)------------------------------------------',
])

self.fail('\n'.join([
'Bazel exited with %d (expected %d), stderr:' % (actual_exit_code,
expected_exit_code),
'Bazel exited with %d %s, stderr:' %
(actual_exit_code, expectation_msg),
stdout,
'(start stderr)----------------------------------------',
] + (stderr_lines or []) + [
'(end stderr)------------------------------------------',
]))

def AssertExitCode(self,
actual_exit_code,
expected_exit_code,
stderr_lines,
stdout_lines=None):
"""Assert that `actual_exit_code` == `expected_exit_code`."""
self._AssertExitCodeIs(actual_exit_code, lambda x: x == expected_exit_code,
'(expected %d)' % expected_exit_code, stderr_lines,
stdout_lines)

def AssertNotExitCode(self,
actual_exit_code,
not_expected_exit_code,
stderr_lines,
stdout_lines=None):
"""Assert that `actual_exit_code` != `not_expected_exit_code`."""
self._AssertExitCodeIs(
actual_exit_code, lambda x: x != not_expected_exit_code,
'(against expectations)', stderr_lines, stdout_lines)

@staticmethod
def GetEnv(name, default=None):
"""Returns environment variable `name`.
Expand Down

0 comments on commit e8af81c

Please sign in to comment.