Move shell executable to its own configuration fragment. RELNOTES: None. PiperOrigin-RevId: 191861074
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 0e43077..6d6d8a0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -1270,7 +1270,7 @@ getPrerequisiteMapForTesting( eventHandler, configuredTarget, configurations, toolchainContext)) .setConfigConditions(ImmutableMap.<Label, ConfigMatchingProvider>of()) - .setUniversalFragment(ruleClassProvider.getUniversalFragment()) + .setUniversalFragments(ruleClassProvider.getUniversalFragments()) .setToolchainContext(toolchainContext) .build(); }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java index 85dfc7a..f364257 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
@@ -306,6 +306,7 @@ 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().getShellExecutable(); + ? PathFragment.create("/bin/bash") + : ruleContext.getConfiguration().getFragment(ShellConfiguration.class).getShellExecutable(); } }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index 34836ab..e55b44a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -225,7 +225,8 @@ private final Digraph<Class<? extends RuleDefinition>> dependencyGraph = new Digraph<>(); private PatchTransition lipoDataTransition; - private Class<? extends BuildConfiguration.Fragment> universalFragment; + private List<Class<? extends BuildConfiguration.Fragment>> universalFragments = + new ArrayList<>(); private PrerequisiteValidator prerequisiteValidator; private ImmutableMap.Builder<String, Object> skylarkAccessibleTopLevels = ImmutableMap.builder(); @@ -340,9 +341,9 @@ return this; } - public Builder setUniversalConfigurationFragment( + public Builder addUniversalConfigurationFragment( Class<? extends BuildConfiguration.Fragment> fragment) { - this.universalFragment = fragment; + this.universalFragments.add(fragment); return this; } @@ -456,7 +457,7 @@ ImmutableList.copyOf(configurationOptions), ImmutableList.copyOf(configurationFragmentFactories), lipoDataTransition, - universalFragment, + ImmutableList.copyOf(universalFragments), prerequisiteValidator, skylarkAccessibleTopLevels.build(), skylarkModules.build(), @@ -555,10 +556,10 @@ private final PatchTransition lipoDataTransition; /** - * A configuration fragment that should be available to all rules even when they don't + * Configuration fragments that should be available to all rules even when they don't * explicitly require it. */ - private final Class<? extends BuildConfiguration.Fragment> universalFragment; + private final ImmutableList<Class<? extends BuildConfiguration.Fragment>> universalFragments; private final ImmutableList<BuildInfoFactory> buildInfoFactories; @@ -583,7 +584,7 @@ ImmutableList<Class<? extends FragmentOptions>> configurationOptions, ImmutableList<ConfigurationFragmentFactory> configurationFragments, PatchTransition lipoDataTransition, - Class<? extends BuildConfiguration.Fragment> universalFragment, + ImmutableList<Class<? extends BuildConfiguration.Fragment>> universalFragments, PrerequisiteValidator prerequisiteValidator, ImmutableMap<String, Object> skylarkAccessibleJavaClasses, ImmutableList<Class<?>> skylarkModules, @@ -600,7 +601,7 @@ this.configurationOptions = configurationOptions; this.configurationFragmentFactories = configurationFragments; this.lipoDataTransition = lipoDataTransition; - this.universalFragment = universalFragment; + this.universalFragments = universalFragments; this.prerequisiteValidator = prerequisiteValidator; this.globals = createGlobals(skylarkAccessibleJavaClasses, skylarkModules); this.nativeProviders = nativeProviders; @@ -685,8 +686,8 @@ * Returns the configuration fragment that should be available to all rules even when they * don't explicitly require it. */ - public Class<? extends BuildConfiguration.Fragment> getUniversalFragment() { - return universalFragment; + public ImmutableList<Class<? extends BuildConfiguration.Fragment>> getUniversalFragments() { + return universalFragments; } /** @@ -808,7 +809,7 @@ for (ConfigurationFragmentFactory factory : getConfigurationFragments()) { fragmentsBuilder.add(factory.creates()); } - fragmentsBuilder.add(getUniversalFragment()); + fragmentsBuilder.addAll(getUniversalFragments()); return fragmentsBuilder.build(); }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index 1edf561..7a1d4ec 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -339,7 +339,7 @@ .setVisibility(convertVisibility(prerequisiteMap, env.getEventHandler(), rule, null)) .setPrerequisites(prerequisiteMap) .setConfigConditions(configConditions) - .setUniversalFragment(ruleClassProvider.getUniversalFragment()) + .setUniversalFragments(ruleClassProvider.getUniversalFragments()) .setToolchainContext(toolchainContext) .build(); if (ruleContext.hasErrors()) { @@ -461,7 +461,7 @@ .setPrerequisites(prerequisiteMap) .setAspectAttributes(aspectAttributes) .setConfigConditions(configConditions) - .setUniversalFragment(ruleClassProvider.getUniversalFragment()) + .setUniversalFragments(ruleClassProvider.getUniversalFragments()) .setToolchainContext(toolchainContext) .build(); if (ruleContext.hasErrors()) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index c8c4775..043bee7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -184,7 +184,7 @@ private final BuildConfiguration hostConfiguration; private final PatchTransition disableLipoTransition; private final ConfigurationFragmentPolicy configurationFragmentPolicy; - private final Class<? extends BuildConfiguration.Fragment> universalFragment; + private final ImmutableList<Class<? extends BuildConfiguration.Fragment>> universalFragments; private final ErrorReporter reporter; @Nullable private final ToolchainContext toolchainContext; @@ -199,7 +199,7 @@ ListMultimap<String, ConfiguredTargetAndData> targetMap, ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap, ImmutableMap<Label, ConfigMatchingProvider> configConditions, - Class<? extends BuildConfiguration.Fragment> universalFragment, + ImmutableList<Class<? extends BuildConfiguration.Fragment>> universalFragments, String ruleClassNameForLogging, ImmutableMap<String, Attribute> aspectAttributes, @Nullable ToolchainContext toolchainContext) { @@ -214,7 +214,7 @@ .map(a -> a.getDescriptor()) .collect(ImmutableList.toImmutableList()); this.configurationFragmentPolicy = builder.configurationFragmentPolicy; - this.universalFragment = universalFragment; + this.universalFragments = universalFragments; this.targetMap = targetMap; this.filesetEntryMap = filesetEntryMap; this.configConditions = configConditions; @@ -452,7 +452,7 @@ public <T extends Fragment> boolean isLegalFragment( Class<T> fragment, ConfigurationTransition transition) { - return fragment == universalFragment + return universalFragments.contains(fragment) || fragment == PlatformConfiguration.class || configurationFragmentPolicy.isLegalConfigurationFragment(fragment, transition); } @@ -1405,7 +1405,7 @@ private final AnalysisEnvironment env; private final Rule rule; private final ConfigurationFragmentPolicy configurationFragmentPolicy; - private Class<? extends BuildConfiguration.Fragment> universalFragment; + private ImmutableList<Class<? extends BuildConfiguration.Fragment>> universalFragments; private final BuildConfiguration configuration; private final BuildConfiguration hostConfiguration; private PatchTransition disableLipoTransition; @@ -1453,7 +1453,7 @@ targetMap, filesetEntryMap, configConditions, - universalFragment, + universalFragments, getRuleClassNameForLogging(), aspectAttributes != null ? aspectAttributes : ImmutableMap.<String, Attribute>of(), toolchainContext); @@ -1498,12 +1498,13 @@ /** * Sets the fragment that can be legally accessed even when not explicitly declared. */ - Builder setUniversalFragment(Class<? extends BuildConfiguration.Fragment> fragment) { + Builder setUniversalFragments( + ImmutableList<Class<? extends BuildConfiguration.Fragment>> fragments) { // TODO(bazel-team): Add this directly to ConfigurationFragmentPolicy, so we // don't need separate logic specifically for checking this fragment. The challenge is // that we need RuleClassProvider to figure out what this fragment is, and not every // call state that creates ConfigurationFragmentPolicy has access to that. - this.universalFragment = fragment; + this.universalFragments = fragments; return this; }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java new file mode 100644 index 0000000..2e4769e --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java
@@ -0,0 +1,213 @@ +// 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.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.analysis.config.BuildConfiguration; +import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment; +import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory; +import com.google.devtools.build.lib.analysis.config.FragmentOptions; +import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; +import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; +import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; +import com.google.devtools.build.lib.skyframe.serialization.SerializationException; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.lib.util.OS; +import com.google.devtools.build.lib.util.OptionsUtils.PathFragmentConverter; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.common.options.Option; +import com.google.devtools.common.options.OptionDocumentationCategory; +import com.google.devtools.common.options.OptionEffectTag; +import com.google.protobuf.CodedInputStream; +import com.google.protobuf.CodedOutputStream; +import java.io.IOException; +import java.util.Map; +import javax.annotation.Nullable; + +/** A configuration fragment that tells where the shell is. */ +public class ShellConfiguration extends BuildConfiguration.Fragment { + + /** + * A codec for {@link ShellConfiguration}. + * + * <p>It does not handle the Bazel version, but that's fine, because we don't want to serialize + * anything in Bazel. + * + * <p>We cannot use {@code AutoCodec} because the field {@link #actionEnvironment} is a lambda. + * That does not necessarily need to be the case, but it's there in support for + * {@link BuildConfiguration.Fragment#setupActionEnvironment()}, which is slated to be removed. + */ + public static final class Codec implements ObjectCodec<ShellConfiguration> { + @Override + public Class<? extends ShellConfiguration> getEncodedClass() { + return ShellConfiguration.class; + } + + @Override + public void serialize(SerializationContext context, ShellConfiguration obj, + CodedOutputStream codedOut) throws SerializationException, IOException { + context.serialize(obj.shellExecutable, codedOut); + } + + @Override + public ShellConfiguration deserialize(DeserializationContext context, CodedInputStream codedIn) + throws SerializationException, IOException { + PathFragment shellExecutable = context.deserialize(codedIn); + return new ShellConfiguration(shellExecutable, NO_ACTION_ENV.fromOptions(null)); + } + } + + private static final ImmutableMap<OS, PathFragment> OS_SPECIFIC_SHELL = + ImmutableMap.<OS, PathFragment>builder() + .put(OS.WINDOWS, PathFragment.create("c:/tools/msys64/usr/bin/bash.exe")) + .put(OS.FREEBSD, PathFragment.create("/usr/local/bin/bash")) + .build(); + + private final PathFragment shellExecutable; + private final ShellActionEnvironment actionEnvironment; + + public ShellConfiguration(PathFragment shellExecutable, + ShellActionEnvironment actionEnvironment) { + this.shellExecutable = shellExecutable; + this.actionEnvironment = actionEnvironment; + } + + public PathFragment getShellExecutable() { + return shellExecutable; + } + + @Override + public void setupActionEnvironment(Map<String, String> builder) { + actionEnvironment.setupActionEnvironment(this, builder); + } + + /** An option that tells Bazel where the shell is. */ + @AutoCodec(strategy = AutoCodec.Strategy.PUBLIC_FIELDS) + public static class Options extends FragmentOptions { + @Option( + name = "shell_executable", + converter = PathFragmentConverter.class, + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + "Absolute path to the shell executable for Bazel to use. If this is unset, but the " + + "BAZEL_SH environment variable is set on the first Bazel invocation (that starts " + + "up a Bazel server), Bazel uses that. If neither is set, Bazel uses a hard-coded " + + "default path depending on the operating system it runs on (Windows: " + + "c:/tools/msys64/usr/bin/bash.exe, FreeBSD: /usr/local/bin/bash, all others: " + + "/bin/bash). Note that using a shell that is not compatible with bash may lead " + + "to build failures or runtime failures of the generated binaries." + ) + public PathFragment shellExecutable; + + @Override + public Options getHost() { + Options host = (Options) getDefault(); + host.shellExecutable = shellExecutable; + return host; + } + } + + /** + * Encapsulates the contributions of {@link ShellConfiguration} to the action environment. + * + * <p>This is done this way because we need the shell environment to be different between Bazel + * and Blaze, but configuration fragments are handed out based on their classes, thus, + * doing this with inheritance would be difficult. The good old "has-a instead of this-a" pattern. + */ + public interface ShellActionEnvironment { + void setupActionEnvironment(ShellConfiguration configuration, Map<String, String> builder); + } + + /** + * A factory for shell action environments. + * + * <p>This is necessary because the Bazel shell action environment depends on whether we use + * strict action environments or not, but we cannot simply hardcode the dependency on that bit + * here because it doesn't exist in Blaze. Thus, during configuration creation time, we call this + * factory which returns an object, which, when called, updates the actual action environment. + */ + public interface ShellActionEnvironmentFactory { + ShellActionEnvironment fromOptions(BuildOptions options); + } + + /** A {@link ShellConfiguration} that contributes nothing to the action environment. */ + public static final ShellActionEnvironmentFactory NO_ACTION_ENV = + (BuildOptions options) -> (ShellConfiguration config, Map<String, String> builder) -> {}; + + /** the part of {@link ShellConfiguration} that determines where the shell is. */ + public interface ShellExecutableProvider { + PathFragment getShellExecutable(BuildOptions options); + } + + /** A shell executable whose path is hard-coded. */ + public static ShellExecutableProvider hardcodedShellExecutable(String shell) { + return (BuildOptions options) -> PathFragment.create(shell); + } + + /** The loader for {@link ShellConfiguration}. */ + public static class Loader implements ConfigurationFragmentFactory { + private final ShellExecutableProvider shellExecutableProvider; + private final ShellActionEnvironmentFactory actionEnvironmentFactory; + private final ImmutableSet<Class<? extends FragmentOptions>> requiredOptions; + + public Loader(ShellExecutableProvider shellExecutableProvider, + ShellActionEnvironmentFactory actionEnvironmentFactory, + Class<? extends FragmentOptions>... requiredOptions) { + this.shellExecutableProvider = shellExecutableProvider; + this.actionEnvironmentFactory = actionEnvironmentFactory; + this.requiredOptions = ImmutableSet.copyOf(requiredOptions); + } + + @Nullable + @Override + public Fragment create(ConfigurationEnvironment env, BuildOptions buildOptions) { + return new ShellConfiguration( + shellExecutableProvider.getShellExecutable(buildOptions), + actionEnvironmentFactory.fromOptions(buildOptions)); + } + + public static PathFragment determineShellExecutable( + OS os, Options options, PathFragment defaultShell) { + if (options.shellExecutable != null) { + return options.shellExecutable; + } + + // Honor BAZEL_SH env variable for backwards compatibility. + String path = System.getenv("BAZEL_SH"); + if (path != null) { + return PathFragment.create(path); + } + // TODO(ulfjack): instead of using the OS Bazel runs on, we need to use the exec platform, + // which may be different for remote execution. For now, this can be overridden with + // --shell_executable, so at least there's a workaround. + PathFragment result = OS_SPECIFIC_SHELL.get(os); + return result != null ? result : defaultShell; + } + + @Override + public Class<? extends Fragment> creates() { + return ShellConfiguration.class; + } + + @Override + public ImmutableSet<Class<? extends FragmentOptions>> requiredOptions() { + return requiredOptions; + } + } +}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 3d352c4..a89651c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -55,6 +55,7 @@ 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; @@ -678,8 +679,8 @@ AnalysisEnvironment analysisEnvironment, BuildConfiguration configuration, List<Action> paramFileActions) { - ImmutableList<String> executableArgs = - buildExecutableArgs(configuration.getShellExecutable()); + ImmutableList<String> executableArgs = buildExecutableArgs( + configuration.getFragment(ShellConfiguration.class).getShellExecutable()); boolean hasConditionalParamFile = commandLines.stream().anyMatch(c -> c.paramFileInfo != null && !c.paramFileInfo.always()); boolean spillToParamFiles = false;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index 51cbfbc..a068e51 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -165,16 +165,6 @@ } /** - * Returns the shell to be used. - * - * <p>Each configuration instance must have at most one fragment that returns non-null. - */ - @SuppressWarnings("unused") - public PathFragment getShellExecutable() { - return null; - } - - /** * Returns { 'option name': 'alternative default' } entries for options where the * "real default" should be something besides the default specified in the {@link Option} * declaration. @@ -1115,9 +1105,6 @@ // is mutable, so a cached value there could fall out of date when it's updated. private final boolean actionsEnabled; - // TODO(bazel-team): Move this to a configuration fragment. - private final PathFragment shellExecutable; - /** * The global "make variables" such as "$(TARGET_CPU)"; these get applied to all rules analyzed in * this configuration. @@ -1335,8 +1322,6 @@ OutputDirectory.MIDDLEMAN.getRoot( RepositoryName.MAIN, outputDirName, directories, mainRepositoryName); - this.shellExecutable = computeShellExecutable(); - this.actionEnv = setupActionEnvironment(); this.testEnv = setupTestEnvironment(); @@ -1642,13 +1627,6 @@ } /** - * Returns the path to sh. - */ - public PathFragment getShellExecutable() { - return shellExecutable; - } - - /** * Returns a regex-based instrumentation filter instance that used to match label * names to identify targets to be instrumented in the coverage mode. */ @@ -1930,22 +1908,6 @@ } /** - * Collects executables defined by fragments. - */ - private PathFragment computeShellExecutable() { - PathFragment result = null; - - for (Fragment fragment : fragments.values()) { - if (fragment.getShellExecutable() != null) { - Verify.verify(result == null); - result = fragment.getShellExecutable(); - } - } - - return result; - } - - /** * Returns the transition that produces the "artifact owner" for this configuration, or null * if this configuration is its own owner. */
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 336ca69..500a4f5 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -33,6 +33,7 @@ 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; @@ -754,7 +755,7 @@ } public PathFragment getShExecutable() { - return configuration.getShellExecutable(); + return configuration.getFragment(ShellConfiguration.class).getShellExecutable(); } public ImmutableMap<String, String> getLocalShellEnvironment() {
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java deleted file mode 100644 index bde1dcce..0000000 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfiguration.java +++ /dev/null
@@ -1,200 +0,0 @@ -// Copyright 2014 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.bazel.rules; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment; -import com.google.devtools.build.lib.analysis.config.BuildOptions; -import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment; -import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory; -import com.google.devtools.build.lib.analysis.config.FragmentOptions; -import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; -import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -import com.google.devtools.build.lib.util.OS; -import com.google.devtools.build.lib.util.OptionsUtils.PathFragmentConverter; -import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.common.options.Option; -import com.google.devtools.common.options.OptionDocumentationCategory; -import com.google.devtools.common.options.OptionEffectTag; -import java.util.Map; -import javax.annotation.Nullable; - -/** Bazel-specific configuration fragment. */ -@AutoCodec -@Immutable -public class BazelConfiguration extends Fragment { - /** Command-line options. */ - @AutoCodec(strategy = AutoCodec.Strategy.PUBLIC_FIELDS) - public static class Options extends FragmentOptions { - @Option( - name = "experimental_strict_action_env", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, - help = - "If true, Bazel uses an environment with a static value for PATH and does not " - + "inherit LD_LIBRARY_PATH or TMPDIR. Use --action_env=ENV_VARIABLE if you want to " - + "inherit specific environment variables from the client, but note that doing so " - + "can prevent cross-user caching if a shared cache is used." - ) - public boolean useStrictActionEnv; - - @Option( - name = "shell_executable", - converter = PathFragmentConverter.class, - defaultValue = "null", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, - help = - "Absolute path to the shell executable for Bazel to use. If this is unset, but the " - + "BAZEL_SH environment variable is set on the first Bazel invocation (that starts " - + "up a Bazel server), Bazel uses that. If neither is set, Bazel uses a hard-coded " - + "default path depending on the operating system it runs on (Windows: " - + "c:/tools/msys64/usr/bin/bash.exe, FreeBSD: /usr/local/bin/bash, all others: " - + "/bin/bash). Note that using a shell that is not compatible with bash may lead to " - + "build failures or runtime failures of the generated binaries." - ) - public PathFragment shellExecutable; - - @Override - public Options getHost() { - Options host = (Options) getDefault(); - host.useStrictActionEnv = useStrictActionEnv; - host.shellExecutable = shellExecutable; - return host; - } - } - - /** - * Loader for Bazel-specific settings. - */ - public static class Loader implements ConfigurationFragmentFactory { - @Override - public Fragment create(ConfigurationEnvironment env, BuildOptions buildOptions) - throws InvalidConfigurationException { - return new BazelConfiguration(OS.getCurrent(), buildOptions.get(Options.class)); - } - - @Override - public Class<? extends Fragment> creates() { - return BazelConfiguration.class; - } - - @Override - public ImmutableSet<Class<? extends FragmentOptions>> requiredOptions() { - return ImmutableSet.of(Options.class); - } - } - - private static final ImmutableMap<OS, PathFragment> OS_SPECIFIC_SHELL = - ImmutableMap.<OS, PathFragment>builder() - .put(OS.WINDOWS, PathFragment.create("c:/tools/msys64/usr/bin/bash.exe")) - .put(OS.FREEBSD, PathFragment.create("/usr/local/bin/bash")) - .build(); - private static final PathFragment FALLBACK_SHELL = PathFragment.create("/bin/bash"); - - private final OS os; - private final boolean useStrictActionEnv; - private final PathFragment shellExecutable; - - public BazelConfiguration(OS os, Options options) { - this(os, options.useStrictActionEnv, determineShellExecutable(os, options.shellExecutable)); - } - - @AutoCodec.Instantiator - BazelConfiguration(OS os, boolean useStrictActionEnv, PathFragment shellExecutable) { - this.os = os; - this.useStrictActionEnv = useStrictActionEnv; - this.shellExecutable = shellExecutable; - } - - @Override - public PathFragment getShellExecutable() { - return shellExecutable; - } - - @Override - public void setupActionEnvironment(Map<String, String> builder) { - // All entries in the builder that have a value of null inherit the value from the client - // environment, which is only known at execution time - we don't want to bake the client env - // into the configuration since any change to the configuration requires rerunning the full - // analysis phase. - if (useStrictActionEnv) { - builder.put("PATH", pathOrDefault(os, null, getShellExecutable())); - } else if (os == OS.WINDOWS) { - // TODO(ulfjack): We want to add the MSYS root to the PATH, but that prevents us from - // inheriting PATH from the client environment. For now we use System.getenv even though - // that is incorrect. We should enable strict_action_env by default and then remove this - // code, but that change may break Windows users who are relying on the MSYS root being in - // the PATH. - builder.put("PATH", pathOrDefault(os, System.getenv("PATH"), getShellExecutable())); - builder.put("LD_LIBRARY_PATH", null); - } else { - // The previous implementation used System.getenv (which uses the server's environment), and - // fell back to a hard-coded "/bin:/usr/bin" if PATH was not set. - builder.put("PATH", null); - builder.put("LD_LIBRARY_PATH", null); - } - } - - private static PathFragment determineShellExecutable(OS os, PathFragment fromOption) { - if (fromOption != null) { - return fromOption; - } - // Honor BAZEL_SH env variable for backwards compatibility. - String path = System.getenv("BAZEL_SH"); - if (path != null) { - return PathFragment.create(path); - } - // TODO(ulfjack): instead of using the OS Bazel runs on, we need to use the exec platform, which - // may be different for remote execution. For now, this can be overridden with - // --shell_executable, so at least there's a workaround. - PathFragment result = OS_SPECIFIC_SHELL.get(os); - return result != null ? result : FALLBACK_SHELL; - } - - @VisibleForTesting - static String pathOrDefault(OS os, @Nullable String path, @Nullable PathFragment sh) { - // TODO(ulfjack): The default PATH should be set from the exec platform, which may be different - // from the local machine. For now, this can be overridden with --action_env=PATH=<value>, so - // at least there's a workaround. - if (os != OS.WINDOWS) { - return "/bin:/usr/bin"; - } - - // Attempt to compute the MSYS root (the real Windows path of "/") from `sh`. - if (sh != null && sh.getParentDirectory() != null) { - String newPath = sh.getParentDirectory().getPathString(); - if (sh.getParentDirectory().endsWith(PathFragment.create("usr/bin"))) { - newPath += - ";" + sh.getParentDirectory().getParentDirectory().replaceName("bin").getPathString(); - } else if (sh.getParentDirectory().endsWith(PathFragment.create("bin"))) { - newPath += - ";" + sh.getParentDirectory().replaceName("usr").getRelative("bin").getPathString(); - } - newPath = newPath.replace('/', '\\'); - - if (path != null) { - newPath += ";" + path; - } - return newPath; - } else { - return null; - } - } -}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index f3b5110..483fa23 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
@@ -16,12 +16,18 @@ import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.Builder; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.RuleSet; +import com.google.devtools.build.lib.analysis.ShellConfiguration; +import com.google.devtools.build.lib.analysis.ShellConfiguration.ShellActionEnvironmentFactory; +import com.google.devtools.build.lib.analysis.ShellConfiguration.ShellExecutableProvider; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.bazel.rules.android.AndroidNdkRepositoryRule; import com.google.devtools.build.lib.bazel.rules.android.AndroidSdkRepositoryRule; import com.google.devtools.build.lib.bazel.rules.android.BazelAarImportRule; @@ -86,13 +92,76 @@ import com.google.devtools.build.lib.rules.repository.CoreWorkspaceRules; import com.google.devtools.build.lib.rules.repository.NewLocalRepositoryRule; import com.google.devtools.build.lib.rules.test.TestingSupportRules; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.ResourceFileLoader; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.common.options.Option; +import com.google.devtools.common.options.OptionDocumentationCategory; +import com.google.devtools.common.options.OptionEffectTag; import java.io.IOException; +import java.util.Map; +import javax.annotation.Nullable; /** A rule class provider implementing the rules Bazel knows. */ public class BazelRuleClassProvider { public static final String TOOLS_REPOSITORY = "@bazel_tools"; + /** Command-line options. */ + @AutoCodec(strategy = AutoCodec.Strategy.PUBLIC_FIELDS) + public static class StrictActionEnvOptions extends FragmentOptions { + @Option( + name = "experimental_strict_action_env", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + "If true, Bazel uses an environment with a static value for PATH and does not " + + "inherit LD_LIBRARY_PATH or TMPDIR. Use --action_env=ENV_VARIABLE if you want to " + + "inherit specific environment variables from the client, but note that doing so " + + "can prevent cross-user caching if a shared cache is used." + ) + public boolean useStrictActionEnv; + + @Override + public StrictActionEnvOptions getHost() { + StrictActionEnvOptions host = (StrictActionEnvOptions) getDefault(); + host.useStrictActionEnv = useStrictActionEnv; + return host; + } + } + + public static final ShellActionEnvironmentFactory SHELL_ACTION_ENV = (BuildOptions options) -> { + boolean strictActionEnv = options.get( + StrictActionEnvOptions.class).useStrictActionEnv; + return (ShellConfiguration configuration, Map<String, String> builder) -> { + OS os = OS.getCurrent(); + // All entries in the builder that have a value of null inherit the value from the client + // environment, which is only known at execution time - we don't want to bake the client env + // into the configuration since any change to the configuration requires rerunning the full + // analysis phase. + if (!strictActionEnv) { + builder.put("LD_LIBRARY_PATH", null); + } + + if (strictActionEnv) { + builder.put("PATH", pathOrDefault(os, null, configuration.getShellExecutable())); + } else if (os == OS.WINDOWS) { + // TODO(ulfjack): We want to add the MSYS root to the PATH, but that prevents us from + // inheriting PATH from the client environment. For now we use System.getenv even though + // that is incorrect. We should enable strict_action_env by default and then remove this + // code, but that change may break Windows users who are relying on the MSYS root being in + // the PATH. + builder.put("PATH", pathOrDefault( + os, System.getenv("PATH"), configuration.getShellExecutable())); + } else { + // The previous implementation used System.getenv (which uses the server's environment), and + // fell back to a hard-coded "/bin:/usr/bin" if PATH was not set. + builder.put("PATH", null); + } + }; + }; + /** Used by the build encyclopedia generator. */ public static ConfiguredRuleClassProvider create() { ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder(); @@ -107,6 +176,14 @@ } } + private static final PathFragment FALLBACK_SHELL = PathFragment.create("/bin/bash"); + + public static final ShellExecutableProvider SHELL_EXECUTABLE = (BuildOptions options) -> + ShellConfiguration.Loader.determineShellExecutable( + OS.getCurrent(), + options.get(ShellConfiguration.Options.class), + FALLBACK_SHELL); + public static final RuleSet BAZEL_SETUP = new RuleSet() { @Override @@ -117,9 +194,14 @@ .setRunfilesPrefix(Label.DEFAULT_REPOSITORY_DIRECTORY) .setPrerequisiteValidator(new BazelPrerequisiteValidator()); - builder.setUniversalConfigurationFragment(BazelConfiguration.class); - builder.addConfigurationFragment(new BazelConfiguration.Loader()); - builder.addConfigurationOptions(BazelConfiguration.Options.class); + builder.addConfigurationOptions(ShellConfiguration.Options.class); + builder.addConfigurationFragment(new ShellConfiguration.Loader( + SHELL_EXECUTABLE, + SHELL_ACTION_ENV, + ShellConfiguration.Options.class, + StrictActionEnvOptions.class)); + builder.addUniversalConfigurationFragment(ShellConfiguration.class); + builder.addConfigurationOptions(StrictActionEnvOptions.class); builder.addConfigurationOptions(BuildConfiguration.Options.class); } @@ -315,4 +397,34 @@ // This rule set is a little special: it needs to depend on every configuration fragment // that has Make variables, so we put it last. ToolchainRules.INSTANCE); + + @VisibleForTesting + public static String pathOrDefault(OS os, @Nullable String path, @Nullable PathFragment sh) { + // TODO(ulfjack): The default PATH should be set from the exec platform, which may be different + // from the local machine. For now, this can be overridden with --action_env=PATH=<value>, so + // at least there's a workaround. + if (os != OS.WINDOWS) { + return "/bin:/usr/bin"; + } + + // Attempt to compute the MSYS root (the real Windows path of "/") from `sh`. + if (sh != null && sh.getParentDirectory() != null) { + String newPath = sh.getParentDirectory().getPathString(); + if (sh.getParentDirectory().endsWith(PathFragment.create("usr/bin"))) { + newPath += + ";" + sh.getParentDirectory().getParentDirectory().replaceName("bin").getPathString(); + } else if (sh.getParentDirectory().endsWith(PathFragment.create("bin"))) { + newPath += + ";" + sh.getParentDirectory().replaceName("usr").getRelative("bin").getPathString(); + } + newPath = newPath.replace('/', '\\'); + + if (path != null) { + newPath += ";" + path; + } + return newPath; + } else { + return null; + } + } }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java index 561ce8e..cb79332 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
@@ -26,6 +26,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.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction; @@ -37,7 +38,6 @@ import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction.Template; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.analysis.test.TestConfiguration; -import com.google.devtools.build.lib.bazel.rules.BazelConfiguration; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -405,13 +405,13 @@ Substitution.of( "%bash_exe_path%", ruleContext - .getFragment(BazelConfiguration.class) + .getFragment(ShellConfiguration.class) .getShellExecutable() .getPathString()), Substitution.of( "%cygpath_exe_path%", ruleContext - .getFragment(BazelConfiguration.class) + .getFragment(ShellConfiguration.class) .getShellExecutable() .replaceName("cygpath.exe") .getPathString())),
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java index 85de9e9..19de022 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java
@@ -23,6 +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.actions.ExecutableSymlinkAction; import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction; import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction.LaunchInfo; @@ -30,7 +31,6 @@ import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction.Substitution; import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction.Template; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode; -import com.google.devtools.build.lib.bazel.rules.BazelConfiguration; 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; @@ -113,7 +113,7 @@ .addKeyValuePair( "bash_bin_path", ruleContext - .getFragment(BazelConfiguration.class) + .getFragment(ShellConfiguration.class) .getShellExecutable() .getPathString()) .build(); @@ -153,7 +153,7 @@ Substitution.of( "%bash_exe_path%", ruleContext - .getFragment(BazelConfiguration.class) + .getFragment(ShellConfiguration.class) .getShellExecutable() .getPathString())), true));
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 b457eba..07a1942 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
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.TestExecException; import com.google.devtools.build.lib.actions.UserExecException; +import com.google.devtools.build.lib.analysis.ShellConfiguration; import com.google.devtools.build.lib.analysis.config.PerLabelOptions; import com.google.devtools.build.lib.analysis.test.TestActionContext; import com.google.devtools.build.lib.analysis.test.TestResult; @@ -183,7 +184,8 @@ // the --run_under parameter and getCommand only returns the first such token. boolean needsShell = !command.contains("/"); if (needsShell) { - args.add(testAction.getConfiguration().getShellExecutable().getPathString()); + args.add(testAction.getConfiguration().getFragment(ShellConfiguration.class) + .getShellExecutable().getPathString()); args.add("-c"); args.add("\"$@\""); args.add("/bin/sh"); // Sets $0.
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 66b4e67..37ed1f8 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
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.RunfilesSupport; +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.analysis.test.TestProvider; @@ -192,7 +193,7 @@ } private BlazeCommandResult runTargetUnderServer(CommandEnvironment env, - ConfiguredTarget targetToRun, BuildConfiguration configuration, + ConfiguredTarget targetToRun, PathFragment shellExecutable, ConfiguredTarget runUnderTarget, Path workingDir, List<String> commandLineArgs) { RunOptions runOptions = env.getOptions().getOptions(RunOptions.class); List<String> args = computeArgs(env, targetToRun, commandLineArgs); @@ -215,7 +216,7 @@ cmdLine.add(ProcessWrapperUtil.getProcessWrapper(env).getPathString()); } List<String> prettyCmdLine = new ArrayList<>(); - constructCommandLine(cmdLine, prettyCmdLine, env, configuration.getShellExecutable(), + constructCommandLine(cmdLine, prettyCmdLine, env, shellExecutable, targetToRun, runUnderTarget, args); // Add a newline between the blaze output and the binary's output. @@ -420,9 +421,12 @@ } + PathFragment shellExecutable = + configuration.getFragment(ShellConfiguration.class).getShellExecutable(); + if (!runOptions.direct) { - return runTargetUnderServer(env, targetToRun, configuration, runUnderTarget, - runfilesDir, commandLineArgs); + return runTargetUnderServer( + env, targetToRun, shellExecutable, runUnderTarget, runfilesDir, commandLineArgs); } Map<String, String> runEnvironment = new TreeMap<>(); @@ -473,7 +477,7 @@ List<String> prettyCmdLine = new ArrayList<>(); List<String> args = computeArgs(env, targetToRun, commandLineArgs); constructCommandLine(cmdLine, prettyCmdLine, env, - configuration.getShellExecutable(), targetToRun, runUnderTarget, args); + shellExecutable, targetToRun, runUnderTarget, args); } if (runOptions.scriptPath != null) { @@ -492,7 +496,7 @@ ByteString.copyFrom(workingDir.getPathString(), StandardCharsets.ISO_8859_1)); ImmutableList<String> shellCmdLine = ImmutableList.<String>of( - configuration.getShellExecutable().getPathString(), + shellExecutable.getPathString(), "-c", ShellEscaper.escapeJoinAll(cmdLine));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java index 8614666..2d820e7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
@@ -218,8 +218,10 @@ addFragmentsIfNew(builder, getFragmentsFromRequiredOptions(rule)); // Fragments to unconditionally include: - addFragmentIfNew(builder, - ruleClassProvider.getUniversalFragment().asSubclass(BuildConfiguration.Fragment.class)); + for (Class<? extends BuildConfiguration.Fragment> universalFragment : + ruleClassProvider.getUniversalFragments()) { + addFragmentIfNew(builder, universalFragment); + } } return builder.build(errorLoadingTarget);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java index 61076f9..90bc03b 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -1241,7 +1241,7 @@ AnalysisResult res = update("//foo:x"); ConfiguredTarget topLevelTarget = Iterables.getOnlyElement(res.getTargetsToBuild()); assertThat(getConfiguration(topLevelTarget).getFragmentsMap().keySet()) - .containsExactly(ruleClassProvider.getUniversalFragment()); + .containsExactlyElementsIn(ruleClassProvider.getUniversalFragments()); } /**
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ShellConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ShellConfigurationTest.java new file mode 100644 index 0000000..8d077ad --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/ShellConfigurationTest.java
@@ -0,0 +1,66 @@ +// 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 static com.google.common.truth.Truth.assertThat; + +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.util.OS; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.common.options.Options; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Unit tests for {@link ShellConfiguration}. + */ +@RunWith(JUnit4.class) +public class ShellConfigurationTest extends BuildViewTestCase { + @Test + public void getShellExecutableUnset() { + assertThat(determineShellExecutable(OS.LINUX, null)) + .isEqualTo(PathFragment.create("/bin/bash")); + assertThat(determineShellExecutable(OS.FREEBSD, null)) + .isEqualTo(PathFragment.create("/usr/local/bin/bash")); + assertThat(determineShellExecutable(OS.WINDOWS, null)) + .isEqualTo(PathFragment.create("c:/tools/msys64/usr/bin/bash.exe")); + } + + @Test + public void getShellExecutableIfSet() { + PathFragment binBash = PathFragment.create("/bin/bash"); + assertThat(determineShellExecutable(OS.LINUX, binBash)) + .isEqualTo(PathFragment.create("/bin/bash")); + assertThat(determineShellExecutable(OS.FREEBSD, binBash)) + .isEqualTo(PathFragment.create("/bin/bash")); + assertThat(determineShellExecutable(OS.WINDOWS, binBash)) + .isEqualTo(PathFragment.create("/bin/bash")); + } + + @Test + public void optionsAlsoApplyToHost() { + ShellConfiguration.Options o = Options.getDefaults(ShellConfiguration.Options.class); + o.shellExecutable = PathFragment.create("/my/shell/binary"); + ShellConfiguration.Options h = o.getHost(); + assertThat(h.shellExecutable).isEqualTo(PathFragment.create("/my/shell/binary")); + } + + private static PathFragment determineShellExecutable(OS os, PathFragment executableOption) { + ShellConfiguration.Options options = Options.getDefaults(ShellConfiguration.Options.class); + options.shellExecutable = executableOption; + return ShellConfiguration.Loader.determineShellExecutable( + os, options, PathFragment.create("/bin/bash")); + } +}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index 34450da..eb08224 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -17,9 +17,11 @@ import com.google.common.collect.Iterables; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.PlatformConfigurationLoader; +import com.google.devtools.build.lib.analysis.ShellConfiguration; import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory; import com.google.devtools.build.lib.analysis.util.AnalysisMock; -import com.google.devtools.build.lib.bazel.rules.BazelConfiguration; +import com.google.devtools.build.lib.bazel.rules.BazelRuleClassProvider; +import com.google.devtools.build.lib.bazel.rules.BazelRuleClassProvider.StrictActionEnvOptions; import com.google.devtools.build.lib.bazel.rules.python.BazelPythonConfiguration; import com.google.devtools.build.lib.packages.util.BazelMockCcSupport; import com.google.devtools.build.lib.packages.util.MockCcSupport; @@ -280,8 +282,12 @@ @Override public List<ConfigurationFragmentFactory> getDefaultConfigurationFragmentFactories() { return ImmutableList.<ConfigurationFragmentFactory>of( - new BazelConfiguration.Loader(), new CppConfigurationLoader(CpuTransformer.IDENTITY), + new ShellConfiguration.Loader( + BazelRuleClassProvider.SHELL_EXECUTABLE, + BazelRuleClassProvider.SHELL_ACTION_ENV, + ShellConfiguration.Options.class, + StrictActionEnvOptions.class), new PythonConfigurationLoader(), new BazelPythonConfiguration.Loader(), new JavaConfigurationLoader(),
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationTest.java deleted file mode 100644 index f4ce424..0000000 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationTest.java +++ /dev/null
@@ -1,90 +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.bazel.rules; - -import static com.google.common.truth.Truth.assertThat; -import static com.google.devtools.build.lib.bazel.rules.BazelConfiguration.pathOrDefault; - -import com.google.devtools.build.lib.util.OS; -import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.common.options.Options; -import java.util.HashMap; -import java.util.Map; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** - * Tests for {@link BazelConfiguration}. - */ -@RunWith(JUnit4.class) -public class BazelConfigurationTest { - @Test - public void getShellExecutableUnset() { - BazelConfiguration.Options o = Options.getDefaults(BazelConfiguration.Options.class); - assertThat(new BazelConfiguration(OS.LINUX, o).getShellExecutable()) - .isEqualTo(PathFragment.create("/bin/bash")); - assertThat(new BazelConfiguration(OS.FREEBSD, o).getShellExecutable()) - .isEqualTo(PathFragment.create("/usr/local/bin/bash")); - assertThat(new BazelConfiguration(OS.WINDOWS, o).getShellExecutable()) - .isEqualTo(PathFragment.create("c:/tools/msys64/usr/bin/bash.exe")); - } - - @Test - public void getShellExecutableIfSet() { - BazelConfiguration.Options o = Options.getDefaults(BazelConfiguration.Options.class); - o.shellExecutable = PathFragment.create("/bin/bash"); - assertThat(new BazelConfiguration(OS.LINUX, o).getShellExecutable()) - .isEqualTo(PathFragment.create("/bin/bash")); - assertThat(new BazelConfiguration(OS.FREEBSD, o).getShellExecutable()) - .isEqualTo(PathFragment.create("/bin/bash")); - assertThat(new BazelConfiguration(OS.WINDOWS, o).getShellExecutable()) - .isEqualTo(PathFragment.create("/bin/bash")); - } - - @Test - public void strictActionEnv() { - BazelConfiguration.Options options = Options.getDefaults(BazelConfiguration.Options.class); - options.useStrictActionEnv = true; - BazelConfiguration config = new BazelConfiguration(OS.LINUX, options); - Map<String, String> env = new HashMap<>(); - config.setupActionEnvironment(env); - assertThat(env).containsEntry("PATH", "/bin:/usr/bin"); - } - - @Test - public void pathOrDefaultOnLinux() { - assertThat(pathOrDefault(OS.LINUX, null, null)).isEqualTo("/bin:/usr/bin"); - assertThat(pathOrDefault(OS.LINUX, "/not/bin", null)).isEqualTo("/bin:/usr/bin"); - } - - @Test - public void pathOrDefaultOnWindows() { - assertThat(pathOrDefault(OS.WINDOWS, null, null)).isNull(); - assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", null)).isNull(); - assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", PathFragment.create("D:/foo/shell"))) - .isEqualTo("D:\\foo;C:/mypath"); - } - - @Test - public void optionsAlsoApplyToHost() { - BazelConfiguration.Options o = Options.getDefaults(BazelConfiguration.Options.class); - o.shellExecutable = PathFragment.create("/my/shell/binary"); - o.useStrictActionEnv = true; - BazelConfiguration.Options h = o.getHost(); - assertThat(h.shellExecutable).isEqualTo(PathFragment.create("/my/shell/binary")); - assertThat(h.useStrictActionEnv).isTrue(); - } -}
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java index d7bd55f..1c1fe3a 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java
@@ -16,19 +16,30 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.devtools.build.lib.bazel.rules.BazelRuleClassProvider.SHELL_ACTION_ENV; +import static com.google.devtools.build.lib.bazel.rules.BazelRuleClassProvider.pathOrDefault; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.RuleSet; +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.BuildOptions; import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory; import com.google.devtools.build.lib.analysis.config.FragmentOptions; +import com.google.devtools.build.lib.bazel.rules.BazelRuleClassProvider.StrictActionEnvOptions; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.rules.config.ConfigRules; import com.google.devtools.build.lib.rules.core.CoreRules; import com.google.devtools.build.lib.rules.cpp.transitions.LipoDataTransitionRuleSet; import com.google.devtools.build.lib.rules.repository.CoreWorkspaceRules; +import com.google.devtools.build.lib.util.OS; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.common.options.Options; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import org.junit.Test; import org.junit.runner.RunWith; @@ -153,4 +164,45 @@ public void toolchainConsistency() { checkModule(ToolchainRules.INSTANCE); } + + @Test + public void strictActionEnv() throws Exception { + if (OS.getCurrent() == OS.WINDOWS) { + return; + } + + BuildOptions options = BuildOptions.of( + ImmutableList.of(StrictActionEnvOptions.class), + "--experimental_strict_action_env"); + + ShellConfiguration configuration = new ShellConfiguration( + PathFragment.create("/bin/bash"), + SHELL_ACTION_ENV.fromOptions(options)); + Map<String, String> env = new HashMap<>(); + configuration.setupActionEnvironment(env); + assertThat(env).containsEntry("PATH", "/bin:/usr/bin"); + } + + @Test + public void pathOrDefaultOnLinux() { + assertThat(pathOrDefault(OS.LINUX, null, null)).isEqualTo("/bin:/usr/bin"); + assertThat(pathOrDefault(OS.LINUX, "/not/bin", null)).isEqualTo("/bin:/usr/bin"); + } + + @Test + public void pathOrDefaultOnWindows() { + assertThat(pathOrDefault(OS.WINDOWS, null, null)).isNull(); + assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", null)).isNull(); + assertThat(pathOrDefault(OS.WINDOWS, "C:/mypath", PathFragment.create("D:/foo/shell"))) + .isEqualTo("D:\\foo;C:/mypath"); + } + + @Test + public void optionsAlsoApplyToHost() { + StrictActionEnvOptions o = Options.getDefaults( + StrictActionEnvOptions.class); + o.useStrictActionEnv = true; + StrictActionEnvOptions h = o.getHost(); + assertThat(h.useStrictActionEnv).isTrue(); + } }
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java index e0f1689..a70e694 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.ShellConfiguration; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.configuredtargets.FileConfiguredTarget; import com.google.devtools.build.lib.analysis.util.AnalysisMock; @@ -188,8 +189,8 @@ assertThat(shellAction.getOutputs()).containsExactly(messageArtifact); String expected = "echo \"Hello, world.\" >" + messageArtifact.getExecPathString(); - assertThat(shellAction.getArguments().get(0)) - .isEqualTo(targetConfig.getShellExecutable().getPathString()); + assertThat(shellAction.getArguments().get(0)).isEqualTo( + targetConfig.getFragment(ShellConfiguration.class).getShellExecutable().getPathString()); assertThat(shellAction.getArguments().get(1)).isEqualTo("-c"); assertCommandEquals(expected, shellAction.getArguments().get(2)); }