From 23d986d347d0c6b4674d9255173cbf0ef434157f Mon Sep 17 00:00:00 2001 From: jmmv Date: Fri, 10 May 2019 13:12:07 -0700 Subject: [PATCH] Homogenize LocalEnvProvider instantiation in a single place. This adds a new forCurrentOS factory method to LocalEnvProvider to construct the correct local env provider for the current OS, and removes duplicate (and inconsistent) logic from a bunch of places. To make this change simpler, this also moves XcodeLocalEnvProvider from an "apple" package (which is useless because it only contains one class and we depend on in anyway in a Bazel build) to the "local" subpackage. And as a side-effect, this gets rid of XcodeLocalEnvProviderTest, which I don't understand how it could possibly have ever worked: the test was trying to validate a condition on non-Darwin platforms but was later restricting itself to running on Darwin platforms only. We didn't see this because this test was not being executed in the Bazel exported tree (that is, was not run on BazelCI) and is now run after the code move. I tried to fix the test, but it's so out of sync with the current code that even understanding what condition it wanted to test was hard (but it was not that useful). RELNOTES: None. PiperOrigin-RevId: 247663150 --- .../java/com/google/devtools/build/lib/BUILD | 1 - .../devtools/build/lib/exec/apple/BUILD | 20 ------- .../devtools/build/lib/exec/local/BUILD | 2 + .../lib/exec/local/LocalEnvProvider.java | 25 ++++++--- .../lib/exec/local/PosixLocalEnvProvider.java | 3 + .../exec/local/WindowsLocalEnvProvider.java | 4 +- .../XcodeLocalEnvProvider.java | 7 ++- .../google/devtools/build/lib/sandbox/BUILD | 1 - .../sandbox/DarwinSandboxedSpawnRunner.java | 3 +- .../sandbox/DockerSandboxedSpawnRunner.java | 3 +- .../ProcessWrapperSandboxedSpawnRunner.java | 7 +-- .../build/lib/sandbox/SandboxModule.java | 19 ++----- .../devtools/build/lib/standalone/BUILD | 2 - .../lib/standalone/StandaloneModule.java | 33 +++-------- .../google/devtools/build/lib/worker/BUILD | 2 - .../build/lib/worker/WorkerModule.java | 14 +---- .../exec/apple/XcodeLocalEnvProviderTest.java | 55 ------------------- .../lib/exec/local/LocalSpawnRunnerTest.java | 38 ++++++++----- .../StandaloneSpawnStrategyTest.java | 3 +- 19 files changed, 73 insertions(+), 169 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/exec/apple/BUILD rename src/main/java/com/google/devtools/build/lib/exec/{apple => local}/XcodeLocalEnvProvider.java (98%) delete mode 100644 src/test/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProviderTest.java diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index ecc916563a2a21..665da0bf06e8ed 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -35,7 +35,6 @@ filegroup( "//src/main/java/com/google/devtools/build/lib/collect:srcs", "//src/main/java/com/google/devtools/build/lib/concurrent:srcs", "//src/main/java/com/google/devtools/build/lib/dynamic:srcs", - "//src/main/java/com/google/devtools/build/lib/exec/apple:srcs", "//src/main/java/com/google/devtools/build/lib/exec/local:srcs", "//src/main/java/com/google/devtools/build/lib/graph:srcs", "//src/main/java/com/google/devtools/build/lib/metrics:srcs", diff --git a/src/main/java/com/google/devtools/build/lib/exec/apple/BUILD b/src/main/java/com/google/devtools/build/lib/exec/apple/BUILD deleted file mode 100644 index cbe9d78b4453c2..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/exec/apple/BUILD +++ /dev/null @@ -1,20 +0,0 @@ -package(default_visibility = ["//src:__subpackages__"]) - -filegroup( - name = "srcs", - srcs = glob(["**"]), - visibility = ["//src/main/java/com/google/devtools/build/lib:__pkg__"], -) - -java_library( - name = "apple", - srcs = glob(["*.java"]), - deps = [ - "//src/main/java/com/google/devtools/build/lib:build-base", - "//src/main/java/com/google/devtools/build/lib/exec/local", - "//src/main/java/com/google/devtools/build/lib/rules/apple", - "//src/main/java/com/google/devtools/build/lib/shell", - "//src/main/java/com/google/devtools/build/lib/vfs", - "//third_party:guava", - ], -) diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/BUILD b/src/main/java/com/google/devtools/build/lib/exec/local/BUILD index 9ef0881a89dcd4..3b29a2425449b2 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/local/BUILD @@ -13,6 +13,7 @@ java_library( "LocalSpawnRunner.java", "PosixLocalEnvProvider.java", "WindowsLocalEnvProvider.java", + "XcodeLocalEnvProvider.java", ], data = [ "//src/main/tools:process-wrapper", @@ -26,6 +27,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/rules/apple", "//src/main/java/com/google/devtools/build/lib/shell", "//src/main/java/com/google/devtools/build/lib/vfs", "//third_party:error_prone_annotations", diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java index 7ae3e756b36d70..7d5213a4460051 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.exec.local; import com.google.devtools.build.lib.exec.BinTools; +import com.google.devtools.build.lib.util.OS; import java.io.IOException; import java.util.Map; @@ -23,14 +24,22 @@ */ public interface LocalEnvProvider { - public static final LocalEnvProvider UNMODIFIED = - new LocalEnvProvider() { - @Override - public Map rewriteLocalEnv( - Map env, BinTools binTools, String fallbackTmpDir) { - return env; - } - }; + /** + * Creates a local environment provider for the current OS. + * + * @param clientEnv the environment variables as supplied by the Bazel client + * @return the local environment provider + */ + static LocalEnvProvider forCurrentOs(Map clientEnv) { + switch (OS.getCurrent()) { + case DARWIN: + return new XcodeLocalEnvProvider(clientEnv); + case WINDOWS: + return new WindowsLocalEnvProvider(clientEnv); + default: + return new PosixLocalEnvProvider(clientEnv); + } + } /** * Rewrites a {@code Spawn}'s the environment if necessary. diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java index 2c154bcde7f200..7fdea613b409e4 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java @@ -26,6 +26,9 @@ public final class PosixLocalEnvProvider implements LocalEnvProvider { /** * Create a new {@link PosixLocalEnvProvider}. * + *

Use {@link LocalEnvProvider#forCurrentOs(Map)} to instantiate this unless the calling code + * is platform-specific. + * * @param clientEnv a map of the current Bazel command's environment */ public PosixLocalEnvProvider(Map clientEnv) { diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java index b1700f69ca3084..e0ce8aeb639ee9 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java @@ -26,9 +26,11 @@ public final class WindowsLocalEnvProvider implements LocalEnvProvider { /** * Create a new {@link WindowsLocalEnvProvider}. * + *

Use {@link LocalEnvProvider#forCurrentOs(Map)} to instantiate this. + * * @param clientEnv a map of the current Bazel command's environment */ - public WindowsLocalEnvProvider(Map clientEnv) { + WindowsLocalEnvProvider(Map clientEnv) { this.clientEnv = clientEnv; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java similarity index 98% rename from src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java rename to src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java index 77d7fb0b1666b3..c2c5ea6e6da972 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java @@ -11,13 +11,12 @@ // 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.apple; +package com.google.devtools.build.lib.exec.local; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import com.google.devtools.build.lib.exec.BinTools; -import com.google.devtools.build.lib.exec.local.LocalEnvProvider; import com.google.devtools.build.lib.rules.apple.AppleConfiguration; import com.google.devtools.build.lib.rules.apple.DottedVersion; import com.google.devtools.build.lib.shell.AbnormalTerminationException; @@ -57,9 +56,11 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider { /** * Creates a new {@link XcodeLocalEnvProvider}. * + *

Use {@link LocalEnvProvider#forCurrentOs(Map)} to instantiate this. + * * @param clientEnv a map of the current Bazel command's environment */ - public XcodeLocalEnvProvider(Map clientEnv) { + XcodeLocalEnvProvider(Map clientEnv) { this.clientEnv = clientEnv; } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD index 7b745bef2fc3c5..635013f1dfb77a 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD +++ b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD @@ -27,7 +27,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib:util", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", - "//src/main/java/com/google/devtools/build/lib/exec/apple", "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/exec/local:options", "//src/main/java/com/google/devtools/build/lib/profiler", 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 0e32929e5602cc..f63e3a5450c2c7 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 @@ -27,7 +27,6 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.Spawns; import com.google.devtools.build.lib.exec.TreeDeleter; -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; import com.google.devtools.build.lib.runtime.ProcessWrapperUtil; @@ -148,7 +147,7 @@ private static boolean computeIsSupported() { this.allowNetwork = SandboxHelpers.shouldAllowNetwork(cmdEnv.getOptions()); this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem()); this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv); - this.localEnvProvider = new XcodeLocalEnvProvider(cmdEnv.getClientEnv()); + this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv()); this.sandboxBase = sandboxBase; this.timeoutKillDelay = timeoutKillDelay; this.sandboxfsProcess = sandboxfsProcess; diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java index 76df1d2990b0c3..1afdf68c852bae 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java @@ -30,7 +30,6 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.TreeDeleter; import com.google.devtools.build.lib.exec.local.LocalEnvProvider; -import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider; import com.google.devtools.build.lib.runtime.CommandCompleteEvent; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.ProcessWrapperUtil; @@ -179,7 +178,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient) this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv); this.sandboxBase = sandboxBase; this.defaultImage = defaultImage; - this.localEnvProvider = new PosixLocalEnvProvider(cmdEnv.getClientEnv()); + this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv()); this.timeoutKillDelay = timeoutKillDelay; this.commandId = cmdEnv.getCommandId().toString(); this.reporter = cmdEnv.getReporter(); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java index 169497ec93ab29..5cf27cbe136b9f 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java @@ -18,9 +18,7 @@ import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.exec.TreeDeleter; -import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider; import com.google.devtools.build.lib.exec.local.LocalEnvProvider; -import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.ProcessWrapperUtil; import com.google.devtools.build.lib.util.OS; @@ -60,10 +58,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv) { super(cmdEnv); this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv); this.execRoot = cmdEnv.getExecRoot(); - this.localEnvProvider = - OS.getCurrent() == OS.DARWIN - ? new XcodeLocalEnvProvider(cmdEnv.getClientEnv()) - : new PosixLocalEnvProvider(cmdEnv.getClientEnv()); + this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv()); this.sandboxBase = sandboxBase; this.timeoutKillDelay = timeoutKillDelay; this.treeDeleter = treeDeleter; diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java index e02b0c206ed72b..e8ce0794a03807 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java @@ -33,11 +33,9 @@ import com.google.devtools.build.lib.exec.ExecutorBuilder; import com.google.devtools.build.lib.exec.SpawnRunner; import com.google.devtools.build.lib.exec.TreeDeleter; -import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider; import com.google.devtools.build.lib.exec.local.LocalEnvProvider; import com.google.devtools.build.lib.exec.local.LocalExecutionOptions; import com.google.devtools.build.lib.exec.local.LocalSpawnRunner; -import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.runtime.BlazeModule; @@ -374,17 +372,12 @@ private static SpawnRunner withFallback(CommandEnvironment env, SpawnRunner sand private static SpawnRunner createFallbackRunner(CommandEnvironment env) { LocalExecutionOptions localExecutionOptions = env.getOptions().getOptions(LocalExecutionOptions.class); - LocalEnvProvider localEnvProvider = - OS.getCurrent() == OS.DARWIN - ? new XcodeLocalEnvProvider(env.getClientEnv()) - : new PosixLocalEnvProvider(env.getClientEnv()); - return - new LocalSpawnRunner( - env.getExecRoot(), - localExecutionOptions, - ResourceManager.instance(), - localEnvProvider, - env.getBlazeWorkspace().getBinTools()); + return new LocalSpawnRunner( + env.getExecRoot(), + localExecutionOptions, + ResourceManager.instance(), + LocalEnvProvider.forCurrentOs(env.getClientEnv()), + env.getBlazeWorkspace().getBinTools()); } private static final class SandboxFallbackSpawnRunner implements SpawnRunner { diff --git a/src/main/java/com/google/devtools/build/lib/standalone/BUILD b/src/main/java/com/google/devtools/build/lib/standalone/BUILD index c1a479d3b918e7..0a66839b140a64 100644 --- a/src/main/java/com/google/devtools/build/lib/standalone/BUILD +++ b/src/main/java/com/google/devtools/build/lib/standalone/BUILD @@ -17,11 +17,9 @@ java_library( ], deps = [ "//src/main/java/com/google/devtools/build/lib:build-base", - "//src/main/java/com/google/devtools/build/lib:os_util", "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib:testing-support-rules", "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/exec/apple", "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/exec/local:options", "//src/main/java/com/google/devtools/build/lib/rules/cpp", diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java index bc08cbc92bd219..49228183c9107b 100644 --- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java +++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java @@ -25,16 +25,12 @@ import com.google.devtools.build.lib.exec.SpawnRunner; import com.google.devtools.build.lib.exec.StandaloneTestStrategy; import com.google.devtools.build.lib.exec.TestStrategy; -import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider; import com.google.devtools.build.lib.exec.local.LocalEnvProvider; import com.google.devtools.build.lib.exec.local.LocalExecutionOptions; import com.google.devtools.build.lib.exec.local.LocalSpawnRunner; -import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider; -import com.google.devtools.build.lib.exec.local.WindowsLocalEnvProvider; import com.google.devtools.build.lib.rules.test.ExclusiveTestStrategy; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.CommandEnvironment; -import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.Path; /** @@ -56,11 +52,18 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB env.getBlazeWorkspace().getBinTools(), testTmpRoot); + SpawnRunner localSpawnRunner = + new LocalSpawnRunner( + env.getExecRoot(), + env.getOptions().getOptions(LocalExecutionOptions.class), + ResourceManager.instance(), + LocalEnvProvider.forCurrentOs(env.getClientEnv()), + env.getBlazeWorkspace().getBinTools()); + // Order of strategies passed to builder is significant - when there are many strategies that // could potentially be used and a spawnActionContext doesn't specify which one it wants, the // last one from strategies list will be used - builder.addActionContext( - new StandaloneSpawnStrategy(env.getExecRoot(), createLocalRunner(env))); + builder.addActionContext(new StandaloneSpawnStrategy(env.getExecRoot(), localSpawnRunner)); builder.addActionContext(testStrategy); builder.addActionContext(new ExclusiveTestStrategy(testStrategy)); builder.addActionContext(new FileWriteStrategy()); @@ -74,22 +77,4 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB // necessarily the default. builder.addStrategyByContext(SpawnActionContext.class, "standalone"); } - - private static SpawnRunner createLocalRunner(CommandEnvironment env) { - LocalExecutionOptions localExecutionOptions = - env.getOptions().getOptions(LocalExecutionOptions.class); - LocalEnvProvider localEnvProvider = - OS.getCurrent() == OS.DARWIN - ? new XcodeLocalEnvProvider(env.getClientEnv()) - : (OS.getCurrent() == OS.WINDOWS - ? new WindowsLocalEnvProvider(env.getClientEnv()) - : new PosixLocalEnvProvider(env.getClientEnv())); - return - new LocalSpawnRunner( - env.getExecRoot(), - localExecutionOptions, - ResourceManager.instance(), - localEnvProvider, - env.getBlazeWorkspace().getBinTools()); - } } diff --git a/src/main/java/com/google/devtools/build/lib/worker/BUILD b/src/main/java/com/google/devtools/build/lib/worker/BUILD index d1f096214d4873..422eae71319678 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/BUILD +++ b/src/main/java/com/google/devtools/build/lib/worker/BUILD @@ -16,12 +16,10 @@ java_library( "//src/main/java/com/google/devtools/build/lib:build-base", "//src/main/java/com/google/devtools/build/lib:events", "//src/main/java/com/google/devtools/build/lib:io", - "//src/main/java/com/google/devtools/build/lib:os_util", "//src/main/java/com/google/devtools/build/lib:resource-converter", "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib:util", "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/exec/apple", "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/exec/local:options", "//src/main/java/com/google/devtools/build/lib/sandbox", diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index 544f0c053ebd34..ee258d4832fab0 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java @@ -27,18 +27,14 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.exec.ExecutorBuilder; import com.google.devtools.build.lib.exec.SpawnRunner; -import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider; import com.google.devtools.build.lib.exec.local.LocalEnvProvider; import com.google.devtools.build.lib.exec.local.LocalExecutionOptions; import com.google.devtools.build.lib.exec.local.LocalSpawnRunner; -import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider; -import com.google.devtools.build.lib.exec.local.WindowsLocalEnvProvider; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.Command; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.commands.CleanCommand.CleanStartingEvent; import com.google.devtools.build.lib.sandbox.SandboxOptions; -import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.worker.WorkerOptions.MultiResourceConverter; import com.google.devtools.common.options.OptionsBase; @@ -145,7 +141,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB Preconditions.checkNotNull(workerPool); ImmutableMultimap extraFlags = ImmutableMultimap.copyOf(env.getOptions().getOptions(WorkerOptions.class).workerExtraFlags); - LocalEnvProvider localEnvProvider = createLocalEnvProvider(env); + LocalEnvProvider localEnvProvider = LocalEnvProvider.forCurrentOs(env.getClientEnv()); WorkerSpawnRunner spawnRunner = new WorkerSpawnRunner( env.getExecRoot(), @@ -176,14 +172,6 @@ private static SpawnRunner createFallbackRunner( env.getBlazeWorkspace().getBinTools()); } - private static LocalEnvProvider createLocalEnvProvider(CommandEnvironment env) { - return OS.getCurrent() == OS.DARWIN - ? new XcodeLocalEnvProvider(env.getClientEnv()) - : (OS.getCurrent() == OS.WINDOWS - ? new WindowsLocalEnvProvider(env.getClientEnv()) - : new PosixLocalEnvProvider(env.getClientEnv())); - } - @Subscribe public void buildComplete(BuildCompleteEvent event) { if (options != null && options.workerQuitAfterBuild) { diff --git a/src/test/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProviderTest.java b/src/test/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProviderTest.java deleted file mode 100644 index 3ad0503906ea46..00000000000000 --- a/src/test/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProviderTest.java +++ /dev/null @@ -1,55 +0,0 @@ -// 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.apple; - -import static com.google.common.truth.Truth.assertThat; -import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; -import static org.junit.Assume.assumeTrue; - -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.exec.BinTools; -import com.google.devtools.build.lib.rules.apple.AppleConfiguration; -import com.google.devtools.build.lib.util.OS; -import com.google.devtools.build.lib.vfs.DigestHashFunction; -import com.google.devtools.build.lib.vfs.FileSystem; -import com.google.devtools.build.lib.vfs.JavaIoFileSystem; -import java.io.IOException; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for {@link XcodeLocalEnvProvider}. */ -@RunWith(JUnit4.class) -public class XcodeLocalEnvProviderTest { - private final FileSystem fs = new JavaIoFileSystem(DigestHashFunction.DEFAULT_HASH_FOR_TESTS); - - @Test - public void testIOSEnvironmentOnNonDarwin() { - assumeTrue(OS.getCurrent() == OS.DARWIN); - IOException e = - assertThrows( - IOException.class, - () -> - new XcodeLocalEnvProvider(ImmutableMap.of()) - .rewriteLocalEnv( - ImmutableMap.of( - AppleConfiguration.APPLE_SDK_VERSION_ENV_NAME, "8.4", - AppleConfiguration.APPLE_SDK_PLATFORM_ENV_NAME, "iPhoneSimulator"), - BinTools.forUnitTesting( - fs.getPath("/tmp"), ImmutableSet.of("xcode-locator")), - "bazel")); - assertThat(e).hasMessageThat().contains("Cannot locate iOS SDK on non-darwin operating system"); - } -} diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java index a809f76c11452a..c2c721b86e9bd9 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java @@ -73,6 +73,7 @@ import java.time.Duration; import java.util.ArrayList; import java.util.List; +import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; import java.util.concurrent.ThreadLocalRandom; @@ -271,6 +272,10 @@ public MetadataInjector getMetadataInjector() { private Logger logger; + private static Map keepLocalEnvUnchanged( + Map env, BinTools binTools, String fallbackTmpDir) { + return env; + } @Before public final void suppressLogging() { @@ -326,7 +331,7 @@ public void vanillaZeroExit() throws Exception { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); @@ -377,8 +382,13 @@ public void testParamFiles() throws Exception { Path execRoot = fs.getPath("/execroot"); LocalSpawnRunner runner = new TestedLocalSpawnRunner( - execRoot, fs.getPath("/embedded_bin"), options, resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + execRoot, + fs.getPath("/embedded_bin"), + options, + resourceManager, + USE_WRAPPER, + OS.LINUX, + LocalSpawnRunnerTest::keepLocalEnvUnchanged); ParamFileActionInput paramFileActionInput = new ParamFileActionInput( PathFragment.create("some/dir/params"), @@ -432,7 +442,7 @@ public void noProcessWrapper() throws Exception { resourceManager, NO_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); @@ -477,7 +487,7 @@ public void nonZeroExit() throws Exception { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); @@ -525,7 +535,7 @@ public void processStartupThrows() throws Exception { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); assertThat(fs.getPath("/out").createDirectory()).isTrue(); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); @@ -561,7 +571,7 @@ public void disallowLocalExecution() throws Exception { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); FileOutErr fileOutErr = new FileOutErr(); @@ -612,7 +622,7 @@ public void waitFor() throws InterruptedException { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); @@ -639,7 +649,7 @@ public void checkPrefetchCalled() throws Exception { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); @@ -666,7 +676,7 @@ public void checkNoPrefetchCalled() throws Exception { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); @@ -736,7 +746,7 @@ public void useCorrectExtensionOnWindows() throws Exception { resourceManager, USE_WRAPPER, OS.WINDOWS, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); @@ -867,7 +877,7 @@ public void hasExecutionStatistics_whenOptionIsEnabled() throws Exception { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED, + LocalSpawnRunnerTest::keepLocalEnvUnchanged, binTools); Spawn spawn = @@ -930,7 +940,7 @@ public void hasNoExecutionStatistics_whenOptionIsDisabled() throws Exception { resourceManager, USE_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED, + LocalSpawnRunnerTest::keepLocalEnvUnchanged, binTools); Spawn spawn = @@ -984,7 +994,7 @@ public void relativePath() throws Exception { resourceManager, NO_WRAPPER, OS.LINUX, - LocalEnvProvider.UNMODIFIED); + LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); 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 049ef85ae670c0..aba06f6414b08d 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 @@ -44,7 +44,6 @@ import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.SingleBuildFileCache; import com.google.devtools.build.lib.exec.SpawnActionContextMaps; -import com.google.devtools.build.lib.exec.local.LocalEnvProvider; import com.google.devtools.build.lib.exec.local.LocalExecutionOptions; import com.google.devtools.build.lib.exec.local.LocalSpawnRunner; import com.google.devtools.build.lib.integration.util.IntegrationMock; @@ -141,7 +140,7 @@ public final void setUp() throws Exception { execRoot, localExecutionOptions, resourceManager, - LocalEnvProvider.UNMODIFIED, + (env, unusedBinTools, unusedFallbackTempDir) -> env, BinTools.forIntegrationTesting( directories, ImmutableList.of())))))), ImmutableList.of());