diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index de26fe50b3e72b..45c4e7f0abbdcb 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -96,6 +96,7 @@ import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.CommandEnvironment; +import com.google.devtools.build.lib.runtime.CommonCommandOptions; import com.google.devtools.build.lib.runtime.InfoItem; import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; @@ -151,6 +152,8 @@ public class BazelRepositoryModule extends BlazeModule { private final AtomicBoolean isFetch = new AtomicBoolean(false); private final StarlarkRepositoryFunction starlarkRepositoryFunction; private final RepositoryCache repositoryCache = new RepositoryCache(); + private final MutableSupplier> repoEnvironmentSupplier = + new MutableSupplier<>(); private final MutableSupplier> clientEnvironmentSupplier = new MutableSupplier<>(); private ImmutableMap overrides = ImmutableMap.of(); @@ -251,12 +254,14 @@ public void workspaceInit( repositoryHandlers, starlarkRepositoryFunction, isFetch, + repoEnvironmentSupplier, clientEnvironmentSupplier, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, repositoryCache.getRepoContentsCache()); singleExtensionEvalFunction = - new SingleExtensionEvalFunction(directories, clientEnvironmentSupplier); + new SingleExtensionEvalFunction( + directories, repoEnvironmentSupplier, clientEnvironmentSupplier); if (builtinModules == null) { builtinModules = ModuleFileFunction.getBuiltinModules(directories.getEmbeddedBinariesRoot()); @@ -332,6 +337,12 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { this.yankedVersionsFunction.setDownloadManager(downloadManager); this.vendorCommand.setDownloadManager(downloadManager); + CommonCommandOptions commandOptions = env.getOptions().getOptions(CommonCommandOptions.class); + if (commandOptions.useStrictRepoEnv) { + repoEnvironmentSupplier.set(env.getRepoEnvFromOptions()); + } else { + repoEnvironmentSupplier.set(env.getRepoEnv()); + } clientEnvironmentSupplier.set(env.getRepoEnv()); PackageOptions pkgOptions = env.getOptions().getOptions(PackageOptions.class); isFetch.set(pkgOptions != null && pkgOptions.fetch); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java index d2a9f8e785ec2d..d2135a6ba4aa82 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java @@ -55,7 +55,8 @@ protected ModuleExtensionContext( Path workingDirectory, BlazeDirectories directories, Environment env, - Map envVariables, + Map repoEnvVariables, + Map clientEnvVariables, DownloadManager downloadManager, double timeoutScaling, @Nullable ProcessWrapper processWrapper, @@ -69,7 +70,8 @@ protected ModuleExtensionContext( workingDirectory, directories, env, - envVariables, + repoEnvVariables, + clientEnvVariables, downloadManager, timeoutScaling, processWrapper, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java index dc21be0dd5d37f..ac6deab5150f4b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java @@ -73,6 +73,7 @@ final class RegularRunnableExtension implements RunnableExtension { private final ModuleExtension extension; private final ImmutableMap> staticEnvVars; private final BlazeDirectories directories; + private final Supplier> repoEnvironmentSupplier; private final Supplier> clientEnvironmentSupplier; private final double timeoutScaling; @Nullable private final ProcessWrapper processWrapper; @@ -84,6 +85,7 @@ final class RegularRunnableExtension implements RunnableExtension { ModuleExtension extension, ImmutableMap> staticEnvVars, BlazeDirectories directories, + Supplier> repoEnvironmentSupplier, Supplier> clientEnvironmentSupplier, double timeoutScaling, @Nullable ProcessWrapper processWrapper, @@ -93,6 +95,7 @@ final class RegularRunnableExtension implements RunnableExtension { this.extension = extension; this.staticEnvVars = staticEnvVars; this.directories = directories; + this.repoEnvironmentSupplier = repoEnvironmentSupplier; this.clientEnvironmentSupplier = clientEnvironmentSupplier; this.timeoutScaling = timeoutScaling; this.processWrapper = processWrapper; @@ -141,6 +144,7 @@ static RegularRunnableExtension load( StarlarkSemantics starlarkSemantics, Environment env, BlazeDirectories directories, + Supplier> repoEnvironmentSupplier, Supplier> clientEnvironmentSupplier, double timeoutScaling, @Nullable ProcessWrapper processWrapper, @@ -176,16 +180,17 @@ static RegularRunnableExtension load( SpellChecker.didYouMean(extensionId.extensionName(), exportedExtensions)); } - ImmutableMap> envVars = + ImmutableMap> staticEnvVars = RepositoryFunction.getEnvVarValues(env, ImmutableSet.copyOf(extension.envVariables())); - if (envVars == null) { + if (staticEnvVars == null) { return null; } return new RegularRunnableExtension( bzlLoadValue, extension, - envVars, + staticEnvVars, directories, + repoEnvironmentSupplier, clientEnvironmentSupplier, timeoutScaling, processWrapper, @@ -361,6 +366,7 @@ private ModuleExtensionContext createContext( workingDirectory, directories, env, + repoEnvironmentSupplier.get(), clientEnvironmentSupplier.get(), downloadManager, timeoutScaling, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 6e6266947ec062..ee2bc90155dcc7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -63,6 +63,7 @@ */ public class SingleExtensionEvalFunction implements SkyFunction { private final BlazeDirectories directories; + private final Supplier> repoEnvironmentSupplier; private final Supplier> clientEnvironmentSupplier; private double timeoutScaling = 1.0; @@ -71,8 +72,11 @@ public class SingleExtensionEvalFunction implements SkyFunction { @Nullable private DownloadManager downloadManager = null; public SingleExtensionEvalFunction( - BlazeDirectories directories, Supplier> clientEnvironmentSupplier) { + BlazeDirectories directories, + Supplier> repoEnvironmentSupplier, + Supplier> clientEnvironmentSupplier) { this.directories = directories; + this.repoEnvironmentSupplier = repoEnvironmentSupplier; this.clientEnvironmentSupplier = clientEnvironmentSupplier; } @@ -127,6 +131,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) starlarkSemantics, env, directories, + repoEnvironmentSupplier, clientEnvironmentSupplier, timeoutScaling, processWrapper, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index a964fe72d94e51..47cdb8f228c06a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -158,7 +158,8 @@ private interface AsyncTask extends SilentCloseable { protected final Path workingDirectory; protected final BlazeDirectories directories; protected final Environment env; - protected final ImmutableMap envVariables; + protected final ImmutableMap repoEnvVariables; + protected final ImmutableMap clientEnvVariables; private final StarlarkOS osObject; protected final DownloadManager downloadManager; protected final double timeoutScaling; @@ -179,7 +180,8 @@ protected StarlarkBaseExternalContext( Path workingDirectory, BlazeDirectories directories, Environment env, - Map envVariables, + Map repoEnvVariables, + Map clientEnvVariables, DownloadManager downloadManager, double timeoutScaling, @Nullable ProcessWrapper processWrapper, @@ -190,8 +192,9 @@ protected StarlarkBaseExternalContext( this.workingDirectory = workingDirectory; this.directories = directories; this.env = env; - this.envVariables = ImmutableMap.copyOf(envVariables); - this.osObject = new StarlarkOS(this.envVariables); + this.repoEnvVariables = ImmutableMap.copyOf(repoEnvVariables); + this.clientEnvVariables = ImmutableMap.copyOf(clientEnvVariables); + this.osObject = new StarlarkOS(this.repoEnvVariables); this.downloadManager = downloadManager; this.timeoutScaling = timeoutScaling; this.processWrapper = processWrapper; @@ -823,7 +826,7 @@ public Object download( canonicalId, Optional.empty(), outputPath.getPath(), - envVariables, + clientEnvVariables, identifyingStringForLogging, downloadPhaser, // The repo rule may modify the file after the download, so we cannot guarantee that @@ -1063,7 +1066,7 @@ public StructImpl downloadAndExtract( canonicalId, Optional.of(type), downloadDirectory, - envVariables, + clientEnvVariables, identifyingStringForLogging, downloadPhaser, // The archive is not going to be modified and not accessible to the user, so its safe @@ -1430,15 +1433,15 @@ private static T nullIfNone(Object object, Class type) { @Nullable public String getEnvironmentValue(String name, Object defaultValue) throws InterruptedException, NeedsSkyframeRestartException { - // Must look up via AEF, rather than solely copy from `this.envVariables`, in order to + // Must look up via AEF, rather than solely copy from `this.repoEnvVariables`, in order to // establish a SkyKey dependency relationship. if (env.getValue(ActionEnvironmentFunction.key(name)) == null) { throw new NeedsSkyframeRestartException(); } - // However, to account for --repo_env we take the value from `this.envVariables`. + // However, to account for --repo_env we take the value from `this.repoEnvVariables`. // See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 . - String envVarValue = envVariables.get(name); + String envVarValue = repoEnvVariables.get(name); accumulatedEnvKeys.add(name); return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class); } @@ -1908,17 +1911,17 @@ public StarlarkExecutionResult execute( validateExecuteArguments(arguments); int timeout = Starlark.toInt(timeoutI, "timeout"); - Map forceEnvVariablesRaw = + Map forceRepoEnvVariablesRaw = Dict.cast(uncheckedEnvironment, String.class, Object.class, "environment"); - Map forceEnvVariables = new LinkedHashMap<>(); - Set removeEnvVariables = new LinkedHashSet<>(); - for (Map.Entry entry : forceEnvVariablesRaw.entrySet()) { + Map forceRepoEnvVariables = new LinkedHashMap<>(); + Set removeRepoEnvVariables = new LinkedHashSet<>(); + for (Map.Entry entry : forceRepoEnvVariablesRaw.entrySet()) { String key = entry.getKey(); Object value = entry.getValue(); if (value == Starlark.NONE) { - removeEnvVariables.add(key); + removeRepoEnvVariables.add(key); } else if (value instanceof String s) { - forceEnvVariables.put(key, s); + forceRepoEnvVariables.put(key, s); } else { throw Starlark.errorf("environment values must be strings or None, got %s", value); } @@ -1927,7 +1930,8 @@ public StarlarkExecutionResult execute( if (canExecuteRemote()) { // Remote execution only sees the explicitly set environment variables, so removing env vars // isn't necessary. - return executeRemote(arguments, timeout, forceEnvVariables, quiet, overrideWorkingDirectory); + return executeRemote( + arguments, timeout, forceRepoEnvVariables, quiet, overrideWorkingDirectory); } // Execute on the local/host machine @@ -1946,8 +1950,8 @@ public StarlarkExecutionResult execute( WorkspaceRuleEvent.newExecuteEvent( args, timeout, - Maps.filterKeys(envVariables, k -> !removeEnvVariables.contains(k)), - forceEnvVariables, + Maps.filterKeys(repoEnvVariables, k -> !removeRepoEnvVariables.contains(k)), + forceRepoEnvVariables, workingDirectory.getPathString(), quiet, identifyingStringForLogging, @@ -1979,8 +1983,8 @@ public StarlarkExecutionResult execute( return StarlarkExecutionResult.builder(osObject.getEnvironmentVariables()) .addArguments(args) .setDirectory(workingDirectoryPath.getPathFile()) - .addEnvironmentVariables(forceEnvVariables) - .removeEnvironmentVariables(removeEnvVariables) + .addEnvironmentVariables(forceRepoEnvVariables) + .removeEnvironmentVariables(removeRepoEnvVariables) .setTimeout(timeoutMillis) .setQuiet(quiet) .execute(); @@ -2244,7 +2248,7 @@ public StarlarkPath which(String program, StarlarkThread thread) throws EvalExce @Nullable private StarlarkPath findCommandOnPath(String program) throws IOException { - String pathEnvVariable = envVariables.get("PATH"); + String pathEnvVariable = repoEnvVariables.get("PATH"); if (pathEnvVariable == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 4932ab393f9df2..8b987a9f825132 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -96,7 +96,8 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext { Path outputDirectory, IgnoredSubdirectories ignoredSubdirectories, Environment environment, - ImmutableMap env, + ImmutableMap repoEnvVariables, + ImmutableMap clientEnvVariables, DownloadManager downloadManager, double timeoutScaling, @Nullable ProcessWrapper processWrapper, @@ -109,7 +110,8 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext { outputDirectory, directories, environment, - env, + repoEnvVariables, + clientEnvVariables, downloadManager, timeoutScaling, processWrapper, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index 18a66af17d9a3a..87f887adb07dd8 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java @@ -255,6 +255,7 @@ private FetchResult fetchInternal( outputDirectory, ignoredSubdirectories, env, + ImmutableMap.copyOf(repoEnvironment), ImmutableMap.copyOf(clientEnvironment), downloadManager, timeoutScaling, diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 1ffde7af46c14c..a738356830acba 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -108,6 +108,7 @@ public final class RepositoryDelegatorFunction implements SkyFunction { private final AtomicBoolean isFetch; private final BlazeDirectories directories; private final ExternalPackageHelper externalPackageHelper; + private final Supplier> repoEnvironmentSupplier; private final Supplier> clientEnvironmentSupplier; private final RepoContentsCache repoContentsCache; @@ -115,6 +116,7 @@ public RepositoryDelegatorFunction( ImmutableMap handlers, @Nullable RepositoryFunction starlarkHandler, AtomicBoolean isFetch, + Supplier> repoEnvironmentSupplier, Supplier> clientEnvironmentSupplier, BlazeDirectories directories, ExternalPackageHelper externalPackageHelper, @@ -122,6 +124,7 @@ public RepositoryDelegatorFunction( this.handlers = handlers; this.starlarkHandler = starlarkHandler; this.isFetch = isFetch; + this.repoEnvironmentSupplier = repoEnvironmentSupplier; this.clientEnvironmentSupplier = clientEnvironmentSupplier; this.directories = directories; this.externalPackageHelper = externalPackageHelper; @@ -441,6 +444,7 @@ private RepositoryFunction getHandler(Rule rule) { handler = handlers.get(rule.getRuleClass()); } if (handler != null) { + handler.setRepoEnvironment(repoEnvironmentSupplier.get()); handler.setClientEnvironment(clientEnvironmentSupplier.get()); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index 7fcb3175c0803d..62ea1b970e7b4b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -85,6 +85,7 @@ */ public abstract class RepositoryFunction { + protected Map repoEnvironment; protected Map clientEnvironment; /** @@ -420,7 +421,12 @@ public static void addExternalFilesDependencies( env.getValue(RepositoryDirectoryValue.key(repositoryName)); } - /** Sets up a mapping of environment variables to use. */ + /** Sets up a mapping of environment variables to use for repository operations. */ + public void setRepoEnvironment(Map repoEnvironment) { + this.repoEnvironment = repoEnvironment; + } + + /** Sets up a mapping of environment variables to use for client operations (e.g. auth). */ public void setClientEnvironment(Map clientEnvironment) { this.clientEnvironment = clientEnvironment; } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java index e29c11c6019a8e..aece641a9477cb 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java @@ -61,6 +61,7 @@ import com.google.devtools.build.lib.skyframe.serialization.analysis.RemoteAnalysisCachingEventListener; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.DetailedExitCode; +import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.io.CommandExtensionReporter; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; @@ -319,6 +320,19 @@ public void exit(AbruptExitException exception) { : UUID.randomUUID().toString(); this.repoEnv.putAll(clientEnv); + + Set defaultRepoEnvInherited = new TreeSet<>(); + defaultRepoEnvInherited.add("PATH"); + if (OS.getCurrent() == OS.WINDOWS) { + defaultRepoEnvInherited.add("PATHEXT"); + } + for (String name : defaultRepoEnvInherited) { + String value = clientEnv.get(name); + if (value != null) { + this.repoEnvFromOptions.put(name, value); + } + } + // TODO: This only needs to check for loads() rather than analyzes() due to // the effect of --action_env on the repository env. Revert back to // analyzes() when --action_env no longer affects it. @@ -333,6 +347,7 @@ public void exit(AbruptExitException exception) { visibleActionEnv.remove(entry.getKey()); if (!options.getOptions(CommonCommandOptions.class).repoEnvIgnoresActionEnv) { repoEnv.put(entry.getKey(), entry.getValue()); + repoEnvFromOptions.put(entry.getKey(), entry.getValue()); } } } @@ -937,6 +952,15 @@ public Map getRepoEnv() { return Collections.unmodifiableMap(repoEnv); } + /** + * Returns the repository environment created from specific client environment variables ({@code + * PATH} and on Windows {@code PATH_EXT}), {@code --repo_env}, and {@code --action_env=NAME=VALUE} + * (when {@code --incompatible_repo_env_ignores_action_env=false}). + */ + public Map getRepoEnvFromOptions() { + return Collections.unmodifiableMap(repoEnvFromOptions); + } + /** Returns the file cache to use during this build. */ public InputMetadataProvider getFileCache() { synchronized (fileCacheLock) { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java index a3e7f11fd915f2..00a05693554fe7 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java @@ -549,7 +549,7 @@ public String getTypeDescription() { @Option( name = "incompatible_repo_env_ignores_action_env", defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, help = @@ -559,6 +559,22 @@ public String getTypeDescription() { """) public boolean repoEnvIgnoresActionEnv; + @Option( + name = "experimental_strict_repo_env", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.EXPERIMENTAL}, + help = + """ + If true, repository rules and module extensions will only inherit `PATH`, `PATHEXT` + (on Windows), and environment variables explicitly specified by `--repo_env`. + + Note that unless `--incompatible_repo_env_ignores_action_env` is true, + `--action_env=NAME=VALUE` will also be included. + """) + public boolean useStrictRepoEnv; + @Option( name = "heuristically_drop_nodes", oldName = "experimental_heuristically_drop_nodes", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java index bd3457d7632917..4bf99d967d5a91 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java @@ -209,6 +209,7 @@ public void handle(Event event) {} starlarkRepositoryFunction, isFetch, ImmutableMap::of, + ImmutableMap::of, directories, EXTERNAL_PACKAGE_HELPER, repositoryCache.getRepoContentsCache())) diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java index 69c927f086fb11..3f47d4ecacfbc7 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java @@ -90,11 +90,14 @@ dependency on a label to a file (a repository rule cannot depend on a generated }, defaultValue = "[]", doc = - "Deprecated. This parameter has been deprecated. Migrate to " - + "repository_ctx.getenv instead.
" - + "Provides a list of environment variable that this repository rule depends " - + "on. If an environment variable in that list change, the repository will be " - + "refetched.", + """ + Deprecated. This parameter has been deprecated. Migrate to \ + repository_ctx.getenv instead.
+ Provides a list of environment variable that this repository rule depends \ + on. If an environment variable in that list change, the repository will be \ + refetched.
+ See also --experimental_strict_repo_env. + """, named = true, positional = false), @Param( @@ -178,9 +181,14 @@ StarlarkCallable repositoryRule( }, defaultValue = "[]", doc = - "Provides a list of environment variable that this module extension depends on. If " - + "an environment variable in that list changes, the extension will be " - + "re-evaluated.", + """ + Deprecated. This parameter has been deprecated. Migrate to \ + module_ctx.getenv instead.
+ Provides a list of environment variable that this module extension depends on. If \ + an environment variable in that list changes, the extension will be \ + re-evaluated.
+ See also --experimental_strict_repo_env. + """, named = true, positional = false), @Param( diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java index 2fa49bb654540b..a3a5fcb196305e 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java @@ -218,6 +218,7 @@ public ImmutableMap getSkyFunctions(BlazeDirectori new StarlarkRepositoryFunction(), new AtomicBoolean(true), ImmutableMap::of, + ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, new RepoContentsCache())) @@ -235,7 +236,7 @@ public ImmutableMap getSkyFunctions(BlazeDirectori .put(SkyFunctions.SINGLE_EXTENSION, new SingleExtensionFunction()) .put( SkyFunctions.SINGLE_EXTENSION_EVAL, - new SingleExtensionEvalFunction(directories, ImmutableMap::of)) + new SingleExtensionEvalFunction(directories, ImmutableMap::of, ImmutableMap::of)) .put(SkyFunctions.SINGLE_EXTENSION_USAGES, new SingleExtensionUsagesFunction()) .put( SkyFunctions.REGISTRY, diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java index 11aa05c717f8dc..97c9300441b0a2 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java @@ -165,6 +165,7 @@ public void setup() throws Exception { starlarkRepositoryFunction, new AtomicBoolean(true), ImmutableMap::of, + ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, new RepoContentsCache())) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java index 033bcd459f0ccc..6b5974f6b0e0f6 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/DiscoveryTest.java @@ -197,6 +197,7 @@ private void setUpWithBuiltinModules(ImmutableMap b null, new AtomicBoolean(true), ImmutableMap::of, + ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, new RepoContentsCache())) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index e40e717f44ca3d..36d2c3440a2933 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -242,6 +242,7 @@ public void setup() throws Exception { starlarkRepositoryFunction, new AtomicBoolean(true), ImmutableMap::of, + ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, new RepoContentsCache())) @@ -257,7 +258,7 @@ public void setup() throws Exception { .put(SkyFunctions.SINGLE_EXTENSION, new SingleExtensionFunction()) .put( SkyFunctions.SINGLE_EXTENSION_EVAL, - new SingleExtensionEvalFunction(directories, ImmutableMap::of)) + new SingleExtensionEvalFunction(directories, ImmutableMap::of, ImmutableMap::of)) .put( SkyFunctions.REGISTRY, new RegistryFunction(registryFactory, directories.getWorkspace())) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index 6ba357621f1a62..d3bd07504c423d 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -178,6 +178,7 @@ private void setUpWithBuiltinModules(ImmutableMap b null, new AtomicBoolean(true), ImmutableMap::of, + ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, new RepoContentsCache())) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java index 9efeb33ea630f4..f89b40b22d1fab 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java @@ -143,7 +143,8 @@ private static Object exec(String... lines) { private void setUpContextForRule( Map kwargs, IgnoredSubdirectories ignoredSubdirectories, - ImmutableMap envVariables, + ImmutableMap repoEnvVariables, + ImmutableMap clientEnvVariables, StarlarkSemantics starlarkSemantics, @Nullable RepositoryRemoteExecutor repoRemoteExecutor, Attribute... attributes) @@ -193,7 +194,8 @@ private void setUpContextForRule( outputDirectory, ignoredSubdirectories, environment, - envVariables, + repoEnvVariables, + clientEnvVariables, downloader, 1.0, /* processWrapper= */ null, @@ -213,6 +215,7 @@ private void setUpContextForRule(String name, StarlarkSemantics starlarkSemantic ImmutableMap.of("name", name), IgnoredSubdirectories.EMPTY, ImmutableMap.of("FOO", "BAR"), + ImmutableMap.of("FOO", "BAR"), starlarkSemantics, /* repoRemoteExecutor= */ null); } @@ -223,6 +226,7 @@ public void testAttr() throws Exception { ImmutableMap.of("name", "test", "foo", "bar"), IgnoredSubdirectories.EMPTY, ImmutableMap.of("FOO", "BAR"), + ImmutableMap.of("FOO", "BAR"), StarlarkSemantics.DEFAULT, /* repoRemoteExecutor= */ null, Attribute.attr("foo", Type.STRING).build()); @@ -237,6 +241,7 @@ public void testWhich() throws Exception { ImmutableMap.of("name", "test"), IgnoredSubdirectories.EMPTY, ImmutableMap.of("PATH", String.join(File.pathSeparator, "/bin", "/path/sbin", ".")), + ImmutableMap.of("PATH", String.join(File.pathSeparator, "/bin", "/path/sbin", ".")), StarlarkSemantics.DEFAULT, /* repoRemoteExecutor= */ null); scratch.file("/bin/true").setExecutable(true); @@ -328,6 +333,7 @@ public void testDelete() throws Exception { ImmutableMap.of("name", "test"), IgnoredSubdirectories.of(ImmutableSet.of(PathFragment.create("under_workspace"))), ImmutableMap.of("FOO", "BAR"), + ImmutableMap.of("FOO", "BAR"), StarlarkSemantics.DEFAULT, /* repoRemoteExecutor= */ null); assertThat(context.delete(underWorkspace.toString(), thread)).isTrue(); @@ -446,6 +452,7 @@ public void testRemoteExec() throws Exception { attrValues, IgnoredSubdirectories.EMPTY, ImmutableMap.of("FOO", "BAR"), + ImmutableMap.of("FOO", "BAR"), StarlarkSemantics.builder() .setBool(BuildLanguageOptions.EXPERIMENTAL_REPO_REMOTE_EXEC, true) .build(), diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java index 43df66e67364c7..5809cdde7ae453 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java @@ -142,6 +142,7 @@ public void setupDelegator() throws Exception { repositoryHandlers, new StarlarkRepositoryFunction(), /* isFetch= */ new AtomicBoolean(true), + /* repoEnvironmentSupplier= */ ImmutableMap::of, /* clientEnvironmentSupplier= */ ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, @@ -259,7 +260,8 @@ public void setupDelegator() throws Exception { .put(SkyFunctions.SINGLE_EXTENSION, new SingleExtensionFunction()) .put( SkyFunctions.SINGLE_EXTENSION_EVAL, - new SingleExtensionEvalFunction(directories, ImmutableMap::of)) + new SingleExtensionEvalFunction( + directories, ImmutableMap::of, ImmutableMap::of)) .put(SkyFunctions.SINGLE_EXTENSION_USAGES, new SingleExtensionUsagesFunction()) .put( SkyFunctions.MODULE_EXTENSION_REPO_MAPPING_ENTRIES, diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java index 3082df0c80932f..5f3109c0b8ebcb 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java @@ -151,6 +151,7 @@ public final void setUp() throws Exception { null, new AtomicBoolean(true), ImmutableMap::of, + ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, new RepoContentsCache())); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java index 02de3514bd7a80..3c3cee8dc7bf80 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java @@ -216,6 +216,7 @@ private MemoizingEvaluator makeEvaluator(ExternalFileAction externalFileAction) null, new AtomicBoolean(true), ImmutableMap::of, + ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, new RepoContentsCache())) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java index d32929f06b68d5..2c9f7067188be7 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java @@ -166,6 +166,7 @@ public final void setUp() throws Exception { null, new AtomicBoolean(true), ImmutableMap::of, + ImmutableMap::of, directories, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER, new RepoContentsCache())); diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 8f011cfeefa935..24179204dd00ca 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -3638,6 +3638,42 @@ EOF @repo//... &> $TEST_log || fail "expected Bazel to succeed" } +function test_execute_environment_strict_vars() { + cat >> $(setup_module_dot_bazel) <<'EOF' +my_repo = use_repo_rule("//:repo.bzl", "my_repo") +my_repo(name="repo") +EOF + touch BUILD + cat > repo.bzl <<'EOF' +def _impl(ctx): + st = ctx.execute( + ["env"], + ) + if st.return_code: + fail("Command did not succeed") + vars = {line.partition("=")[0]: line.partition("=")[-1] for line in st.stdout.strip().split("\n")} + if vars.get("CLIENT_ENV_PRESENT") != "value1": + fail("CLIENT_ENV_PRESENT has wrong value: " + vars.get("CLIENT_ENV_PRESENT")) + if "CLIENT_ENV_REMOVED" in vars: + fail("CLIENT_ENV_REMOVED should not be in the environment") + if vars.get("REPO_ENV_PRESENT") != "value3": + fail("REPO_ENV_PRESENT has wrong value: " + vars.get("REPO_ENV_PRESENT")) + if "PATH" not in vars: + fail("PATH should be in the environment") + if ctx.os.name.startswith("windows") and "PATHEXT" not in vars: + fail("PATHEXT should be in the environment (on Windows)") + ctx.file("BUILD", "exports_files(['data.txt'])") +my_repo = repository_rule(_impl) +EOF + + CLIENT_ENV_PRESENT=value1 CLIENT_ENV_REMOVED=value2 PATHEXT=".COM;.EXE;.BAT;.CMD;"\ + bazel build \ + --experimental_strict_repo_env \ + --repo_env=CLIENT_ENV_PRESENT \ + --repo_env=REPO_ENV_PRESENT=value3 \ + @repo//... &> $TEST_log || fail "expected Bazel to succeed" +} + function test_dependency_on_repo_with_invalid_name() { cat >> $(setup_module_dot_bazel) <<'EOF' my_repo = use_repo_rule("//:repo.bzl", "my_repo")