[7.1.0] [rfc] Allow repository rules to lazily declare environment variable deps

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`)

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",

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<Label, String> accumulatedFileDigests = new HashMap<>();
+  private final HashSet<String> accumulatedEnvKeys = new HashSet<>();
   private final RepositoryRemoteExecutor remoteExecutor;
   private final List<AsyncTask> asyncTasks;
 
@@ -193,6 +198,11 @@ public ImmutableMap<Label, String> getAccumulatedFileDigests() {
     return ImmutableMap.copyOf(accumulatedFileDigests);
   }
 
+  /** Returns set of environment variable keys encountered so far. */
+  public ImmutableSet<String> 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> T nullIfNone(Object object, Class<T> type) {
+    return object != Starlark.NONE ? type.cast(object) : null;
+  }
+
+  @StarlarkMethod(
+      name = "getenv",
+      doc =
+          "Returns the value of an environment variable <code>name</code> as a string if exists, "
+              + "or <code>default</code> if it doesn't."
+              + "<p>When building incrementally, any change to the value of the variable named by "
+              + "<code>name</code> 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 =

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."
+              + "<p><b>NOTE</b>: 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 <code>repository_ctx.getenv</code> or "
+              + "<code>module_ctx.getenv</code> instead.")
   public ImmutableMap<String, String> getEnvironmentVariables() {
     return environ;
   }

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<String> getEnviron(Rule rule) {
     return ImmutableSet.copyOf((Iterable<String>) 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<String, String> markerData, Environment env, Set<String> 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<String> markerKeys =
+        markerData.keySet().stream()
+            .filter(s -> s.startsWith("ENV:"))
+            .collect(ImmutableSet.toImmutableSet());
+
+    ImmutableMap<String, String> 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");

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 "
+                "<b>Deprecated</b>. This parameter has been deprecated. Migrate to "
+                    + "<code>repository_ctx.getenv</code> instead.<br/>"
+                    + "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),

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 <<EOF
+def _impl(rctx):
+  rctx.symlink(rctx.attr.build_file, 'BUILD')
+  print('UNDECLARED_KEY=%s' % rctx.os.environ.get('UNDECLARED_KEY'))
+  print('PREDECLARED_KEY=%s' % rctx.os.environ.get('PREDECLARED_KEY'))
+  print('LAZYEVAL_KEY=%s' % rctx.getenv('LAZYEVAL_KEY'))
+
+dummy_repository = repository_rule(
+  implementation = _impl,
+  attrs = {'build_file': attr.label()},
+  environ = ['PREDECLARED_KEY'],  # sic
+)
+EOF
+  cat > BUILD.dummy <<EOF
+filegroup(name='dummy', srcs=['BUILD'])
+EOF
+  touch BUILD
+  cat > WORKSPACE <<EOF
+load('//:repo.bzl', 'dummy_repository')
+dummy_repository(name = 'foo', build_file = '@@//:BUILD.dummy')
+EOF
+
+  # Baseline: DEBUG: UNDECLARED_KEY is logged to stderr.
+  UNDECLARED_KEY=val1 bazel query @foo//:BUILD 2>$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"