From 8626623106fc0f9f83781990f7c235074926d196 Mon Sep 17 00:00:00 2001 From: lberki Date: Wed, 11 Apr 2018 00:33:42 -0700 Subject: Remove BuildConfiguration.Fragment#setupActionEnvironment(). This is accomplished by moving it to ConfiguredRuleClassProvider. This also suggests a neat way to get rid of logic in ShellConfiguration.Loader() by moving the determination of the shell executable, there, too, but not in this change. RELNOTES: None. PiperOrigin-RevId: 192411609 --- .../lib/analysis/ConfiguredRuleClassProvider.java | 18 +++++ .../build/lib/analysis/ShellConfiguration.java | 84 +------------------- .../lib/analysis/config/BuildConfiguration.java | 38 ++------- .../lib/bazel/rules/BazelRuleClassProvider.java | 90 ++++++++++++---------- .../lib/skyframe/BuildConfigurationFunction.java | 4 + 5 files changed, 84 insertions(+), 150 deletions(-) (limited to 'src/main') 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 8cd2b7bef3..cad348ba0b 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 @@ -27,6 +27,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; +import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment; @@ -237,6 +238,8 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { ImmutableList.>builder().addAll(SkylarkModules.MODULES); private ImmutableList.Builder nativeProviders = ImmutableList.builder(); private Set reservedActionMnemonics = new TreeSet<>(); + private BuildConfiguration.ActionEnvironmentProvider actionEnvironmentProvider = + (BuildOptions options) -> ActionEnvironment.EMPTY; private ImmutableBiMap.Builder> registeredSkylarkProviders = ImmutableBiMap.builder(); @@ -371,6 +374,12 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { return this; } + public Builder setActionEnvironmentProvider( + BuildConfiguration.ActionEnvironmentProvider actionEnvironmentProvider) { + this.actionEnvironmentProvider = actionEnvironmentProvider; + return this; + } + /** * Sets the C++ LIPO data transition, as defined in {@link * com.google.devtools.build.lib.rules.cpp.transitions.DisableLipoTransition}. @@ -471,6 +480,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { skylarkAccessibleTopLevels.build(), skylarkModules.build(), ImmutableSet.copyOf(reservedActionMnemonics), + actionEnvironmentProvider, nativeProviders.build()); } @@ -579,6 +589,8 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { private final ImmutableSet reservedActionMnemonics; + private final BuildConfiguration.ActionEnvironmentProvider actionEnvironmentProvider; + private final ImmutableList nativeProviders; private final ImmutableMap> configurationFragmentMap; @@ -601,6 +613,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { ImmutableMap skylarkAccessibleJavaClasses, ImmutableList> skylarkModules, ImmutableSet reservedActionMnemonics, + BuildConfiguration.ActionEnvironmentProvider actionEnvironmentProvider, ImmutableList nativeProviders) { this.preludeLabel = preludeLabel; this.runfilesPrefix = runfilesPrefix; @@ -618,6 +631,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { this.prerequisiteValidator = prerequisiteValidator; this.globals = createGlobals(skylarkAccessibleJavaClasses, skylarkModules); this.reservedActionMnemonics = reservedActionMnemonics; + this.actionEnvironmentProvider = actionEnvironmentProvider; this.nativeProviders = nativeProviders; this.configurationFragmentMap = createFragmentMap(configurationFragmentFactories); } @@ -832,6 +846,10 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { return reservedActionMnemonics; } + public BuildConfiguration.ActionEnvironmentProvider getActionEnvironmentProvider() { + return actionEnvironmentProvider; + } + /** * Returns all registered {@link NativeProvider} instances, i.e. all built-in provider types that * are based on {@link Provider} rather than {@link TransitiveInfoProvider}. 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 index 2e4769e506..df2e5f1d5b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java @@ -21,10 +21,6 @@ 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; @@ -32,45 +28,11 @@ 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. */ +@AutoCodec public class ShellConfiguration extends BuildConfiguration.Fragment { - - /** - * A codec for {@link ShellConfiguration}. - * - *

It does not handle the Bazel version, but that's fine, because we don't want to serialize - * anything in Bazel. - * - *

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 { - @Override - public Class 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_SPECIFIC_SHELL = ImmutableMap.builder() .put(OS.WINDOWS, PathFragment.create("c:/tools/msys64/usr/bin/bash.exe")) @@ -78,23 +40,15 @@ public class ShellConfiguration extends BuildConfiguration.Fragment { .build(); private final PathFragment shellExecutable; - private final ShellActionEnvironment actionEnvironment; - public ShellConfiguration(PathFragment shellExecutable, - ShellActionEnvironment actionEnvironment) { + public ShellConfiguration(PathFragment shellExecutable) { this.shellExecutable = shellExecutable; - this.actionEnvironment = actionEnvironment; } public PathFragment getShellExecutable() { return shellExecutable; } - @Override - public void setupActionEnvironment(Map 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 { @@ -123,33 +77,6 @@ public class ShellConfiguration extends BuildConfiguration.Fragment { } } - /** - * Encapsulates the contributions of {@link ShellConfiguration} to the action environment. - * - *

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 builder); - } - - /** - * A factory for shell action environments. - * - *

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 builder) -> {}; - /** the part of {@link ShellConfiguration} that determines where the shell is. */ public interface ShellExecutableProvider { PathFragment getShellExecutable(BuildOptions options); @@ -163,23 +90,18 @@ public class ShellConfiguration extends BuildConfiguration.Fragment { /** The loader for {@link ShellConfiguration}. */ public static class Loader implements ConfigurationFragmentFactory { private final ShellExecutableProvider shellExecutableProvider; - private final ShellActionEnvironmentFactory actionEnvironmentFactory; private final ImmutableSet> requiredOptions; public Loader(ShellExecutableProvider shellExecutableProvider, - ShellActionEnvironmentFactory actionEnvironmentFactory, Class... 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)); + return new ShellConfiguration(shellExecutableProvider.getShellExecutable(buildOptions)); } public static PathFragment determineShellExecutable( 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 b2b77314b3..2a0faf087b 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 @@ -123,6 +123,11 @@ public class BuildConfiguration { private static final Interner, Fragment>> fragmentsInterner = BlazeInterners.newWeakInterner(); + /** Compute the default shell environment for actions from the command line options. */ + public interface ActionEnvironmentProvider { + ActionEnvironment getActionEnvironment(BuildOptions options); + } + /** * An interface for language-specific configurations. * @@ -156,14 +161,6 @@ public class BuildConfiguration { return null; } - /** - * Add items to the action environment. - * - * @param builder the map to add environment variables to - */ - public void setupActionEnvironment(Map builder) { - } - /** * Returns { 'option name': 'alternative default' } entries for options where the * "real default" should be something besides the default specified in the {@link Option} @@ -1218,27 +1215,6 @@ public class BuildConfiguration { }); } - /** - * Compute the shell environment, which, at configuration level, is a pair consisting of the - * statically set environment variables with their values and the set of environment variables to - * be inherited from the client environment. - */ - private ActionEnvironment setupActionEnvironment() { - // We make a copy first to remove duplicate entries; last one wins. - Map actionEnv = new HashMap<>(); - // TODO(ulfjack): Remove all env variables from configuration fragments. - for (Fragment fragment : fragments.values()) { - fragment.setupActionEnvironment(actionEnv); - } - // Shell environment variables specified via options take precedence over the - // ones inherited from the fragments. In the long run, these fragments will - // be replaced by appropriate default rc files anyway. - for (Map.Entry entry : options.actionEnvironment) { - actionEnv.put(entry.getKey(), entry.getValue()); - } - return ActionEnvironment.split(actionEnv); - } - /** * Compute the test environment, which, at configuration level, is a pair consisting of the * statically set environment variables with their values and the set of environment variables to @@ -1265,6 +1241,7 @@ public class BuildConfiguration { BuildOptions buildOptions, BuildOptions.OptionsDiffForReconstruction buildOptionsDiff, ImmutableSet reservedActionMnemonics, + ActionEnvironment actionEnvironment, String repositoryName) { this.directories = directories; this.fragments = makeFragmentsMap(fragmentsMap); @@ -1314,7 +1291,7 @@ public class BuildConfiguration { OutputDirectory.MIDDLEMAN.getRoot( RepositoryName.MAIN, outputDirName, directories, mainRepositoryName); - this.actionEnv = setupActionEnvironment(); + this.actionEnv = actionEnvironment; this.testEnv = setupTestEnvironment(); @@ -1367,6 +1344,7 @@ public class BuildConfiguration { options, BuildOptions.diffForReconstruction(defaultBuildOptions, options), reservedActionMnemonics, + actionEnv, mainRepositoryName.strippedName()); return newConfig; } 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 3d169694b7..389ac44ff1 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 @@ -19,13 +19,14 @@ 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.actions.ActionEnvironment; 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.BuildConfiguration.ActionEnvironmentProvider; 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; @@ -100,6 +101,7 @@ import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; import java.io.IOException; import java.util.Map; +import java.util.TreeMap; import javax.annotation.Nullable; /** A rule class provider implementing the rules Bazel knows. */ @@ -130,35 +132,53 @@ public class BazelRuleClassProvider { } } - public static final ShellActionEnvironmentFactory SHELL_ACTION_ENV = (BuildOptions options) -> { - boolean strictActionEnv = options.get( - StrictActionEnvOptions.class).useStrictActionEnv; - return (ShellConfiguration configuration, Map 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); - } + private static final PathFragment FALLBACK_SHELL = PathFragment.create("/bin/bash"); - 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); - } - }; + public static final ShellExecutableProvider SHELL_EXECUTABLE = (BuildOptions options) -> + ShellConfiguration.Loader.determineShellExecutable( + OS.getCurrent(), + options.get(ShellConfiguration.Options.class), + FALLBACK_SHELL); + + public static final ActionEnvironmentProvider SHELL_ACTION_ENV = (BuildOptions options) -> { + boolean strictActionEnv = options.get(StrictActionEnvOptions.class).useStrictActionEnv; + OS os = OS.getCurrent(); + PathFragment shellExecutable = SHELL_EXECUTABLE.getShellExecutable(options); + TreeMap env = new TreeMap<>(); + + // 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) { + env.put("LD_LIBRARY_PATH", null); + } + + if (strictActionEnv) { + env.put("PATH", pathOrDefault(os, null, shellExecutable)); + } 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. + env.put("PATH", pathOrDefault( + os, System.getenv("PATH"), shellExecutable)); + } 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. + env.put("PATH", null); + } + + // Shell environment variables specified via options take precedence over the + // ones inherited from the fragments. In the long run, these fragments will + // be replaced by appropriate default rc files anyway. + for (Map.Entry entry : + options.get(BuildConfiguration.Options.class).actionEnvironment) { + env.put(entry.getKey(), entry.getValue()); + } + + return ActionEnvironment.split(env); }; /** Used by the build encyclopedia generator. */ @@ -175,14 +195,6 @@ public class BazelRuleClassProvider { } } - 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 @@ -191,12 +203,12 @@ public class BazelRuleClassProvider { .setPrelude("//tools/build_rules:prelude_bazel") .setNativeLauncherLabel("//tools/launcher:launcher") .setRunfilesPrefix(Label.DEFAULT_REPOSITORY_DIRECTORY) - .setPrerequisiteValidator(new BazelPrerequisiteValidator()); + .setPrerequisiteValidator(new BazelPrerequisiteValidator()) + .setActionEnvironmentProvider(SHELL_ACTION_ENV); builder.addConfigurationOptions(ShellConfiguration.Options.class); builder.addConfigurationFragment(new ShellConfiguration.Loader( SHELL_EXECUTABLE, - SHELL_ACTION_ENV, ShellConfiguration.Options.class, StrictActionEnvOptions.class)); builder.addUniversalConfigurationFragment(ShellConfiguration.class); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java index 093cbb015a..9edebb8735 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java @@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.analysis.config.BuildConfiguration.F import com.google.common.collect.ClassToInstanceMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.MutableClassToInstanceMap; +import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; @@ -77,6 +78,8 @@ public class BuildConfigurationFunction implements SkyFunction { } BuildOptions options = defaultBuildOptions.applyDiff(key.getOptionsDiff()); + ActionEnvironment actionEnvironment = + ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(options); BuildConfiguration config = new BuildConfiguration( @@ -85,6 +88,7 @@ public class BuildConfigurationFunction implements SkyFunction { options, key.getOptionsDiff(), ruleClassProvider.getReservedActionMnemonics(), + actionEnvironment, workspaceNameValue.getName()); return new BuildConfigurationValue(config); } -- cgit v1.2.3