From 6374991ecead5106055e62df8ecfd2718d370275 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 13 Mar 2026 18:02:56 +0000 Subject: [PATCH] [8.7.0] Manual port of essential parts of: Fix and consolidate repo env handling Manual port of only the essential API changes from 01407ce758 needed by later feature commits, since the full `CommandEnvironment` repo-env consolidation does not apply to 8.x: - Add `EnvironmentVariableValue` record type - Add `RepoEnvironmentFunction` with `--repo_env` + client env fallback - Register `REPOSITORY_ENVIRONMENT_VARIABLE` in `SkyFunctions` and `SkyframeExecutor` - Update `EnvVar.getSkyKey()` to use `RepoEnvironmentFunction` - Update `EnvVar.isOutdated()` to use `EnvironmentVariableValue` On 8.7.0, `RepoEnvironmentFunction` checks `--repo_env` first, then falls back to the client environment via `ClientEnvironmentFunction`, since the consolidated repo env computation from `CommandEnvironment` is not present. Additionally includes the regression test from #29946 (`test_unrelated_env_var_does_not_invalidate_repo`): changing an unrelated environment variable via an interleaved `bazel mod` command must not refetch a repository that doesn't depend on it. #29946's production fix is not needed here because this narrow port injects only `repoEnvFromOptions` into `REPO_ENV` and falls back to the already per-variable `ClientEnvironmentFunction` for everything else, so an unrelated client variable never mutates the whole-map node. Original commit message follows: The current invalidation logic didn't take `--incompatible_repo_env_ignores_action_env` and `--experimental_strict_repo_env` into account, which resulted in incorrect invalidation of repo rules and module extensions. This is addressed by a larger refactoring that consolidates the logic that computes the effective environment for repository rules and module extensions in `CommandEnvironment`. This environment is then read and sliced by a new `RepoEnvironmentFunction` that operates analogously to `ClientEnvironmentFunction`. Along the way, the variety of terminology in `CommandEnvironment` and various `SkyFunction`s is cleaned up to consistently use: * `repoEnv` to refer to the environment seen by repo rules and module extensions, which may or may not see the full client env based on the value of `--experimental_strict_repo_env` and * `nonstrictRepoEnv`, which refers to same environment with, conceptually, `--experimental_strict_repo_env` forced to `false`. This is used for certain non-hermetic operations such as downloader and credential helper logic. Closes #28168. PiperOrigin-RevId: 854228202 Change-Id: I900f260dd5d0c6e20dcc32eaee0821567e60d5d1 (cherry picked from commit 01407ce75841295543878f10bddb22d5e178c6ca) --- .../com/google/devtools/build/lib/rules/BUILD | 4 +- .../rules/repository/RepoRecordedInput.java | 13 +- .../google/devtools/build/lib/skyframe/BUILD | 29 +++++ .../skyframe/EnvironmentVariableValue.java | 28 +++++ .../lib/skyframe/RepoEnvironmentFunction.java | 112 ++++++++++++++++++ .../build/lib/skyframe/SkyFunctions.java | 2 + .../build/lib/skyframe/SkyframeExecutor.java | 1 + .../build/lib/skyframe/packages/BUILD | 1 + .../skyframe/packages/BazelPackageLoader.java | 2 + .../shell/bazel/starlark_repository_test.sh | 68 +++++++++++ 10 files changed, 252 insertions(+), 8 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentVariableValue.java create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/RepoEnvironmentFunction.java diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index 453d06ec3ba748..65e8a78f9d27ff 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -340,8 +340,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator", - "//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function", - "//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:environment_variable_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:repo_environment_function", "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_value", "//src/main/java/com/google/devtools/build/lib/skyframe:directory_tree_digest_value", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index 055b4e329ac32a..39a5b43e444dae 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -31,8 +31,8 @@ import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; -import com.google.devtools.build.lib.skyframe.ClientEnvironmentValue; +import com.google.devtools.build.lib.skyframe.EnvironmentVariableValue; +import com.google.devtools.build.lib.skyframe.RepoEnvironmentFunction; import com.google.devtools.build.lib.skyframe.DirectoryListingValue; import com.google.devtools.build.lib.skyframe.DirectoryTreeDigestValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; @@ -625,17 +625,18 @@ public String toStringInternal() { @Override public SkyKey getSkyKey(BlazeDirectories directories) { - return ActionEnvironmentFunction.key(name); + return RepoEnvironmentFunction.key(name); } @Override public Optional isOutdated( Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException { - String v = PrecomputedValue.REPO_ENV.get(env).get(name); - if (v == null) { - v = ((ClientEnvironmentValue) env.getValue(getSkyKey(directories))).getValue(); + var envValue = (EnvironmentVariableValue) env.getValue(getSkyKey(directories)); + if (envValue == null) { + return Optional.empty(); } + String v = envValue.value(); // Note that `oldValue` can be null if the env var was not set. if (!Objects.equals(oldValue, v)) { return Optional.of( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 7a741b87cbf93b..c2210207ecb5fa 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -110,6 +110,7 @@ java_library( ":cached_bzl_load_value_and_deps_builder_factory", ":client_environment_function", ":client_environment_value", + ":environment_variable_value", ":collect_packages_under_directory_function", ":collect_packages_under_directory_value", ":collect_targets_in_package_function", @@ -173,6 +174,7 @@ java_library( ":recursive_package_provider_backed_target_pattern_resolver", ":recursive_pkg_function", ":recursive_pkg_value", + ":repo_environment_function", ":repo_file_function", ":repo_package_args_function", ":repository_mapping_function", @@ -1025,6 +1027,16 @@ java_library( ], ) +java_library( + name = "environment_variable_value", + srcs = ["EnvironmentVariableValue.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:jsr305", + ], +) + java_library( name = "collect_packages_under_directory_function", srcs = ["CollectPackagesUnderDirectoryFunction.java"], @@ -2335,6 +2347,23 @@ java_library( ], ) +java_library( + name = "repo_environment_function", + srcs = ["RepoEnvironmentFunction.java"], + deps = [ + ":client_environment_function", + ":client_environment_value", + ":environment_variable_value", + ":precomputed_value", + ":sky_functions", + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:guava", + "//third_party:jsr305", + ], +) + java_library( name = "repo_file_function", srcs = ["RepoFileFunction.java"], diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentVariableValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentVariableValue.java new file mode 100644 index 00000000000000..eea16f29607648 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentVariableValue.java @@ -0,0 +1,28 @@ +// Copyright 2026 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.skyframe; + +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.skyframe.SkyValue; +import javax.annotation.Nullable; + +/** + * The value of an environmental variable from an environment (client env, action env or repository + * env). These are invalidated and injected by {@link SequencedSkyframeExecutor}. + * + * @param value the value in the client environment or null if unset in the environment. + */ +@AutoCodec +public record EnvironmentVariableValue(@Nullable String value) implements SkyValue {} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepoEnvironmentFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepoEnvironmentFunction.java new file mode 100644 index 00000000000000..ffddeed475ab4a --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepoEnvironmentFunction.java @@ -0,0 +1,112 @@ +// Copyright 2026 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.skyframe; + +import com.google.common.collect.Collections2; +import com.google.common.collect.ImmutableSortedMap; +import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.skyframe.AbstractSkyKey; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.SkyframeLookupResult; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import javax.annotation.Nullable; + +/** + * Skyframe function that provides the effective value for an environment variable in the context of + * repository rules and module extensions. This checks {@code --repo_env} overrides first, then falls + * back to the client environment. + */ +public final class RepoEnvironmentFunction implements SkyFunction { + @Override + public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { + Map repoEnv = PrecomputedValue.REPO_ENV.get(env); + String key = (String) skyKey.argument(); + if (repoEnv.containsKey(key)) { + return new EnvironmentVariableValue(repoEnv.get(key)); + } + // Fall back to the client environment. + ClientEnvironmentValue clientValue = + (ClientEnvironmentValue) env.getValue(ClientEnvironmentFunction.key(key)); + if (clientValue == null) { + return null; + } + return new EnvironmentVariableValue(clientValue.getValue()); + } + + /** Returns the SkyKey to invoke this function for the environment variable {@code variable}. */ + public static Key key(String variable) { + return Key.create(variable); + } + + @VisibleForSerialization + @AutoCodec + static class Key extends AbstractSkyKey { + private static final SkyKeyInterner interner = SkyKey.newInterner(); + + private Key(String arg) { + super(arg); + } + + private static Key create(String arg) { + return interner.intern(new Key(arg)); + } + + @VisibleForSerialization + @AutoCodec.Interner + static Key intern(Key key) { + return interner.intern(key); + } + + @Override + public SkyFunctionName functionName() { + return SkyFunctions.REPOSITORY_ENVIRONMENT_VARIABLE; + } + + @Override + public SkyKeyInterner getSkyKeyInterner() { + return interner; + } + } + + /** + * Returns a map of environment variable key => values, getting them from Skyframe. Returns null + * if and only if some dependencies from Skyframe still need to be resolved. + */ + @Nullable + public static ImmutableSortedMap> getEnvironmentView( + Environment env, Set keys) throws InterruptedException { + var skyKeys = Collections2.transform(keys, RepoEnvironmentFunction::key); + SkyframeLookupResult values = env.getValuesAndExceptions(skyKeys); + if (env.valuesMissing()) { + return null; + } + + var result = ImmutableSortedMap.>naturalOrder(); + for (var key : skyKeys) { + var value = (EnvironmentVariableValue) values.get(key); + if (value == null) { + return null; + } + result.put(key.argument().toString(), Optional.ofNullable(value.value())); + } + return result.buildOrThrow(); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index 5a06e025aa420c..b7fc0c162a8f9e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -26,6 +26,8 @@ public final class SkyFunctions { static final SkyFunctionName ACTION_SKETCH = SkyFunctionName.createHermetic("ACTION_SKETCH"); public static final SkyFunctionName ACTION_ENVIRONMENT_VARIABLE = SkyFunctionName.createHermetic("ACTION_ENVIRONMENT_VARIABLE"); + public static final SkyFunctionName REPOSITORY_ENVIRONMENT_VARIABLE = + SkyFunctionName.createHermetic("REPOSITORY_ENVIRONMENT_VARIABLE"); public static final SkyFunctionName DIRECTORY_LISTING_STATE = SkyFunctionName.createNonHermetic("DIRECTORY_LISTING_STATE"); public static final SkyFunctionName DIRECTORY_LISTING = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index f57e51c8af78b0..2deb46a610348b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -638,6 +638,7 @@ private ImmutableMap skyFunctions() { map.put(SkyFunctions.PRECOMPUTED, new PrecomputedFunction()); map.put(SkyFunctions.CLIENT_ENVIRONMENT_VARIABLE, new ClientEnvironmentFunction(clientEnv)); map.put(SkyFunctions.ACTION_ENVIRONMENT_VARIABLE, new ActionEnvironmentFunction()); + map.put(SkyFunctions.REPOSITORY_ENVIRONMENT_VARIABLE, new RepoEnvironmentFunction()); map.put(FileStateKey.FILE_STATE, newFileStateFunction()); map.put(SkyFunctions.DIRECTORY_LISTING_STATE, newDirectoryListingStateFunction()); map.put(FileSymlinkCycleUniquenessFunction.NAME, new FileSymlinkCycleUniquenessFunction()); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD index a86993c2995fba..5bb1b2b50084a8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD @@ -102,6 +102,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", "//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function", "//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_function", + "//src/main/java/com/google/devtools/build/lib/skyframe:repo_environment_function", "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_function", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", 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 4bf99d967d5a91..da5bd0113f6daa 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 @@ -48,6 +48,7 @@ import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; import com.google.devtools.build.lib.skyframe.PrecomputedValue; +import com.google.devtools.build.lib.skyframe.RepoEnvironmentFunction; import com.google.devtools.build.lib.skyframe.RepositoryMappingFunction; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.vfs.Path; @@ -198,6 +199,7 @@ public void handle(Event event) {} SkyFunctions.DIRECTORY_LISTING_STATE, new DirectoryListingStateFunction(externalFilesHelper, SyscallCache.NO_CACHE)) .put(SkyFunctions.ACTION_ENVIRONMENT_VARIABLE, new ActionEnvironmentFunction()) + .put(SkyFunctions.REPOSITORY_ENVIRONMENT_VARIABLE, new RepoEnvironmentFunction()) .put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction()) .put( SkyFunctions.LOCAL_REPOSITORY_LOOKUP, diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 72404b23dba234..955003b817758f 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -801,6 +801,74 @@ function test_starlark_repository_environ_invalidation_action_env_batch() { environ_invalidation_action_env_test_template --batch } +# Cherry-picked from https://github.com/bazelbuild/bazel/pull/29946. On master, +# the repository environment used to be a single whole-map Skyframe node, so +# changing one environment variable invalidated every repository rule and module +# extension, even ones that didn't depend on it; that PR fixed the over-invalidation. +# +# That bug never applied to 8.x, so #29946's production fix is not ported: the +# narrow repo-env port on this branch injects only `repoEnvFromOptions` into +# `REPO_ENV` and otherwise falls back to the already per-variable +# `ClientEnvironmentFunction`, so an unrelated client variable never mutates the +# whole-map node. This test is kept here to prove that. +# +# The repo here only depends on FOO, yet an interleaved `mod` invocation with an +# unrelated variable OTHER changed must not cause it to be refetched. +function test_unrelated_env_var_does_not_invalidate_repo() { + local execution_file="${TEST_TMPDIR}/execution" + echo 0 > "${execution_file}" + + cat > $(setup_module_dot_bazel) <<'EOF' +ext = use_extension("//:ext.bzl", "ext") +use_repo(ext, "foo") +EOF + cat > ext.bzl < ${execution_file}" % count]) + repository_ctx.file( + "BUILD", "filegroup(name = 'bar', visibility = ['//visibility:public'])") + repository_ctx.file("REPO.bazel", "") + +# A local repo is refetched whenever its node is recomputed, which makes spurious +# invalidation directly observable through the fetch counter. +repo = repository_rule(implementation = _repo_impl, local = True) + +def _ext_impl(module_ctx): + repo(name = "foo") + +ext = module_extension(implementation = _ext_impl) +EOF + cat > BUILD <<'EOF' +filegroup(name = "t", srcs = ["@foo//:bar"]) +EOF + + # Initial fetch. + FOO=1 OTHER=a bazel build //:t >& $TEST_log || fail "Failed to build" + assert_equals 1 $(cat "${execution_file}") + # Rebuilding with the same environment must not refetch. + FOO=1 OTHER=a bazel build //:t >& $TEST_log || fail "Failed to build" + assert_equals 1 $(cat "${execution_file}") + + # Evaluate the whole module/repo graph with an unrelated variable (OTHER) + # changed, then build again with the original environment. The repo only reads + # FOO, so it must not be refetched. + FOO=1 OTHER=b bazel mod graph >& $TEST_log || fail "Failed to run mod" + FOO=1 OTHER=a bazel build //:t >& $TEST_log || fail "Failed to build" + assert_equals 1 $(cat "${execution_file}") + + # A second interleaving with yet another value must likewise not refetch. + FOO=1 OTHER=c bazel mod graph >& $TEST_log || fail "Failed to run mod" + FOO=1 OTHER=a bazel build //:t >& $TEST_log || fail "Failed to build" + assert_equals 1 $(cat "${execution_file}") + + # Sanity check: changing FOO, which the repo does depend on, must refetch. + FOO=2 OTHER=a bazel build //:t >& $TEST_log || fail "Failed to build" + assert_equals 2 $(cat "${execution_file}") +} + # Test invalidation based on change to the bzl files function bzl_invalidation_test_template() { local startup_flag="${1-}"