fixup! [rfc] Allow repository rules to lazily declare environment var…

…iable deps

site/en/extending/repo.md

@@ -108,12 +108,11 @@ following things changes:
 
 * The attributes passed to the repo rule invocation.
 * The Starlark code comprising the implementation of the repo rule.
-* 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/StarlarkBaseExternalContext.java

@@ -92,8 +92,8 @@
 import net.starlark.java.annot.StarlarkMethod;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
-import net.starlark.java.eval.Printer;
 import net.starlark.java.eval.NoneType;
+import net.starlark.java.eval.Printer;
 import net.starlark.java.eval.Sequence;
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkInt;
@@ -1064,12 +1064,19 @@ 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 =
-          // Should we disclose the (intended) side-effect of tracking env vars in the marker file?
-          "Returns the value of an environment variable `name` as a string if exists, or `default` "
-              + "if it doesn't.",
+          "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",
@@ -1085,19 +1092,22 @@ public void createFile(
               },
               defaultValue = "None"
           )
-      })
-  public Object getEnvironmentValue(String name, Object defaultValue)
+      },
+      allowReturnNones = true)
+  @Nullable
+  public String getEnvironmentValue(String name, Object defaultValue)
       throws InterruptedException, NeedsSkyframeRestartException {
-    // Must look up via AEF, rather than a copy like `this.envVariables`, in order to
+    // Must look up via AEF, rather than solely copy from `this.envVariables`, in order to
     // establish a SkyKey dependency relationship.
-    ImmutableMap<String, String> vars = ActionEnvironmentFunction.getEnvironmentView(env,
-        ImmutableSet.of(name));
-    if (vars == null) {
+    if (env.getValue(ActionEnvironmentFunction.key(name)) == null) {
         throw new NeedsSkyframeRestartException();
     }
-    String value = vars.get(name);
+
+    // 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 value != null ? value : defaultValue;
+    return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class);
   }
 
   @StarlarkMethod(

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkOS.java

@@ -42,7 +42,17 @@ 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

@@ -43,6 +43,7 @@
 import com.google.devtools.build.lib.rules.repository.WorkspaceFileHelper;
 import com.google.devtools.build.lib.runtime.ProcessWrapper;
 import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
+import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
 import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesValue;
 import com.google.devtools.build.lib.skyframe.PrecomputedValue;
 import com.google.devtools.build.lib.util.CPU;
@@ -55,6 +56,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;
@@ -325,7 +328,7 @@ private RepositoryDirectoryValue.Builder fetchInternal(
 
       // Ditto for environment variables accessed via `getenv`.
       for (String envKey : starlarkRepositoryContext.getAccumulatedEnvKeys()) {
-        markerData.put("LAZYENV:" + envKey, clientEnvironment.get(envKey));
+        markerData.put("ENV:" + envKey, clientEnvironment.get(envKey));
       }
 
       env.getListener().post(resolved);
@@ -378,6 +381,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/rules/repository/RepositoryFunction.java

@@ -349,30 +349,16 @@ public static ImmutableMap<String, String> getEnvVarValues(Environment env, Set<
   protected boolean verifyEnvironMarkerData(
       Map<String, String> markerData, Environment env, Set<String> keys)
       throws InterruptedException {
-    Map<String, String> repoEnvOverride = PrecomputedValue.REPO_ENV.get(env);
-    if (repoEnvOverride == null) {
-      return false;
-    }
-
-    return verifyPredeclaredEnvironMarkerData(markerData, env, repoEnvOverride, keys) &&
-        verifyLazyEnvironMarkerData(markerData, env, repoEnvOverride);
-  }
-
-  /**
-   * Verify marker data for predeclared environment variables (such as via
-   * {@code StarlarkRepositoryFunction}'s @{code $environ}).
-   */
-  private boolean verifyPredeclaredEnvironMarkerData(
-      Map<String, String> markerData,
-      Environment env,
-      Map<String, String> repoEnvOverride,
-      Set<String> keys
-  ) throws InterruptedException {
     ImmutableMap<String, String> environ = ActionEnvironmentFunction.getEnvironmentView(env, keys);
     if (env.valuesMissing()) {
       return false; // Returns false so caller knows to return immediately
     }
 
+    Map<String, String> repoEnvOverride = PrecomputedValue.REPO_ENV.get(env);
+    if (repoEnvOverride == null) {
+      return false;
+    }
+
     // Only depend on --repo_env values that are specified in the "environ" attribute.
     Map<String, String> repoEnv = new LinkedHashMap<>(environ);
     for (String key : keys) {
@@ -402,43 +388,6 @@ private boolean verifyPredeclaredEnvironMarkerData(
     return true;
   }
 
-  /**
-   * Verify marker data for environment variables lazily looked up by repository rule
-   * implementations.
-   */
-  private boolean verifyLazyEnvironMarkerData(
-      Map<String, String> markerData,
-      Environment env,
-      Map<String, String> repoEnvOverride
-  ) throws InterruptedException {
-    ImmutableSet<String> lazyKeys = ImmutableSet.copyOf(
-        markerData
-            .keySet()
-            .stream()
-            .filter(s -> s.startsWith("LAZYENV:"))
-            .iterator()
-    );
-    ImmutableMap<String, String> environ =
-        ActionEnvironmentFunction.getEnvironmentView(env,
-            lazyKeys
-                .stream()
-                .map(s -> s.substring(8))  // LAZYENV:FOO -> FOO
-                .collect(ImmutableSet.toImmutableSet()));
-    if (env.valuesMissing()) {
-      return false; // Returns false so caller knows to return immediately
-    }
-
-    for (String key : lazyKeys) {
-      String markerValue = markerData.get(key);
-      String envKey = key.substring(8);
-      if (!Objects.equals(markerValue, repoEnvOverride.getOrDefault(envKey, environ.get(envKey)))) {
-        return false;
-      }
-    }
-
-    return true;
-  }
-
   /**
    * Whether fetching is done using local operations only.
    *

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java

@@ -84,8 +84,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

@@ -3051,7 +3051,7 @@ EOF
 
   # Side-effect key.
   LAZYEVAL_KEY=xal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
-  expect_log_n "PREDECLARED_KEY=None" 2  # rctx.getenv("LAZYEVAL_KEY") triggers restart.
+  expect_log "PREDECLARED_KEY=None"
   expect_log "LAZYEVAL_KEY=xal1"
 
   # Side-effect key, no-op build.
@@ -3061,6 +3061,10 @@ EOF
   # 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"