From f13e7d14b6de0bad17ae430c3d55f8e07d0fd414 Mon Sep 17 00:00:00 2001 From: Xdng Yng Date: Mon, 5 Feb 2024 20:58:39 -0800 Subject: [PATCH] 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 https://github.com/bazelbuild/bazel/issues/20952 and https://github.com/bazelbuild/bazel/issues/12227. Closes #21182. PiperOrigin-RevId: 604522692 Change-Id: Idc18ab202adb601cda47914c48642a6c9039da40 --- .../LocalConfigPlatformFunction.java | 3 +- .../RepoFetchingSkyKeyComputeState.java | 5 +- .../starlark/StarlarkRepositoryFunction.java | 100 +----- .../android/AndroidNdkRepositoryFunction.java | 17 +- .../android/AndroidSdkRepositoryFunction.java | 10 +- .../com/google/devtools/build/lib/rules/BUILD | 2 + .../repository/LocalRepositoryFunction.java | 2 +- .../NewLocalRepositoryFunction.java | 4 +- .../repository/NewRepositoryFileHandler.java | 15 +- .../rules/repository/RepoRecordedInput.java | 327 ++++++++++++++++++ .../RepositoryDelegatorFunction.java | 59 ++-- .../rules/repository/RepositoryFunction.java | 166 +-------- .../repository/RepositoryFunctionTest.java | 2 +- 13 files changed, 432 insertions(+), 280 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java index 9d2750c6fb2846..ef55022af8ceea 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java +++ b/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 markerData, + Map recordedInputValues, SkyKey key) throws RepositoryFunctionException { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingSkyKeyComputeState.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingSkyKeyComputeState.java index a709603c830f05..19ce5cb66d849f 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingSkyKeyComputeState.java +++ b/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 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. * *

{@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 markerData = new TreeMap<>(); + final Map recordedInputValues = new TreeMap<>(); SkyFunction.Environment signalForFreshEnv() throws InterruptedException { signalQueue.put(Signal.RESTART); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index 1137d9d9bc8c5f..85789495323c89 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/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 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 markerData, + Map 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,7 +199,7 @@ private RepositoryDirectoryValue.Builder fetchInternal( Path outputDirectory, BlazeDirectories directories, Environment env, - Map markerData, + Map recordedInputValues, SkyKey key) throws RepositoryFunctionException, InterruptedException { @@ -225,15 +207,13 @@ private RepositoryDirectoryValue.Builder fetchInternal( 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 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 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 getEnviron(Rule rule) { return ImmutableSet.copyOf((Iterable) 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 markerData, Environment env, Set 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 markerKeys = - markerData.keySet().stream() - .filter(s -> s.startsWith("ENV:")) - .collect(ImmutableSet.toImmutableSet()); - - ImmutableMap 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 getRuleDefinition() { return null; // unused so safe to return null diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java index 0a2af050d0881a..a974c12f83be51 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java +++ b/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 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 markerData, Environment env) + public boolean verifyRecordedInputs( + Rule rule, Map 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 markerData, + Map recordedInputValues, SkyKey key) throws InterruptedException, RepositoryFunctionException { Map 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 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(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java index 04c955c2ddc965..7862cc453c76e5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java +++ b/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 markerData, Environment env) + public boolean verifyRecordedInputs( + Rule rule, Map 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 markerData, + Map recordedInputValues, SkyKey key) throws RepositoryFunctionException, InterruptedException { Map environ = - declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET); + declareEnvironmentDependencies(recordedInputValues, env, PATH_ENV_VAR_AS_SET); if (environ == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index 9764394f6e2789..460d447a028de7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java index 811b274677475c..2b5482ea82e3f9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java +++ b/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 markerData, + Map recordedInputValues, SkyKey key) throws InterruptedException, RepositoryFunctionException { String userDefinedPath = RepositoryFunction.getPathAttr(rule); diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java index e265e3c1685538..7bc6c444cebd88 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java +++ b/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 markerData, + Map 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() diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java index 37c15b1c3fdf9e..8f2100ae4a6a73 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java +++ b/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 markerData) + public void finishFile( + Rule rule, Path outputDirectory, Map 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 markerData) + public void finishFile( + Rule rule, Path outputDirectory, Map recordedInputValues) throws RepositoryFunctionException { if (fileValue != null) { // Link x/FILENAME to /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); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java new file mode 100644 index 00000000000000..512335744419e0 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -0,0 +1,327 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.rules.repository; + +import com.google.common.base.Splitter; +import com.google.devtools.build.lib.actions.FileValue; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelValidator; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; +import com.google.devtools.build.lib.skyframe.ClientEnvironmentValue; +import com.google.devtools.build.lib.skyframe.PackageLookupValue; +import com.google.devtools.build.lib.skyframe.PrecomputedValue; +import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Root; +import com.google.devtools.build.lib.vfs.RootedPath; +import com.google.devtools.build.skyframe.SkyFunction.Environment; +import com.google.devtools.build.skyframe.SkyKey; +import java.io.IOException; +import java.util.Comparator; +import java.util.List; +import java.util.Objects; +import javax.annotation.Nullable; + +/** + * Represents a "recorded input" of a repo fetch. We define the "input" of a repo fetch as any + * entity that could affect the output of the repo fetch (i.e. the repo contents). A "recorded + * input" is thus any input we can record during the fetch and thus know about only after the fetch. + * This contrasts with "predeclared inputs", which are known before fetching the repo, and + * "undiscoverable inputs", which are used during the fetch but is not recorded or recordable. + * + *

Recorded inputs are of particular interest, since in order to determine whether a fetched repo + * is still up-to-date, the identity of all recorded inputs need to be stored in addition to their + * values. This contrasts with predeclared inputs; the whole set of predeclared inputs are known + * before the fetch, so we can simply hash all predeclared input values. + * + *

Recorded inputs and their values are stored in marker files for repos. Each recorded + * input is stored as a string, with a prefix denoting its type, followed by a colon, and then the + * information identifying that specific input. + */ +public abstract class RepoRecordedInput implements Comparable { + /** Represents a parser for a specific type of recorded inputs. */ + public abstract static class Parser { + /** + * The prefix that identifies the type of the recorded inputs: for example, the {@code ENV} part + * of {@code ENV:MY_ENV_VAR}. + */ + public abstract String getPrefix(); + + /** + * Parses a recorded input from the post-colon substring that identifies the specific input: for + * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. + */ + public abstract RepoRecordedInput parse(String s, Path baseDirectory); + } + + private static final Comparator COMPARATOR = + Comparator.comparing((RepoRecordedInput rri) -> rri.getParser().getPrefix()) + .thenComparing(RepoRecordedInput::toStringInternal); + + /** + * Parses a recorded input from its string representation. + * + * @param s the string representation + * @param baseDirectory the path to a base directory that any filesystem paths should be resolved + * relative to + * @return The parsed recorded input object, or {@code null} if the string representation is + * invalid + */ + @Nullable + public static RepoRecordedInput parse(String s, Path baseDirectory) { + List parts = Splitter.on(':').limit(2).splitToList(s); + for (Parser parser : new Parser[] {File.PARSER, EnvVar.PARSER, RecordedRepoMapping.PARSER}) { + if (parts.get(0).equals(parser.getPrefix())) { + return parser.parse(parts.get(1), baseDirectory); + } + } + return null; + } + + @Override + public final String toString() { + return getParser().getPrefix() + ":" + toStringInternal(); + } + + @Override + public int compareTo(RepoRecordedInput o) { + return COMPARATOR.compare(this, o); + } + + /** + * Returns the post-colon substring that identifies the specific input: for example, the {@code + * MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. + */ + abstract String toStringInternal(); + + /** Returns the parser object for this type of recorded inputs. */ + public abstract Parser getParser(); + + /** Returns the {@link SkyKey} that is necessary to determine {@link #isUpToDate}. */ + public abstract SkyKey getSkyKey(); + + /** + * Returns whether the given {@code oldValue} is still up-to-date for this recorded input. This + * method can assume that {@link #getSkyKey()} is already evaluated; it can request further + * Skyframe evaluations, and if any values are missing, this method can return any value (doesn't + * matter what) and will be reinvoked after a Skyframe restart. + */ + public abstract boolean isUpToDate(Environment env, @Nullable String oldValue) + throws InterruptedException; + + /** Represents a file input accessed during the repo fetch. */ + public abstract static class File extends RepoRecordedInput { + static final Parser PARSER = + new Parser() { + @Override + public String getPrefix() { + return "FILE"; + } + + @Override + public RepoRecordedInput parse(String s, Path baseDirectory) { + if (LabelValidator.isAbsolute(s)) { + return new LabelFile(Label.parseCanonicalUnchecked(s)); + } + Path path = baseDirectory.getRelative(s); + return new AbsolutePathFile( + RootedPath.toRootedPath( + Root.fromPath(path.getParentDirectory()), + PathFragment.create(path.getBaseName()))); + } + }; + + @Override + public Parser getParser() { + return PARSER; + } + } + + /** Represents a file input accessed during the repo fetch that is addressable by a label. */ + public static final class LabelFile extends File { + final Label label; + + public LabelFile(Label label) { + this.label = label; + } + + @Override + String toStringInternal() { + return label.toString(); + } + + @Override + public SkyKey getSkyKey() { + return PackageLookupValue.key(label.getPackageIdentifier()); + } + + @Override + public boolean isUpToDate(Environment env, @Nullable String oldValue) + throws InterruptedException { + PackageLookupValue pkgLookupValue = (PackageLookupValue) env.getValue(getSkyKey()); + if (pkgLookupValue == null || !pkgLookupValue.packageExists()) { + return false; + } + RootedPath rootedPath = + RootedPath.toRootedPath(pkgLookupValue.getRoot(), label.toPathFragment()); + SkyKey fileKey = FileValue.key(rootedPath); + try { + FileValue fileValue = (FileValue) env.getValueOrThrow(fileKey, IOException.class); + if (fileValue == null || !fileValue.isFile() || fileValue.isSpecialFile()) { + return false; + } + return oldValue.equals(RepositoryFunction.fileValueToMarkerValue(fileValue)); + } catch (IOException e) { + return false; + } + } + } + + /** + * Represents a file input accessed during the repo fetch that is not addressable by a + * label. This most likely means that it's outside any known Bazel workspace. + */ + public static final class AbsolutePathFile extends File { + final RootedPath path; + + public AbsolutePathFile(RootedPath path) { + this.path = path; + } + + @Override + String toStringInternal() { + return path.asPath().getPathString(); + } + + @Override + public SkyKey getSkyKey() { + return FileValue.key(path); + } + + @Override + public boolean isUpToDate(Environment env, @Nullable String oldValue) + throws InterruptedException { + try { + FileValue fileValue = (FileValue) env.getValueOrThrow(getSkyKey(), IOException.class); + if (fileValue == null || !fileValue.isFile() || fileValue.isSpecialFile()) { + return false; + } + return oldValue.equals(RepositoryFunction.fileValueToMarkerValue(fileValue)); + } catch (IOException e) { + return false; + } + } + } + + /** Represents an environment variable accessed during the repo fetch. */ + public static final class EnvVar extends RepoRecordedInput { + static final Parser PARSER = + new Parser() { + @Override + public String getPrefix() { + return "ENV"; + } + + @Override + public RepoRecordedInput parse(String s, Path baseDirectory) { + return new EnvVar(s); + } + }; + + final String name; + + public EnvVar(String name) { + this.name = name; + } + + @Override + public Parser getParser() { + return PARSER; + } + + @Override + String toStringInternal() { + return name; + } + + @Override + public SkyKey getSkyKey() { + return ActionEnvironmentFunction.key(name); + } + + @Override + public boolean isUpToDate(Environment env, @Nullable String oldValue) + throws InterruptedException { + String v = PrecomputedValue.REPO_ENV.get(env).get(name); + if (v == null) { + v = ((ClientEnvironmentValue) env.getValue(getSkyKey())).getValue(); + } + // Note that `oldValue` can be null if the env var was not set. + return Objects.equals(oldValue, v); + } + } + + /** Represents a repo mapping entry that was used during the repo fetch. */ + public static final class RecordedRepoMapping extends RepoRecordedInput { + static final Parser PARSER = + new Parser() { + @Override + public String getPrefix() { + return "REPO_MAPPING"; + } + + @Override + public RepoRecordedInput parse(String s, Path baseDirectory) { + List parts = Splitter.on(',').limit(2).splitToList(s); + return new RecordedRepoMapping( + RepositoryName.createUnvalidated(parts.get(0)), parts.get(1)); + } + }; + + final RepositoryName sourceRepo; + final String apparentName; + + public RecordedRepoMapping(RepositoryName sourceRepo, String apparentName) { + this.sourceRepo = sourceRepo; + this.apparentName = apparentName; + } + + @Override + public Parser getParser() { + return PARSER; + } + + @Override + String toStringInternal() { + return sourceRepo.getName() + ',' + apparentName; + } + + @Override + public SkyKey getSkyKey() { + return RepositoryMappingValue.key(sourceRepo); + } + + @Override + public boolean isUpToDate(Environment env, @Nullable String oldValue) + throws InterruptedException { + RepositoryMappingValue repoMappingValue = (RepositoryMappingValue) env.getValue(getSkyKey()); + return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE + && RepositoryName.createUnvalidated(oldValue) + .equals(repoMappingValue.getRepositoryMapping().get(apparentName)); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 87046be6d5c732..0df1146b778400 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -165,7 +165,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) String.format("'%s' is not a repository rule", repositoryName.getCanonicalForm())); } - DigestWriter digestWriter = new DigestWriter(directories, repositoryName, rule); + DigestWriter digestWriter = + new DigestWriter(directories, repositoryName, rule, starlarkSemantics); if (shouldUseVendorRepos(env, handler, rule)) { RepositoryDirectoryValue repositoryDirectoryValue = tryGettingValueUsingVendoredRepo( @@ -202,7 +203,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) // only recreate it after fetching is done to prevent this scenario. DigestWriter.clearMarkerFile(directories, repositoryName); RepositoryDirectoryValue.Builder builder = - fetchRepository(skyKey, repoRoot, env, digestWriter.getMarkerData(), handler, rule); + fetchRepository( + skyKey, repoRoot, env, digestWriter.getRecordedInputValues(), handler, rule); if (builder == null) { return null; } @@ -405,7 +407,7 @@ private RepositoryDirectoryValue.Builder fetchRepository( SkyKey skyKey, Path repoRoot, Environment env, - Map markerData, + Map recordedInputValues, RepositoryFunction handler, Rule rule) throws InterruptedException, RepositoryFunctionException { @@ -417,7 +419,7 @@ private RepositoryDirectoryValue.Builder fetchRepository( RepositoryDirectoryValue.Builder repoBuilder; try { - repoBuilder = handler.fetch(rule, repoRoot, directories, env, markerData, skyKey); + repoBuilder = handler.fetch(rule, repoRoot, directories, env, recordedInputValues, skyKey); } catch (RepositoryFunctionException e) { // Upon an exceptional exit, the fetching of that repository is over as well. env.getListener().post(RepositoryFetchProgress.finished(repoName)); @@ -587,22 +589,26 @@ private static class DigestWriter { private final Path markerPath; private final Rule rule; // not just Map<> to signal that iteration order must be deterministic - private final TreeMap markerData; + private final TreeMap recordedInputValues; private final String ruleKey; - DigestWriter(BlazeDirectories directories, RepositoryName repositoryName, Rule rule) { - ruleKey = computeRuleKey(rule); + DigestWriter( + BlazeDirectories directories, + RepositoryName repositoryName, + Rule rule, + StarlarkSemantics starlarkSemantics) { + ruleKey = computeRuleKey(rule, starlarkSemantics); markerPath = getMarkerPath(directories, repositoryName.getName()); this.rule = rule; - markerData = Maps.newTreeMap(); + recordedInputValues = Maps.newTreeMap(); } byte[] writeMarkerFile() throws RepositoryFunctionException { StringBuilder builder = new StringBuilder(); builder.append(ruleKey).append("\n"); - for (Map.Entry data : markerData.entrySet()) { - String key = data.getKey(); - String value = data.getValue(); + for (Map.Entry recordedInput : recordedInputValues.entrySet()) { + String key = recordedInput.getKey().toString(); + String value = recordedInput.getValue(); builder.append(escape(key)).append(" ").append(escape(value)).append("\n"); } String content = builder.toString(); @@ -639,14 +645,15 @@ byte[] areRepositoryAndMarkerFileConsistent( return null; } - Map markerData = new TreeMap<>(); + Path baseDirectory = rule.getPackage().getPackageDirectory(); + Map recordedInputValues = new TreeMap<>(); String content; try { content = FileSystemUtils.readContent(markerPath, UTF_8); - String markerRuleKey = readMarkerFile(content, markerData); + String markerRuleKey = readMarkerFile(content, baseDirectory, recordedInputValues); boolean verified = false; if (Preconditions.checkNotNull(ruleKey).equals(markerRuleKey)) { - verified = handler.verifyMarkerData(rule, markerData, env); + verified = handler.verifyRecordedInputs(rule, recordedInputValues, env); if (env.valuesMissing()) { return null; } @@ -662,12 +669,13 @@ byte[] areRepositoryAndMarkerFileConsistent( } } - Map getMarkerData() { - return markerData; + Map getRecordedInputValues() { + return recordedInputValues; } @Nullable - private String readMarkerFile(String content, Map markerData) { + private static String readMarkerFile( + String content, Path baseDirectory, Map recordedInputValues) { String markerRuleKey = null; Iterable lines = Splitter.on('\n').split(content); @@ -678,22 +686,27 @@ private String readMarkerFile(String content, Map markerData) { firstLine = false; } else { int sChar = line.indexOf(' '); - String key = line; - String value = ""; if (sChar > 0) { - key = unescape(line.substring(0, sChar)); - value = unescape(line.substring(sChar + 1)); + RepoRecordedInput input = + RepoRecordedInput.parse(unescape(line.substring(0, sChar)), baseDirectory); + if (input == null) { + // ignore invalid entries. + continue; + } + recordedInputValues.put(input, unescape(line.substring(sChar + 1))); } - markerData.put(key, value); } } return markerRuleKey; } - private String computeRuleKey(Rule rule) { + private String computeRuleKey(Rule rule, StarlarkSemantics starlarkSemantics) { return new Fingerprint() .addBytes(RuleFormatter.serializeRule(rule).build().toByteArray()) .addInt(MARKER_FILE_VERSION) + // TODO: Using the hashCode() method for StarlarkSemantics here is suboptimal as + // it doesn't include any default values. + .addInt(starlarkSemantics.hashCode()) .hexDigestAndReset(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index ac0e193dabf893..9aa7f860412b9e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -27,8 +27,6 @@ import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelConstants; -import com.google.devtools.build.lib.cmdline.LabelSyntaxException; -import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.packages.NoSuchPackageException; @@ -41,7 +39,6 @@ import com.google.devtools.build.lib.skyframe.PackageLookupFunction; import com.google.devtools.build.lib.skyframe.PackageLookupValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; -import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -52,12 +49,8 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyframeLookupResult; import java.io.IOException; -import java.util.Collection; -import java.util.LinkedHashMap; import java.util.Map; -import java.util.Objects; import java.util.Set; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; @@ -178,8 +171,8 @@ public void reportSkyframeRestart(Environment env, RepositoryName repoName) { *

The {@code markerData} argument can be mutated to augment the data to write to the * repository marker file. If any data in the {@code markerData} change between 2 execute of the * {@link RepositoryDelegatorFunction} then this should be a reason to invalidate the repository. - * The {@link #verifyMarkerData} method is responsible for checking the value added to that map - * when checking the content of a marker file. + * The {@link #verifyRecordedInputs} method is responsible for checking the value added to that + * map when checking the content of a marker file. */ @ThreadSafe @Nullable @@ -188,103 +181,33 @@ public abstract RepositoryDirectoryValue.Builder fetch( Path outputDirectory, BlazeDirectories directories, Environment env, - Map markerData, + Map recordedInputValues, SkyKey key) throws InterruptedException, RepositoryFunctionException; - @SuppressWarnings("unchecked") - private static ImmutableSet getEnviron(Rule rule) { - if (rule.isAttrDefined("$environ", Type.STRING_LIST)) { - return ImmutableSet.copyOf((Collection) rule.getAttr("$environ")); - } - return ImmutableSet.of(); - } - /** * Verify the data provided by the marker file to check if a refetch is needed. Returns true if * the data is up to date and no refetch is needed and false if the data is obsolete and a refetch * is needed. */ - public boolean verifyMarkerData(Rule rule, Map markerData, Environment env) + public boolean verifyRecordedInputs( + Rule rule, Map recordedInputValues, Environment env) throws InterruptedException { - return verifyEnvironMarkerData(markerData, env, getEnviron(rule)) - && verifySemanticsMarkerData(markerData, env) - && verifyRepoMappingMarkerData(markerData, env) - && verifyMarkerDataForFiles(rule, markerData, env); - } - - protected boolean verifySemanticsMarkerData(Map markerData, Environment env) - throws InterruptedException { - return true; - } - - private static boolean verifyRepoMappingMarkerData( - Map markerData, Environment env) throws InterruptedException { ImmutableSet skyKeys = - markerData.keySet().stream() - .filter(k -> k.startsWith("REPO_MAPPING:")) - // grab the part after the 'REPO_MAPPING:' and before the comma - .map(k -> k.substring("REPO_MAPPING:".length(), k.indexOf(','))) - .map(k -> RepositoryMappingValue.key(RepositoryName.createUnvalidated(k))) + recordedInputValues.keySet().stream() + .map(RepoRecordedInput::getSkyKey) .collect(toImmutableSet()); - SkyframeLookupResult result = env.getValuesAndExceptions(skyKeys); + env.getValuesAndExceptions(skyKeys); if (env.valuesMissing()) { return false; } - for (Map.Entry entry : markerData.entrySet()) { - if (!entry.getKey().startsWith("REPO_MAPPING:")) { - continue; - } - int commaIndex = entry.getKey().indexOf(','); - RepositoryName fromRepo = - RepositoryName.createUnvalidated( - entry.getKey().substring("REPO_MAPPING:".length(), commaIndex)); - String apparentRepoName = entry.getKey().substring(commaIndex + 1); - RepositoryMappingValue repoMappingValue = - (RepositoryMappingValue) result.get(RepositoryMappingValue.key(fromRepo)); - if (repoMappingValue == RepositoryMappingValue.NOT_FOUND_VALUE - || !RepositoryName.createUnvalidated(entry.getValue()) - .equals(repoMappingValue.getRepositoryMapping().get(apparentRepoName))) { + for (Map.Entry recordedInputValue : recordedInputValues.entrySet()) { + if (!recordedInputValue.getKey().isUpToDate(env, recordedInputValue.getValue())) { return false; } } return true; } - - private static boolean verifyLabelMarkerData(Rule rule, String key, String value, Environment env) - throws InterruptedException { - Preconditions.checkArgument(key.startsWith("FILE:")); - try { - RootedPath rootedPath; - String fileKey = key.substring(5); - if (LabelValidator.isAbsolute(fileKey)) { - rootedPath = getRootedPathFromLabel(Label.parseCanonical(fileKey), env); - } else { - // TODO(pcloudy): Removing checking absolute path, they should all be absolute label. - PathFragment filePathFragment = PathFragment.create(fileKey); - Path file = rule.getPackage().getPackageDirectory().getRelative(filePathFragment); - rootedPath = - RootedPath.toRootedPath( - Root.fromPath(file.getParentDirectory()), PathFragment.create(file.getBaseName())); - } - - SkyKey fileSkyKey = FileValue.key(rootedPath); - FileValue fileValue = (FileValue) env.getValueOrThrow(fileSkyKey, IOException.class); - - if (fileValue == null || !fileValue.isFile() || fileValue.isSpecialFile()) { - return false; - } - - return Objects.equals(value, fileValueToMarkerValue(fileValue)); - } catch (LabelSyntaxException e) { - throw new IllegalStateException( - "Key " + key + " is not a correct file key (should be in form FILE:label)", e); - } catch (IOException | EvalException e) { - // Consider those exception to be a cause for invalidation - return false; - } - } - /** * Convert to a {@link com.google.devtools.build.lib.actions.FileValue} to a String appropriate * for placing in a repository marker file. @@ -302,18 +225,6 @@ public static String fileValueToMarkerValue(FileValue fileValue) throws IOExcept return BaseEncoding.base16().lowerCase().encode(digest); } - static boolean verifyMarkerDataForFiles( - Rule rule, Map markerData, Environment env) throws InterruptedException { - for (Map.Entry entry : markerData.entrySet()) { - if (entry.getKey().startsWith("FILE:")) { - if (!verifyLabelMarkerData(rule, entry.getKey(), entry.getValue(), env)) { - return false; - } - } - } - return true; - } - public static RootedPath getRootedPathFromLabel(Label label, Environment env) throws InterruptedException, EvalException { SkyKey pkgSkyKey = PackageLookupValue.key(label.getPackageIdentifier()); @@ -343,7 +254,7 @@ public static RootedPath getRootedPathFromLabel(Label label, Environment env) */ @Nullable protected Map declareEnvironmentDependencies( - Map markerData, Environment env, Set keys) + Map recordedInputValues, Environment env, Set keys) throws InterruptedException { if (keys.isEmpty()) { return ImmutableMap.of(); @@ -354,7 +265,9 @@ protected Map declareEnvironmentDependencies( return null; } // Add the dependencies to the marker file - keys.forEach(key -> markerData.put("ENV:" + key, envDep.get(key))); + for (String key : keys) { + recordedInputValues.put(new RepoRecordedInput.EnvVar(key), envDep.get(key)); + } return envDep; } @@ -382,57 +295,6 @@ public static ImmutableMap getEnvVarValues(Environment env, Set< return repoEnv.buildKeepingLast(); } - /** - * Verify marker data previously saved by {@link #declareEnvironmentDependencies(Map, Environment, - * Iterable)}. This function is to be called from a {@link #verifyMarkerData(Rule, Map, - * Environment)} function to verify the values for environment variables. - */ - protected boolean verifyEnvironMarkerData( - Map markerData, Environment env, Set keys) - throws InterruptedException { - if (keys.isEmpty()) { - return true; - } - - ImmutableMap environ = ActionEnvironmentFunction.getEnvironmentView(env, keys); - if (env.valuesMissing()) { - return false; // Returns false so caller knows to return immediately - } - - Map 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 repoEnv = new LinkedHashMap<>(environ); - for (String key : keys) { - String value = repoEnvOverride.get(key); - if (value != null) { - repoEnv.put(key, value); - } - } - - // Verify that all environment variable in the marker file are also in keys - for (String key : markerData.keySet()) { - if (key.startsWith("ENV:") && !keys.contains(key.substring(4))) { - return false; - } - } - - // Now verify the values of the marker data - for (String key : keys) { - if (!markerData.containsKey("ENV:" + key)) { - return false; - } - String markerValue = markerData.get("ENV:" + key); - if (!Objects.equals(markerValue, repoEnv.get(key))) { - return false; - } - } - return true; - } - /** * Whether fetching is done using local operations only. * diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java index d8f56d8eaae238..9a7f175d633e10 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java @@ -59,7 +59,7 @@ public RepositoryDirectoryValue.Builder fetch( Path outputDirectory, BlazeDirectories directories, SkyFunction.Environment env, - Map markerData, + Map recordedInputValues, SkyKey key) throws InterruptedException { return null;