diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java index 2434bff695bbb9..ef7ecdf4cb2bda 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java @@ -109,14 +109,12 @@ static class BazelWorkspaceStatusAction extends WorkspaceStatusAction { this.getWorkspaceStatusCommand = options.workspaceStatusCommand.equals(PathFragment.EMPTY_FRAGMENT) ? null - : new CommandBuilder() - .addArgs(options.workspaceStatusCommand.toString()) + : new CommandBuilder(workspace) + .addArg(options.workspaceStatusCommand.toString()) // Pass client env, because certain SCM client(like // perforce, git) relies on environment variables to work // correctly. .setEnv(clientEnv) - .setWorkingDir(workspace) - .useShell(true) .build(); this.workspace = workspace; } 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 5f231e4fd087cd..e5b77f1d5603bc 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 @@ -17,7 +17,6 @@ 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.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; @@ -37,6 +36,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; +import java.util.ArrayList; import java.util.List; /** @@ -90,10 +90,7 @@ public void createSymlinksUsingCommand( Path execRoot, BuildConfiguration config, BinTools binTools) throws CommandException { List argv = getSpawnArgumentList(execRoot, binTools.getExecPath(BUILD_RUNFILES)); - CommandBuilder builder = new CommandBuilder(); - builder.addArgs(argv); - builder.setWorkingDir(execRoot); - builder.build().execute(); + new CommandBuilder(execRoot).addArgs(argv).build().execute(); } /** @@ -148,7 +145,7 @@ Spawn createSpawn( ActionInput buildRunfiles = binTools.getActionInput(BUILD_RUNFILES); return new SimpleSpawn( owner, - getSpawnArgumentList(execRoot, buildRunfiles.getExecPath()), + ImmutableList.copyOf(getSpawnArgumentList(execRoot, buildRunfiles.getExecPath())), environment, ImmutableMap.of( ExecutionRequirements.LOCAL, "", @@ -162,8 +159,8 @@ Spawn createSpawn( /** * Returns the complete argument list build-runfiles has to be called with. */ - private ImmutableList getSpawnArgumentList(Path execRoot, PathFragment buildRunfiles) { - List args = Lists.newArrayList(); + private ArrayList getSpawnArgumentList(Path execRoot, PathFragment buildRunfiles) { + ArrayList args = new ArrayList<>(filesetTree ? 5 : 3); args.add(buildRunfiles.getPathString()); if (filesetTree) { @@ -174,6 +171,6 @@ private ImmutableList getSpawnArgumentList(Path execRoot, PathFragment b args.add(inputManifest.relativeTo(execRoot).getPathString()); args.add(symlinkTreeRoot.relativeTo(execRoot).getPathString()); - return ImmutableList.copyOf(args); + return args; } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java index d848d47469c512..c9c91f3f48de88 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java @@ -194,10 +194,10 @@ private static void asyncClean(CommandEnvironment env, Path path, String pathIte logger.info("Executing shell commmand " + ShellEscaper.escapeString(command)); // Doesn't throw iff command exited and was successful. - new CommandBuilder() - .addArg(command) - .useShell(true) - .setWorkingDir(tempPath.getParentDirectory()) + new CommandBuilder(tempPath.getParentDirectory()) + // TODO(laszlocsomor): implement the async directory tree deleter as a native, embedded + // binary, and stop relying on the shell. See GitHub issue #4319. + .addArgs("/bin/sh", "-c", command) .build() .execute(); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index 8aa3566041fe40..e649d31f9a70ed 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -237,8 +237,8 @@ private BlazeCommandResult runTargetUnderServer(CommandEnvironment env, null, "Running command line: " + ShellEscaper.escapeJoinAll(prettyCmdLine))); try { - com.google.devtools.build.lib.shell.Command command = new CommandBuilder() - .addArgs(cmdLine).setEnv(env.getClientEnv()).setWorkingDir(workingDir).build(); + com.google.devtools.build.lib.shell.Command command = new CommandBuilder(workingDir) + .addArgs(cmdLine).setEnv(env.getClientEnv()).build(); // Restore a raw EventHandler if it is registered. This allows for blaze run to produce the // actual output of the command being run even if --color=no is specified. diff --git a/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java index 6484bbe3f2d5be..0d43d8f95a031b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java @@ -285,11 +285,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsProvider options) Path workingDir = env.getBlazeWorkspace().getOutputPath().getParentDirectory(); com.google.devtools.build.lib.shell.Command command = - new CommandBuilder() - .addArgs(cmdLine) - .setEnv(env.getClientEnv()) - .setWorkingDir(workingDir) - .build(); + new CommandBuilder(workingDir).addArgs(cmdLine).setEnv(env.getClientEnv()).build(); try { // Restore a raw EventHandler if it is registered. This allows for blaze run to produce the diff --git a/src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java b/src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java index d60fd94dc48d02..31013ddc71d6f2 100644 --- a/src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java @@ -14,14 +14,8 @@ package com.google.devtools.build.lib.util; -import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR; - import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.CharMatcher; -import com.google.common.base.Joiner; import com.google.common.base.Preconditions; -import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.devtools.build.lib.shell.Command; @@ -30,49 +24,36 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; -import java.util.List; import java.util.Map; /** - * Implements OS aware {@link Command} builder. At this point only Linux, Mac - * and Windows XP are supported. - * - *

Builder will also apply heuristic to identify trivial cases where - * unix-like command lines could be automatically converted into the - * Windows-compatible form. + * Implements {@link Command} builder. * *

TODO(bazel-team): (2010) Some of the code here is very similar to the * {@link com.google.devtools.build.lib.shell.Shell} class. This should be looked at. */ public final class CommandBuilder { - private static final ImmutableList SHELLS = ImmutableList.of("/bin/sh", "/bin/bash"); - - private static final Splitter ARGV_SPLITTER = Splitter.on(CharMatcher.anyOf(" \t")); + private final ArrayList argv = new ArrayList<>(); + private final HashMap env = new HashMap<>(); + private final File workingDir; - private final OS system; - private final List argv = new ArrayList<>(); - private final Map env = new HashMap<>(); - private File workingDir = null; - private boolean useShell = false; - - public CommandBuilder() { - this(OS.getCurrent()); + public CommandBuilder(Path workingDir) { + this(workingDir.getPathFile()); } - @VisibleForTesting - CommandBuilder(OS system) { - this.system = system; + public CommandBuilder(File workingDir) { + this.workingDir = Preconditions.checkNotNull(workingDir); } public CommandBuilder addArg(String arg) { - Preconditions.checkNotNull(arg, "Argument must not be null"); + Preconditions.checkNotNull(arg); argv.add(arg); return this; } public CommandBuilder addArgs(Iterable args) { - Preconditions.checkArgument(!Iterables.contains(args, null), "Arguments must not be null"); + Preconditions.checkArgument(!Iterables.contains(args, null)); Iterables.addAll(argv, args); return this; } @@ -98,82 +79,8 @@ public CommandBuilder setEnv(Map env) { return this; } - public CommandBuilder setWorkingDir(Path path) { - Preconditions.checkNotNull(path); - workingDir = path.getPathFile(); - return this; - } - - public CommandBuilder useTempDir() { - workingDir = new File(JAVA_IO_TMPDIR.value()); - return this; - } - - public CommandBuilder useShell(boolean useShell) { - this.useShell = useShell; - return this; - } - - private boolean argvStartsWithSh() { - return argv.size() >= 2 && SHELLS.contains(argv.get(0)) && "-c".equals(argv.get(1)); - } - - private String[] transformArgvForLinux() { - // If command line already starts with "/bin/sh -c", ignore useShell attribute. - if (useShell && !argvStartsWithSh()) { - // c.g.io.base.shell.Shell.shellify() actually concatenates argv into the space-separated - // string here. Not sure why, but we will do the same. - return new String[] { "/bin/sh", "-c", Joiner.on(' ').join(argv) }; - } - return argv.toArray(new String[argv.size()]); - } - - private String[] transformArgvForWindows() { - List modifiedArgv; - // Heuristic: replace "/bin/sh -c" with something more appropriate for Windows. - if (argvStartsWithSh()) { - useShell = true; - modifiedArgv = Lists.newArrayList(argv.subList(2, argv.size())); - } else { - modifiedArgv = Lists.newArrayList(argv); - } - - if (!modifiedArgv.isEmpty()) { - // args can contain whitespace, so figure out the first word - String argv0 = modifiedArgv.get(0); - String command = ARGV_SPLITTER.split(argv0).iterator().next(); - - // Automatically enable CMD.EXE use if we are executing something else besides "*.exe" file. - // When use CMD.EXE to invoke a bat/cmd file, the file path must have '\' instead of '/' - if (!command.toLowerCase().endsWith(".exe")) { - useShell = true; - modifiedArgv.set(0, argv0.replace('/', '\\')); - } - } else { - // This is degenerate "/bin/sh -c" case. We ensure that Windows behavior is identical - // to the Linux - call shell that will do nothing. - useShell = true; - } - if (useShell) { - // /S - strip first and last quotes and execute everything else as is. - // /E:ON - enable extended command set. - // /V:ON - enable delayed variable expansion - // /D - ignore AutoRun registry entries. - // /C - execute command. This must be the last option before the command itself. - return new String[] { "CMD.EXE", "/S", "/E:ON", "/V:ON", "/D", "/C", - Joiner.on(' ').join(modifiedArgv) }; - } else { - return modifiedArgv.toArray(new String[argv.size()]); - } - } - public Command build() { - Preconditions.checkState(system != OS.UNKNOWN, "Unidentified operating system"); - Preconditions.checkNotNull(workingDir, "Working directory must be set"); Preconditions.checkState(!argv.isEmpty(), "At least one argument is expected"); - - return new Command( - system == OS.WINDOWS ? transformArgvForWindows() : transformArgvForLinux(), - env, workingDir); + return new Command(argv.toArray(new String[0]), env, workingDir); } } diff --git a/src/test/java/com/google/devtools/build/lib/util/CommandBuilderTest.java b/src/test/java/com/google/devtools/build/lib/util/CommandBuilderTest.java index 99ee49d027a205..9eddb03d00202f 100644 --- a/src/test/java/com/google/devtools/build/lib/util/CommandBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/CommandBuilderTest.java @@ -17,6 +17,7 @@ import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; +import java.io.File; import java.util.Arrays; import org.junit.Test; import org.junit.runner.RunWith; @@ -28,12 +29,8 @@ @RunWith(JUnit4.class) public class CommandBuilderTest { - private CommandBuilder linuxBuilder() { - return new CommandBuilder(OS.LINUX).useTempDir(); - } - - private CommandBuilder winBuilder() { - return new CommandBuilder(OS.WINDOWS).useTempDir(); + private CommandBuilder builder() { + return new CommandBuilder(new File("dummy-workdir")); } private void assertArgv(CommandBuilder builder, String... expected) { @@ -43,10 +40,6 @@ private void assertArgv(CommandBuilder builder, String... expected) { .inOrder(); } - private void assertWinCmdArgv(CommandBuilder builder, String expected) { - assertArgv(builder, "CMD.EXE", "/S", "/E:ON", "/V:ON", "/D", "/C", expected); - } - private void assertFailure(CommandBuilder builder, String expected) { try { builder.build(); @@ -57,47 +50,18 @@ private void assertFailure(CommandBuilder builder, String expected) { } @Test - public void linuxBuilderTest() { - assertArgv(linuxBuilder().addArg("abc"), "abc"); - assertArgv(linuxBuilder().addArg("abc def"), "abc def"); - assertArgv(linuxBuilder().addArgs("abc", "def"), "abc", "def"); - assertArgv(linuxBuilder().addArgs(ImmutableList.of("abc", "def")), "abc", "def"); - assertArgv(linuxBuilder().addArg("abc").useShell(true), "/bin/sh", "-c", "abc"); - assertArgv(linuxBuilder().addArg("abc def").useShell(true), "/bin/sh", "-c", "abc def"); - assertArgv(linuxBuilder().addArgs("abc", "def").useShell(true), "/bin/sh", "-c", "abc def"); - assertArgv(linuxBuilder().addArgs("/bin/sh", "-c", "abc").useShell(true), - "/bin/sh", "-c", "abc"); - assertArgv(linuxBuilder().addArgs("/bin/sh", "-c"), "/bin/sh", "-c"); - assertArgv(linuxBuilder().addArgs("/bin/bash", "-c"), "/bin/bash", "-c"); - assertArgv(linuxBuilder().addArgs("/bin/sh", "-c").useShell(true), "/bin/sh", "-c"); - assertArgv(linuxBuilder().addArgs("/bin/bash", "-c").useShell(true), "/bin/bash", "-c"); - } - - @Test - public void windowsBuilderTest() { - assertArgv(winBuilder().addArg("abc.exe"), "abc.exe"); - assertArgv(winBuilder().addArg("abc.exe -o"), "abc.exe -o"); - assertArgv(winBuilder().addArg("ABC.EXE"), "ABC.EXE"); - assertWinCmdArgv(winBuilder().addArg("abc def.exe"), "abc def.exe"); - assertArgv(winBuilder().addArgs("abc.exe", "def"), "abc.exe", "def"); - assertArgv(winBuilder().addArgs(ImmutableList.of("abc.exe", "def")), "abc.exe", "def"); - assertWinCmdArgv(winBuilder().addArgs("abc.exe", "def").useShell(true), "abc.exe def"); - assertWinCmdArgv(winBuilder().addArg("abc"), "abc"); - assertWinCmdArgv(winBuilder().addArgs("abc", "def"), "abc def"); - assertWinCmdArgv(winBuilder().addArgs("/bin/sh", "-c", "abc", "def"), "abc def"); - assertWinCmdArgv(winBuilder().addArgs("/bin/sh", "-c"), ""); - assertWinCmdArgv(winBuilder().addArgs("/bin/bash", "-c"), ""); - assertWinCmdArgv(winBuilder().addArgs("/bin/sh", "-c").useShell(true), ""); - assertWinCmdArgv(winBuilder().addArgs("/bin/bash", "-c").useShell(true), ""); + public void builderTest() { + assertArgv(builder().addArg("abc"), "abc"); + assertArgv(builder().addArg("abc def"), "abc def"); + assertArgv(builder().addArgs("abc", "def"), "abc", "def"); + assertArgv(builder().addArgs(ImmutableList.of("abc", "def")), "abc", "def"); + assertArgv(builder().addArgs("/bin/sh", "-c", "abc"), "/bin/sh", "-c", "abc"); + assertArgv(builder().addArgs("/bin/sh", "-c"), "/bin/sh", "-c"); + assertArgv(builder().addArgs("/bin/bash", "-c"), "/bin/bash", "-c"); } @Test public void failureScenarios() { - assertFailure(linuxBuilder(), "At least one argument is expected"); - assertFailure(new CommandBuilder(OS.UNKNOWN).useTempDir().addArg("a"), - "Unidentified operating system"); - assertFailure(new CommandBuilder(OS.LINUX).addArg("a"), - "Working directory must be set"); + assertFailure(builder(), "At least one argument is expected"); } - } diff --git a/src/test/shell/bazel/bazel_workspace_status_test.sh b/src/test/shell/bazel/bazel_workspace_status_test.sh index 508dabb9055b68..9db30976b6fb73 100755 --- a/src/test/shell/bazel/bazel_workspace_status_test.sh +++ b/src/test/shell/bazel/bazel_workspace_status_test.sh @@ -129,7 +129,7 @@ EOF bazel build --workspace_status_command=$TEST_TMPDIR/wscmissing.sh --stamp //:a &> $TEST_log \ && fail "build succeeded" - expect_log "wscmissing.sh: No such file or directory\|wscmissing.sh: not found" + expect_log "Cannot run program.*/wscmissing.sh.* No such file or directory" }