Refactor marker file logic

Marker files today store all predeclared inputs as one hash (on the first line of the file), and then each recorded input as a following line in the `TYPE:KEY VALUE` format. This commit refactors the parsing/stringification logic of recorded inputs so that they're not all clumped in big methods in `RepositoryFunction`, to pave the way for more recorded input types (watching directories, etc) and more places to write recorded input data (for the true repo cache).

- The StarlarkSemantics object is no longer treated as a recorded input (only recorded for Starlark repo rules, ignored for native repo rules), but as a predeclared input instead (i.e. hashed on the first line).
  - This slightly simplifies logic, and since the existing native repo rules are either local (local_repository, new_local_repository, local_config_platform) or being Starlarkified (the two Android repo rules), it will have minimal visible impact.
- Each type of recorded inputs is a subclass of `RepoRecordedInput`, which knows how to stringify itself, verify its own up-to-date-ness, and how to parse itself from a string.
- We also try to collect as many SkyKeys needed to verify up-to-date-ness as possible in one go and do a mass Skyframe evaluation. This avoids a fair amount of Skyframe restarts (unlikely to have super big impact on performance, but is a nice thing to do).

Work towards #20952 and #12227.

Closes #21182.

PiperOrigin-RevId: 604522692
Change-Id: Idc18ab202adb601cda47914c48642a6c9039da40

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;
@@ -56,8 +57,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;
@@ -70,8 +69,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;
@@ -99,26 +96,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
@@ -144,7 +121,7 @@ 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
@@ -155,7 +132,7 @@ public RepositoryDirectoryValue.Builder fetch(
  // Fortunately, no Skyframe restarts should happen during error bubbling anyway, so this
  // shouldn't be a performance concern. See https://github.com/bazelbuild/bazel/issues/21238
  // for more context.
- 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;
@@ -169,7 +146,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);
  }
@@ -186,7 +168,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);
@@ -217,23 +199,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));
- markerData.put("ARCH:", CPU.getCurrent().getCanonicalName());
 
  PathPackageLocator packageLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
  if (env.valuesMissing()) {
@@ -333,22 +313,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());
  }
 
@@ -402,47 +380,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");
@@ -463,6 +400,7 @@ public static boolean isConfigureRule(Rule rule) {
  return rule.getRuleClassObject().isStarlark() && ((Boolean) rule.getAttr("$configure"));
  }
 
+ @Nullable
  @Override
  public Class<? extends RuleDefinition> getRuleDefinition() {
  return null; // unused so safe to return null

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;
@@ -60,6 +61,7 @@
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
@@ -75,7 +77,7 @@ public class AndroidNdkRepositoryFunction extends AndroidRepositoryFunction {
 
  private static final ImmutableSet<String> PATH_ENV_VAR_AS_SET = ImmutableSet.of(PATH_ENV_VAR);
 
- private static String getDefaultCrosstool(Integer majorRevision) {
+ private static String getDefaultCrosstool(int majorRevision) {
  // From NDK 17, libc++ replaces gnu-libstdc++ as the default STL.
  return majorRevision <= 16 ? GnuLibStdCppStlImpl.NAME : LibCppStlImpl.NAME;
  }
@@ -181,7 +183,7 @@ private static String createCcToolchainRule(
  // but the gcc tool will actually be clang.
  ToolPath gcc = null;
  for (ToolPath toolPath : toolchain.getToolPathList()) {
- if ("gcc".equals(toolPath.getName())) {
+ if (toolPath.getName().equals("gcc")) {
  gcc = toolPath;
  }
  }
@@ -258,13 +260,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
@@ -274,11 +277,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;
  }
@@ -521,7 +524,7 @@ private ImmutableList<String> generateBzlConfigFor(String version, CToolchain to
  tp ->
  String.format(
  " %s_path = '%s'",
- tp.getName().toLowerCase().replaceAll("-", "_"), tp.getPath()))
+ tp.getName().toLowerCase(Locale.ROOT).replaceAll("-", "_"), tp.getPath()))
  .collect(ImmutableList.toImmutableList()));
  return bigConditional.add("").build();
  }

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

@@ -381,6 +381,7 @@ java_library(
  srcs = [
  "repository/LocalRepositoryFunction.java",
  "repository/NeedsSkyframeRestartException.java",
+ "repository/RepoRecordedInput.java",
  "repository/RepositoryDelegatorFunction.java",
  "repository/RepositoryDirectoryDirtinessChecker.java",
  "repository/RepositoryFunction.java",
@@ -411,6 +412,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);