diff --git a/site/en/extending/repo.md b/site/en/extending/repo.md index cf6bc1d38a976a..e617d625bde305 100644 --- a/site/en/extending/repo.md +++ b/site/en/extending/repo.md @@ -114,12 +114,11 @@ following things changes: * The parameters passed to the declaration of the repository in the `WORKSPACE` file. * The Starlark code comprising the implementation of the repository. -* The value of any environment variable declared with the `environ` - attribute of the [`repository_rule`](/rules/lib/globals/bzl#repository_rule). - The values of these environment variables can be hard-wired on the command - line with the - [`--action_env`](/reference/command-line-reference#flag--action_env) - flag (but this flag will invalidate every action of the build). +* The value of any environment variable passed to `repository_ctx`'s + `getenv()` method or declared with the `environ` attribute of the + [`repository_rule`](/rules/lib/globals/bzl#repository_rule). The values + of these environment variables can be hard-wired on the command line with the + [`--repo_env`](/reference/command-line-reference#flag--repo_env) flag. * The content of any file passed to the `read()`, `execute()` and similar methods of `repository_ctx` which is referred to by a label (for example, `//mypkg:label.txt` but not `mypkg/label.txt`) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD index ab35f1c9d9a46b..4ce0fabc14a265 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD @@ -40,6 +40,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_file_helper", "//src/main/java/com/google/devtools/build/lib/shell", + "//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function", "//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_value", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository", 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 c15947239ef77f..50c5554c772e6f 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 @@ -22,6 +22,7 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Maps; import com.google.common.util.concurrent.Futures; @@ -51,6 +52,7 @@ import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult; +import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; import com.google.devtools.build.lib.util.OsUtils; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -77,6 +79,7 @@ import java.util.Arrays; import java.util.Base64; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -89,6 +92,7 @@ import net.starlark.java.annot.StarlarkMethod; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.NoneType; import net.starlark.java.eval.Printer; import net.starlark.java.eval.Sequence; import net.starlark.java.eval.Starlark; @@ -137,6 +141,7 @@ private interface AsyncTask { @Nullable private final ProcessWrapper processWrapper; protected final StarlarkSemantics starlarkSemantics; private final HashMap accumulatedFileDigests = new HashMap<>(); + private final HashSet accumulatedEnvKeys = new HashSet<>(); private final RepositoryRemoteExecutor remoteExecutor; private final List asyncTasks; @@ -193,6 +198,11 @@ public ImmutableMap getAccumulatedFileDigests() { return ImmutableMap.copyOf(accumulatedFileDigests); } + /** Returns set of environment variable keys encountered so far. */ + public ImmutableSet getAccumulatedEnvKeys() { + return ImmutableSet.copyOf(accumulatedEnvKeys); + } + protected void checkInOutputDirectory(String operation, StarlarkPath path) throws RepositoryFunctionException { if (!path.getPath().getPathString().startsWith(workingDirectory.getPathString())) { @@ -1006,6 +1016,47 @@ public void createFile( } } + // Move to a common location like net.starlark.java.eval.Starlark? + @Nullable + private static T nullIfNone(Object object, Class type) { + return object != Starlark.NONE ? type.cast(object) : null; + } + + @StarlarkMethod( + name = "getenv", + doc = + "Returns the value of an environment variable name as a string if exists, " + + "or default if it doesn't." + + "

When building incrementally, any change to the value of the variable named by " + + "name will cause this repository to be re-fetched.", + parameters = { + @Param( + name = "name", + doc = "name of desired environment variable", + allowedTypes = {@ParamType(type = String.class)}), + @Param( + name = "default", + doc = "Default value to return if `name` is not found", + allowedTypes = {@ParamType(type = String.class), @ParamType(type = NoneType.class)}, + defaultValue = "None") + }, + allowReturnNones = true) + @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 + // 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`. + // See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 . + String envVarValue = envVariables.get(name); + accumulatedEnvKeys.add(name); + return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class); + } + @StarlarkMethod( name = "path", doc = diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkOS.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkOS.java index 333ea07801e2a1..500f0f39e065e8 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkOS.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkOS.java @@ -42,7 +42,16 @@ public boolean isImmutable() { return true; // immutable and Starlark-hashable } - @StarlarkMethod(name = "environ", structField = true, doc = "The list of environment variables.") + @StarlarkMethod( + name = "environ", + structField = true, + doc = + "The dictionary of environment variables." + + "

NOTE: Retrieving an environment variable from this dictionary does not " + + "establish a dependency from a repository rule or module extension to the " + + "environment variable. To establish a dependency when looking up an " + + "environment variable, use either repository_ctx.getenv or " + + "module_ctx.getenv instead.") public ImmutableMap getEnvironmentVariables() { return environ; } 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 74f52360613611..be0c3e1f656027 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 @@ -55,6 +55,8 @@ import com.google.devtools.build.skyframe.SkyKey; import java.io.IOException; import java.util.Map; +import java.util.Objects; +import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import javax.annotation.Nullable; @@ -323,6 +325,11 @@ private RepositoryDirectoryValue.Builder fetchInternal( markerData.put("FILE:" + entry.getKey(), entry.getValue()); } + // Ditto for environment variables accessed via `getenv`. + for (String envKey : starlarkRepositoryContext.getAccumulatedEnvKeys()) { + markerData.put("ENV:" + envKey, clientEnvironment.get(envKey)); + } + env.getListener().post(resolved); } catch (NeedsSkyframeRestartException e) { // A dependency is missing, cleanup and returns null @@ -373,6 +380,47 @@ private static ImmutableSet getEnviron(Rule rule) { return ImmutableSet.copyOf((Iterable) rule.getAttr("$environ")); } + /** + * Verify marker data previously saved by {@link #declareEnvironmentDependencies(Map, Environment, + * Set)} and/or {@link #fetchInternal(Rule, Path, BlazeDirectories, Environment, Map, SkyKey)} (on + * behalf of {@link StarlarkBaseExternalContext#getEnvironmentValue(String, Object)}). + */ + @Override + protected boolean verifyEnvironMarkerData( + Map markerData, Environment env, Set keys) + throws InterruptedException { + /* + * We can ignore `keys` and instead only verify what's recorded in the marker file, because + * any change to `keys` between builds would be caused by a change to a .bzl file, and that's + * covered by RepositoryDelegatorFunction.DigestWriter#areRepositoryAndMarkerFileConsistent. + */ + + ImmutableSet markerKeys = + markerData.keySet().stream() + .filter(s -> s.startsWith("ENV:")) + .collect(ImmutableSet.toImmutableSet()); + + ImmutableMap environ = + getEnvVarValues( + env, + markerKeys.stream() + .map(s -> s.substring(4)) // ENV:FOO -> FOO + .collect(ImmutableSet.toImmutableSet())); + if (environ == null) { + return false; + } + + for (String key : markerKeys) { + String markerValue = markerData.get(key); + String envKey = key.substring(4); // ENV:FOO -> FOO + if (!Objects.equals(markerValue, environ.get(envKey))) { + return false; + } + } + + return true; + } + @Override protected boolean isLocal(Rule rule) { return (Boolean) rule.getAttr("$local"); 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 1391a0ff5b7fd0..d713013f882386 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 @@ -82,8 +82,10 @@ public interface RepositoryModuleApi { }, defaultValue = "[]", doc = - "Provides a list of environment variable that this repository rule depends on. If " - + "an environment variable in that list change, the repository will be " + "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.", named = true, positional = false), diff --git a/src/test/shell/bazel/external_integration_test.sh b/src/test/shell/bazel/external_integration_test.sh index e378a7074ccc4b..7ee18e3ecc6305 100755 --- a/src/test/shell/bazel/external_integration_test.sh +++ b/src/test/shell/bazel/external_integration_test.sh @@ -1943,7 +1943,7 @@ EOF function test_cache_hit_reported() { # Verify that information about a cache hit is reported - # if an error happend in that repository. This information + # if an error happened in that repository. This information # is useful as users sometimes change the URL but do not # update the hash. WRKDIR=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX") @@ -3000,4 +3000,71 @@ EOF test -h "$execroot/external/ext" || fail "Expected symlink to external repo." } +function test_environ_incrementally() { + # Set up workspace with a repository rule to examine env vars. Assert that undeclared + # env vars don't trigger reevaluations. + cat > repo.bzl < BUILD.dummy < WORKSPACE <$TEST_log || fail 'Expected no-op build to succeed' + expect_log "UNDECLARED_KEY=val1" + + # UNDECLARED_KEY is, well, undeclared. This will be a no-op. + UNDECLARED_KEY=val2 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed' + expect_not_log "UNDECLARED_KEY" + + #--- + + # Predeclared key. + PREDECLARED_KEY=wal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed' + expect_log "PREDECLARED_KEY=wal1" + + # Predeclared key, no-op build. + PREDECLARED_KEY=wal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed' + expect_not_log "PREDECLARED_KEY" + + # Predeclared key, new value -> refetch. + PREDECLARED_KEY=wal2 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed' + expect_log "PREDECLARED_KEY=wal2" + + #--- + + # Side-effect key. + LAZYEVAL_KEY=xal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed' + expect_log "PREDECLARED_KEY=None" + expect_log "LAZYEVAL_KEY=xal1" + + # Side-effect key, no-op build. + LAZYEVAL_KEY=xal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed' + expect_not_log "LAZYEVAL_KEY" + + # Side-effect key, new value -> refetch. + LAZYEVAL_KEY=xal2 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed' + expect_log "LAZYEVAL_KEY=xal2" + + # Ditto, but with --repo_env overriding environment. + LAZYEVAL_KEY=xal2 bazel query --repo_env=LAZYEVAL_KEY=xal3 @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed' + expect_log "LAZYEVAL_KEY=xal3" +} + run_suite "external tests"