Refactor marker file logic

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

@@ -20,6 +20,7 @@
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.bazel.ResolvedEvent;
 import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.ResolvedFileValue;
@@ -56,7 +57,7 @@ public RepositoryDirectoryValue.Builder fetch(
       Path outputDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> markerData,
+      Map<RepoRecordedInput, String> recordedInputValues,
       SkyKey key)
       throws RepositoryFunctionException {
 

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

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.bazel.repository.starlark;
 
+import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState;
@@ -71,12 +72,12 @@ enum Signal {
   @Nullable volatile Future<RepositoryDirectoryValue.Builder> workerFuture = null;
 
   /**
-   * This is where the {@code markerData} for the whole invocation is collected.
+   * This is where the recorded inputs & values for the whole invocation is collected.
    *
    * <p>{@link com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction} creates a
    * new map on each restart, so we can't simply plumb that in.
    */
-  final Map<String, String> markerData = new TreeMap<>();
+  final Map<RepoRecordedInput, String> recordedInputValues = new TreeMap<>();
 
   SkyFunction.Environment signalForFreshEnv() throws InterruptedException {
     signalQueue.put(Signal.RESTART);

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

@@ -39,6 +39,7 @@
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException;
+import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.WorkspaceFileHelper;
@@ -55,8 +56,6 @@
 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;
@@ -69,8 +68,6 @@
 
 /** A repository function to delegate work done by Starlark remote repositories. */
 public final class StarlarkRepositoryFunction extends RepositoryFunction {
-  static final String SEMANTICS = "STARLARK_SEMANTICS";
-
   private final DownloadManager downloadManager;
   private double timeoutScaling = 1.0;
   @Nullable private ExecutorService workerExecutorService = null;
@@ -98,26 +95,6 @@ public void setWorkerExecutorService(@Nullable ExecutorService workerExecutorSer
     this.workerExecutorService = workerExecutorService;
   }
 
-  static String describeSemantics(StarlarkSemantics semantics) {
-    // Here we use the hash code provided by AutoValue. This is unique, as long
-    // as the number of bits in the StarlarkSemantics is small enough. We will have to
-    // move to a longer description once the number of flags grows too large.
-    return "" + semantics.hashCode();
-  }
-
-  @Override
-  protected boolean verifySemanticsMarkerData(Map<String, String> markerData, Environment env)
-      throws InterruptedException {
-    StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
-    if (starlarkSemantics == null) {
-      // As it is a precomputed value, it should already be available. If not, returning
-      // false is the safe thing to do.
-      return false;
-    }
-
-    return describeSemantics(starlarkSemantics).equals(markerData.get(SEMANTICS));
-  }
-
   @Override
   protected void setupRepoRootBeforeFetching(Path repoRoot) throws RepositoryFunctionException {
     // DON'T delete the repo root here if we're using a worker thread, since when this SkyFunction
@@ -143,11 +120,11 @@ public RepositoryDirectoryValue.Builder fetch(
       Path outputDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> markerData,
+      Map<RepoRecordedInput, String> recordedInputValues,
       SkyKey key)
       throws RepositoryFunctionException, InterruptedException {
     if (workerExecutorService == null) {
-      return fetchInternal(rule, outputDirectory, directories, env, markerData, key);
+      return fetchInternal(rule, outputDirectory, directories, env, recordedInputValues, key);
     }
     var state = env.getState(RepoFetchingSkyKeyComputeState::new);
     var workerFuture = state.workerFuture;
@@ -161,7 +138,12 @@ public RepositoryDirectoryValue.Builder fetch(
               () -> {
                 try {
                   return fetchInternal(
-                      rule, outputDirectory, directories, workerEnv, state.markerData, key);
+                      rule,
+                      outputDirectory,
+                      directories,
+                      workerEnv,
+                      state.recordedInputValues,
+                      key);
                 } finally {
                   state.signalQueue.put(Signal.DONE);
                 }
@@ -178,7 +160,7 @@ public RepositoryDirectoryValue.Builder fetch(
       case DONE:
         try {
           RepositoryDirectoryValue.Builder result = workerFuture.get();
-          markerData.putAll(state.markerData);
+          recordedInputValues.putAll(state.recordedInputValues);
           return result;
         } catch (ExecutionException e) {
           Throwables.throwIfInstanceOf(e.getCause(), RepositoryFunctionException.class);
@@ -209,22 +191,21 @@ private RepositoryDirectoryValue.Builder fetchInternal(
       Path outputDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> markerData,
+      Map<RepoRecordedInput, String> recordedInputValues,
       SkyKey key)
       throws RepositoryFunctionException, InterruptedException {
 
     String defInfo = RepositoryResolvedEvent.getRuleDefinitionInformation(rule);
     env.getListener().post(new StarlarkRepositoryDefinitionLocationEvent(rule.getName(), defInfo));
 
     StarlarkCallable function = rule.getRuleClassObject().getConfiguredTargetFunction();
-    if (declareEnvironmentDependencies(markerData, env, getEnviron(rule)) == null) {
+    if (declareEnvironmentDependencies(recordedInputValues, env, getEnviron(rule)) == null) {
       return null;
     }
     StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
     if (env.valuesMissing()) {
       return null;
     }
-    markerData.put(SEMANTICS, describeSemantics(starlarkSemantics));
 
     PathPackageLocator packageLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
     if (env.valuesMissing()) {
@@ -324,22 +305,20 @@ private RepositoryDirectoryValue.Builder fetchInternal(
       // Modify marker data to include the files used by the rule's implementation function.
       for (Map.Entry<Label, String> entry :
           starlarkRepositoryContext.getAccumulatedFileDigests().entrySet()) {
-        // A label does not contain spaces so it's safe to use as a key.
-        markerData.put("FILE:" + entry.getKey(), entry.getValue());
+        recordedInputValues.put(new RepoRecordedInput.LabelFile(entry.getKey()), entry.getValue());
       }
 
       // Ditto for environment variables accessed via `getenv`.
       for (String envKey : starlarkRepositoryContext.getAccumulatedEnvKeys()) {
-        markerData.put("ENV:" + envKey, clientEnvironment.get(envKey));
+        recordedInputValues.put(
+            new RepoRecordedInput.EnvVar(envKey), clientEnvironment.get(envKey));
       }
 
       for (Table.Cell<RepositoryName, String, RepositoryName> repoMappings :
           repoMappingRecorder.recordedEntries().cellSet()) {
-        markerData.put(
-            "REPO_MAPPING:"
-                + repoMappings.getRowKey().getName()
-                + ","
-                + repoMappings.getColumnKey(),
+        recordedInputValues.put(
+            new RepoRecordedInput.RecordedRepoMapping(
+                repoMappings.getRowKey(), repoMappings.getColumnKey()),
             repoMappings.getValue().getName());
       }
 
@@ -393,47 +372,6 @@ 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/bazel/rules/android/AndroidNdkRepositoryFunction.java

@@ -38,6 +38,7 @@
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
+import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
 import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
@@ -247,13 +248,14 @@ public boolean isLocal(Rule rule) {
   }
 
   @Override
-  public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Environment env)
+  public boolean verifyRecordedInputs(
+      Rule rule, Map<RepoRecordedInput, String> recordedInputValues, Environment env)
       throws InterruptedException {
     WorkspaceAttributeMapper attributes = WorkspaceAttributeMapper.of(rule);
     if (attributes.isAttributeValueExplicitlySpecified("path")) {
       return true;
     }
-    return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_SET);
+    return super.verifyRecordedInputs(rule, recordedInputValues, env);
   }
 
   @Override
@@ -263,11 +265,11 @@ public RepositoryDirectoryValue.Builder fetch(
       Path outputDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> markerData,
+      Map<RepoRecordedInput, String> recordedInputValues,
       SkyKey key)
       throws InterruptedException, RepositoryFunctionException {
     Map<String, String> environ =
-        declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET);
+        declareEnvironmentDependencies(recordedInputValues, env, PATH_ENV_VAR_AS_SET);
     if (environ == null) {
       return null;
     }

src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.io.InconsistentFilesystemException;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Type;
+import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
 import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
@@ -177,13 +178,14 @@ public boolean isLocal(Rule rule) {
   }
 
   @Override
-  public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Environment env)
+  public boolean verifyRecordedInputs(
+      Rule rule, Map<RepoRecordedInput, String> recordedInputValues, Environment env)
       throws InterruptedException {
     WorkspaceAttributeMapper attributes = WorkspaceAttributeMapper.of(rule);
     if (attributes.isAttributeValueExplicitlySpecified("path")) {
       return true;
     }
-    return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_SET);
+    return super.verifyRecordedInputs(rule, recordedInputValues, env);
   }
 
   @Override
@@ -193,11 +195,11 @@ public RepositoryDirectoryValue.Builder fetch(
       final Path outputDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> markerData,
+      Map<RepoRecordedInput, String> recordedInputValues,
       SkyKey key)
       throws RepositoryFunctionException, InterruptedException {
     Map<String, String> environ =
-        declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET);
+        declareEnvironmentDependencies(recordedInputValues, env, PATH_ENV_VAR_AS_SET);
     if (environ == null) {
       return null;
     }

src/main/java/com/google/devtools/build/lib/rules/BUILD

@@ -382,6 +382,7 @@ java_library(
     srcs = [
         "repository/LocalRepositoryFunction.java",
         "repository/NeedsSkyframeRestartException.java",
+        "repository/RepoRecordedInput.java",
         "repository/RepositoryDelegatorFunction.java",
         "repository/RepositoryDirectoryDirtinessChecker.java",
         "repository/RepositoryFunction.java",
@@ -412,6 +413,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/repository:repository_events",
         "//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function",
         "//src/main/java/com/google/devtools/build/lib/skyframe:already_reported_exception",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function",
         "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",

src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java

@@ -43,7 +43,7 @@ public RepositoryDirectoryValue.Builder fetch(
       Path outputDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> markerData,
+      Map<RepoRecordedInput, String> recordedInputValues,
       SkyKey key)
       throws InterruptedException, RepositoryFunctionException {
     String userDefinedPath = RepositoryFunction.getPathAttr(rule);

src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java

@@ -59,7 +59,7 @@ public RepositoryDirectoryValue.Builder fetch(
       Path outputDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> markerData,
+      Map<RepoRecordedInput, String> recordedInputValues,
       SkyKey key)
       throws InterruptedException, RepositoryFunctionException {
 
@@ -161,7 +161,7 @@ public RepositoryDirectoryValue.Builder fetch(
       return null;
     }
 
-    fileHandler.finishFile(rule, outputDirectory, markerData);
+    fileHandler.finishFile(rule, outputDirectory, recordedInputValues);
     env.getListener().post(resolve(rule));
 
     return RepositoryDirectoryValue.builder()

src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java

@@ -60,10 +60,11 @@ public boolean prepareFile(Rule rule, Environment env)
     return true;
   }
 
-  public void finishFile(Rule rule, Path outputDirectory, Map<String, String> markerData)
+  public void finishFile(
+      Rule rule, Path outputDirectory, Map<RepoRecordedInput, String> recordedInputValues)
       throws RepositoryFunctionException {
-    this.workspaceFileHandler.finishFile(rule, outputDirectory, markerData);
-    this.buildFileHandler.finishFile(rule, outputDirectory, markerData);
+    this.workspaceFileHandler.finishFile(rule, outputDirectory, recordedInputValues);
+    this.buildFileHandler.finishFile(rule, outputDirectory, recordedInputValues);
   }
 
   /**
@@ -139,14 +140,16 @@ public boolean prepareFile(Rule rule, Environment env)
      * @throws IllegalStateException if {@link #prepareFile} was not called before this, or if
      *     {@link #prepareFile} failed and this was called.
      */
-    public void finishFile(Rule rule, Path outputDirectory, Map<String, String> markerData)
+    public void finishFile(
+        Rule rule, Path outputDirectory, Map<RepoRecordedInput, String> recordedInputValues)
         throws RepositoryFunctionException {
       if (fileValue != null) {
         // Link x/FILENAME to <build_root>/x.FILENAME.
         symlinkFile(fileValue, filename, outputDirectory);
-        String fileKey = getFileAttributeAsLabel(rule).toString();
         try {
-          markerData.put("FILE:" + fileKey, RepositoryFunction.fileValueToMarkerValue(fileValue));
+          recordedInputValues.put(
+              new RepoRecordedInput.LabelFile(getFileAttributeAsLabel(rule)),
+              RepositoryFunction.fileValueToMarkerValue(fileValue));
         } catch (IOException e) {
           throw new RepositoryFunctionException(e, Transience.TRANSIENT);
         }