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);
  }