diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java index 398df8266ffe65..b96e3eaa1f2850 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java @@ -47,7 +47,7 @@ public abstract class BazelLockFileValue implements SkyValue { // https://cs.opensource.google/bazel/bazel/+/release-7.3.0:src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java;l=120-127;drc=5f5355b75c7c93fba1e15f6658f308953f4baf51 // While this hack exists on 7.x, lockfile version increments should be done 2 at a time (i.e. // keep this number even). - public static final int LOCK_FILE_VERSION = 24; + public static final int LOCK_FILE_VERSION = 26; /** A valid empty lockfile. */ public static final BazelLockFileValue EMPTY_LOCKFILE = builder().build(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java index f509b01a194cfc..433124f1061085 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java @@ -20,11 +20,12 @@ import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_MAP; import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SET; import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SORTED_MAP; +import static com.google.devtools.build.lib.rules.repository.RepoRecordedInput.NeverUpToDateRepoRecordedInput.PARSE_FAILURE; +import static com.google.devtools.build.lib.util.StringEncoding.internalToUnicode; +import static com.google.devtools.build.lib.util.StringEncoding.unicodeToInternal; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableTable; -import com.google.common.collect.Table; import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException; import com.google.devtools.build.lib.bazel.repository.cache.DownloadCache; import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; @@ -293,113 +294,34 @@ public Optional read(JsonReader jsonReader) throws IOException { } } - /** - * Converts Guava tables into a JSON array of 3-tuples (one per cell). Each 3-tuple is a JSON - * array itself (rowKey, columnKey, value). For example, a JSON snippet could be: {@code [ - * ["row1", "col1", "value1"], ["row2", "col2", "value2"], ... ]} - */ - public static final TypeAdapterFactory IMMUTABLE_TABLE = - new TypeAdapterFactory() { - @Nullable - @Override - @SuppressWarnings("unchecked") - public TypeAdapter create(Gson gson, TypeToken typeToken) { - if (typeToken.getRawType() != ImmutableTable.class) { - return null; - } - Type type = typeToken.getType(); - if (!(type instanceof ParameterizedType)) { - return null; - } - Type[] typeArgs = ((ParameterizedType) typeToken.getType()).getActualTypeArguments(); - if (typeArgs.length != 3) { - return null; - } - var rowTypeAdapter = (TypeAdapter) gson.getAdapter(TypeToken.get(typeArgs[0])); - var colTypeAdapter = (TypeAdapter) gson.getAdapter(TypeToken.get(typeArgs[1])); - var valTypeAdapter = (TypeAdapter) gson.getAdapter(TypeToken.get(typeArgs[2])); - if (rowTypeAdapter == null || colTypeAdapter == null || valTypeAdapter == null) { - return null; - } - return (TypeAdapter) - new TypeAdapter>() { - @Override - public void write(JsonWriter jsonWriter, ImmutableTable t) - throws IOException { - jsonWriter.beginArray(); - for (Table.Cell cell : t.cellSet()) { - jsonWriter.beginArray(); - rowTypeAdapter.write(jsonWriter, cell.getRowKey()); - colTypeAdapter.write(jsonWriter, cell.getColumnKey()); - valTypeAdapter.write(jsonWriter, cell.getValue()); - jsonWriter.endArray(); - } - jsonWriter.endArray(); - } - - @Override - public ImmutableTable read(JsonReader jsonReader) - throws IOException { - var builder = ImmutableTable.builder(); - jsonReader.beginArray(); - while (jsonReader.peek() != JsonToken.END_ARRAY) { - jsonReader.beginArray(); - builder.put( - rowTypeAdapter.read(jsonReader), - colTypeAdapter.read(jsonReader), - valTypeAdapter.read(jsonReader)); - jsonReader.endArray(); - } - jsonReader.endArray(); - return builder.buildOrThrow(); - } - }; - } - }; - - private static final TypeAdapter REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER = + // This is needed because Bazel uses a custom String encoding internally, see StringEncoding for + // details. + public static final TypeAdapter STRING_ADAPTER = new TypeAdapter<>() { @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.File value) throws IOException { - jsonWriter.value(value.toStringInternal()); + public void write(JsonWriter jsonWriter, String s) throws IOException { + jsonWriter.value(internalToUnicode(s)); } @Override - public RepoRecordedInput.File read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.File) - RepoRecordedInput.File.PARSER.parse(jsonReader.nextString()); + public String read(JsonReader jsonReader) throws IOException { + return unicodeToInternal(jsonReader.nextString()); } }; - private static final TypeAdapter - REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER = - new TypeAdapter<>() { - @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.Dirents value) - throws IOException { - jsonWriter.value(value.toStringInternal()); - } - - @Override - public RepoRecordedInput.Dirents read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.Dirents) - RepoRecordedInput.Dirents.PARSER.parse(jsonReader.nextString()); - } - }; - - private static final TypeAdapter - REPO_RECORDED_INPUT_ENV_VAR_TYPE_ADAPTER = + private static final TypeAdapter + REPO_RECORDED_INPUT_WITH_VALUE_TYPE_ADAPTER = new TypeAdapter<>() { @Override - public void write(JsonWriter jsonWriter, RepoRecordedInput.EnvVar value) + public void write(JsonWriter jsonWriter, RepoRecordedInput.WithValue value) throws IOException { - jsonWriter.value(value.toStringInternal()); + jsonWriter.value(internalToUnicode(value.toString())); } @Override - public RepoRecordedInput.EnvVar read(JsonReader jsonReader) throws IOException { - return (RepoRecordedInput.EnvVar) - RepoRecordedInput.EnvVar.PARSER.parse(jsonReader.nextString()); + public RepoRecordedInput.WithValue read(JsonReader jsonReader) throws IOException { + return RepoRecordedInput.WithValue.parse(unicodeToInternal(jsonReader.nextString())) + .orElseGet(() -> new RepoRecordedInput.WithValue(PARSE_FAILURE, "")); } }; @@ -475,7 +397,7 @@ private static GsonBuilder newGsonBuilder() { .registerTypeAdapterFactory(IMMUTABLE_BIMAP) .registerTypeAdapterFactory(IMMUTABLE_SET) .registerTypeAdapterFactory(OPTIONAL) - .registerTypeAdapterFactory(IMMUTABLE_TABLE) + .registerTypeAdapter(String.class, STRING_ADAPTER) .registerTypeAdapter(Label.class, LABEL_TYPE_ADAPTER) .registerTypeAdapter(RepoRuleId.class, REPO_RULE_ID_TYPE_ADAPTER) .registerTypeAdapter(RepositoryName.class, REPOSITORY_NAME_TYPE_ADAPTER) @@ -487,11 +409,8 @@ private static GsonBuilder newGsonBuilder() { .registerTypeAdapter(ModuleExtensionId.IsolationKey.class, ISOLATION_KEY_TYPE_ADAPTER) .registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter()) .registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER) - .registerTypeAdapter(RepoRecordedInput.File.class, REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER) - .registerTypeAdapter( - RepoRecordedInput.Dirents.class, REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER) .registerTypeAdapter( - RepoRecordedInput.EnvVar.class, REPO_RECORDED_INPUT_ENV_VAR_TYPE_ADAPTER); + RepoRecordedInput.WithValue.class, REPO_RECORDED_INPUT_WITH_VALUE_TYPE_ADAPTER); } private GsonTypeAdapterUtil() {} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java index 913d3d24afe199..6599336192b29c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InnateRunnableExtension.java @@ -21,8 +21,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -39,7 +37,6 @@ import com.google.devtools.build.lib.skyframe.BzlLoadValue; import com.google.devtools.build.skyframe.SkyFunction.Environment; import java.util.Map.Entry; -import java.util.Optional; import javax.annotation.Nullable; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; @@ -150,11 +147,6 @@ public byte[] getBzlTransitiveDigest() { return loadedBzl.getTransitiveDigest(); } - @Override - public ImmutableMap> getStaticEnvVars() { - return ImmutableMap.of(); - } - @Override public RunModuleExtensionResult run( Environment env, @@ -237,11 +229,8 @@ public RunModuleExtensionResult run( attributesValue)); } return new RunModuleExtensionResult( - ImmutableSortedMap.of(), - ImmutableSortedMap.of(), - ImmutableSortedMap.of(), + ImmutableList.of(), generatedRepoSpecs.buildOrThrow(), - ModuleExtensionMetadata.REPRODUCIBLE, - ImmutableTable.of()); + ModuleExtensionMetadata.REPRODUCIBLE); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java index 8718920f4b14ab..6a0a622c7f4fc9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java @@ -15,10 +15,8 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; -import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; @@ -35,8 +33,7 @@ public abstract class LockFileModuleExtension { public static Builder builder() { return new AutoValue_LockFileModuleExtension.Builder() - .setModuleExtensionMetadata(Optional.empty()) - .setRecordedRepoMappingEntries(ImmutableTable.of()); + .setModuleExtensionMetadata(Optional.empty()); } @SuppressWarnings("mutable") @@ -45,19 +42,12 @@ public static Builder builder() { @SuppressWarnings("mutable") public abstract byte[] getUsagesDigest(); - public abstract ImmutableSortedMap getRecordedFileInputs(); - - public abstract ImmutableSortedMap getRecordedDirentsInputs(); - - public abstract ImmutableSortedMap> getEnvVariables(); + public abstract ImmutableList getRecordedInputs(); public abstract ImmutableMap getGeneratedRepoSpecs(); public abstract Optional getModuleExtensionMetadata(); - public abstract ImmutableTable - getRecordedRepoMappingEntries(); - public boolean isReproducible() { return getModuleExtensionMetadata() .map(LockfileModuleExtensionMetadata::getReproducible) @@ -72,23 +62,13 @@ public abstract static class Builder { public abstract Builder setUsagesDigest(byte[] digest); - public abstract Builder setRecordedFileInputs( - ImmutableSortedMap value); - - public abstract Builder setRecordedDirentsInputs( - ImmutableSortedMap value); - - public abstract Builder setEnvVariables( - ImmutableSortedMap> value); + public abstract Builder setRecordedInputs(ImmutableList value); public abstract Builder setGeneratedRepoSpecs(ImmutableMap value); public abstract Builder setModuleExtensionMetadata( Optional value); - public abstract Builder setRecordedRepoMappingEntries( - ImmutableTable value); - public abstract LockFileModuleExtension build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java index d2135a6ba4aa82..5c7b8de8a1d6c7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java @@ -15,15 +15,19 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableTable; import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager; import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkBaseExternalContext; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.skyframe.SkyFunction.Environment; import java.util.Map; +import java.util.Optional; import javax.annotation.Nullable; import net.starlark.java.annot.Param; import net.starlark.java.annot.ParamType; @@ -65,7 +69,9 @@ protected ModuleExtensionContext( ModuleExtensionId extensionId, StarlarkList modules, Facts facts, - boolean rootModuleHasNonDevDependency) { + boolean rootModuleHasNonDevDependency, + ImmutableMap> staticEnvVars, + ImmutableTable staticRepoMappingEntries) { super( workingDirectory, directories, @@ -83,6 +89,10 @@ protected ModuleExtensionContext( this.modules = modules; this.facts = facts; this.rootModuleHasNonDevDependency = rootModuleHasNonDevDependency; + // Record inputs to the extension that are known prior to evaluation. + RepoRecordedInput.EnvVar.wrap(staticEnvVars) + .forEach((input, value) -> recordInput(input, value.orElse(null))); + repoMappingRecorder.record(staticRepoMappingEntries); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java index ac6deab5150f4b..af6d7392b4a16b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java @@ -205,11 +205,6 @@ public ModuleExtensionEvalFactors getEvalFactors() { extension.archDependent() ? OS_ARCH.value() : ""); } - @Override - public ImmutableMap> getStaticEnvVars() { - return staticEnvVars; - } - @Override public byte[] getBzlTransitiveDigest() { return BazelModuleContext.of(bzlLoadValue.getModule()).bzlTransitiveDigest(); @@ -274,13 +269,10 @@ private RunModuleExtensionResult runInternal( directories, env.getListener()); ModuleExtensionMetadata moduleExtensionMetadata; - var repoMappingRecorder = new Label.RepoMappingRecorder(); - repoMappingRecorder.mergeEntries(bzlLoadValue.getRecordedRepoMappings()); try (Mutability mu = Mutability.create("module extension", usagesValue.getExtensionUniqueName()); ModuleExtensionContext moduleContext = - createContext( - env, usagesValue, starlarkSemantics, extensionId, repoMappingRecorder, facts)) { + createContext(env, usagesValue, starlarkSemantics, extensionId, facts, bzlLoadValue)) { StarlarkThread thread = StarlarkThread.create( mu, @@ -289,9 +281,7 @@ private RunModuleExtensionResult runInternal( SymbolGenerator.create(extensionId)); thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener())); threadContext.storeInThread(thread); - // This is used by the `Label()` constructor in Starlark, to record any attempts to resolve - // apparent repo names to canonical repo names. See #20721 for why this is necessary. - thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder); + moduleContext.storeRepoMappingRecorderInThread(thread); try (SilentCloseable c = Profiler.instance() .profile(ProfilerTask.BZLMOD, () -> "evaluate module extension: " + extensionId)) { @@ -317,12 +307,9 @@ private RunModuleExtensionResult runInternal( moduleContext.markSuccessful(); env.getListener().post(ModuleExtensionEvaluationProgress.finished(extensionId)); return new RunModuleExtensionResult( - moduleContext.getRecordedFileInputs(), - moduleContext.getRecordedDirentsInputs(), - moduleContext.getRecordedEnvVarInputs(), + moduleContext.getRecordedInputs(), threadContext.createRepos(starlarkSemantics), - moduleExtensionMetadata, - repoMappingRecorder.recordedEntries()); + moduleExtensionMetadata); } catch (EvalException e) { env.getListener().handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack())); throw ExternalDepsException.withMessage( @@ -340,9 +327,11 @@ private ModuleExtensionContext createContext( SingleExtensionUsagesValue usagesValue, StarlarkSemantics starlarkSemantics, ModuleExtensionId extensionId, - Label.RepoMappingRecorder repoMappingRecorder, - Facts facts) + Facts facts, + BzlLoadValue bzlLoadValue) throws ExternalDepsException { + var staticRepoMappingRecorder = new Label.SimpleRepoMappingRecorder(); + staticRepoMappingRecorder.record(bzlLoadValue.getRecordedRepoMappings()); Path workingDirectory = directories .getOutputBase() @@ -357,7 +346,7 @@ private ModuleExtensionContext createContext( extension, usagesValue.getRepoMappings().get(moduleKey), usagesValue.getExtensionUsages().get(moduleKey), - repoMappingRecorder)); + staticRepoMappingRecorder)); } ModuleExtensionUsage rootUsage = usagesValue.getExtensionUsages().get(ModuleKey.ROOT); boolean rootModuleHasNonDevDependency = @@ -376,6 +365,8 @@ private ModuleExtensionContext createContext( extensionId, StarlarkList.immutableCopyOf(modules), facts, - rootModuleHasNonDevDependency); + rootModuleHasNonDevDependency, + staticEnvVars, + staticRepoMappingRecorder.recordedEntries()); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java index 9065194928ef2c..b3a2f7a499e318 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RunnableExtension.java @@ -15,14 +15,11 @@ package com.google.devtools.build.lib.bazel.bzlmod; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; import com.google.devtools.build.lib.cmdline.RepositoryMapping; -import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.skyframe.SkyFunction.Environment; -import java.util.Optional; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkSemantics; @@ -33,17 +30,15 @@ * *

The general idiom is to "load" such a {@link RunnableExtension} object by getting as much * information about it as needed to determine whether it can be reused from the lockfile (hence - * methods such as {@link #getEvalFactors()}, {@link #getBzlTransitiveDigest()}, {@link - * #getStaticEnvVars()}). Then the {@link #run} method can be called if it's determined that we - * can't reuse the cached results in the lockfile and have to re-run this extension. + * methods such as {@link #getEvalFactors()} and {@link #getBzlTransitiveDigest()}). Then the {@link + * #run} method can be called if it's determined that we can't reuse the cached results in the + * lockfile and have to re-run this extension. */ interface RunnableExtension { ModuleExtensionEvalFactors getEvalFactors(); byte[] getBzlTransitiveDigest(); - ImmutableMap> getStaticEnvVars(); - /** Runs the extension. Returns null if a Skyframe restart is required. */ @Nullable RunModuleExtensionResult run( @@ -57,10 +52,7 @@ RunModuleExtensionResult run( /* Holds the result data from running a module extension */ record RunModuleExtensionResult( - ImmutableSortedMap recordedFileInputs, - ImmutableSortedMap recordedDirentsInputs, - ImmutableSortedMap> recordedEnvVarInputs, + ImmutableList recordedInputs, ImmutableMap generatedRepoSpecs, - ModuleExtensionMetadata moduleExtensionMetadata, - ImmutableTable recordedRepoMappingEntries) {} + ModuleExtensionMetadata moduleExtensionMetadata) {} } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index 052240aa2288cd..c83afc25132cf3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -16,15 +16,10 @@ package com.google.devtools.build.lib.bazel.bzlmod; import static com.google.common.collect.ImmutableBiMap.toImmutableBiMap; -import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.util.stream.Collectors.joining; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableTable; -import com.google.common.collect.Maps; -import com.google.common.collect.Table; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.bzlmod.RunnableExtension.RunModuleExtensionResult; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; @@ -45,8 +40,8 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; -import com.google.devtools.build.skyframe.SkyframeLookupResult; import java.util.Arrays; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.Function; @@ -220,9 +215,14 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (!lockfileMode.equals(LockfileMode.OFF)) { var nonVisibleRepoNames = - moduleExtensionResult.recordedRepoMappingEntries().values().stream() - .filter(repoName -> !repoName.isVisible()) - .map(RepositoryName::toString) + moduleExtensionResult.recordedInputs().stream() + .filter( + inputAndValue -> + inputAndValue.input() instanceof RepoRecordedInput.RecordedRepoMapping + && inputAndValue.value() == null) + .map(entry -> (RepoRecordedInput.RecordedRepoMapping) entry.input()) + .map(RepoRecordedInput.RecordedRepoMapping::apparentName) + .map(apparentName -> "@" + apparentName) .collect(joining(", ")); if (!nonVisibleRepoNames.isEmpty()) { env.getListener() @@ -252,7 +252,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) // rollback), which would cause false-positive validation errors. if (lockfileMode.equals(LockfileMode.ERROR) && !newFacts.equals(workspaceLockfileFacts)) { String reason = - "The extension '%s' has changed its facts: %s != %s" + "the extension '%s' has changed its facts: %s != %s" .formatted( extensionId, Starlark.repr(newFacts.value()), @@ -269,15 +269,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) // result is taken from the lockfile, we can already populate the lockfile info. This is // necessary to prevent the extension from rerunning when only the imports change. if (lockfileMode == LockfileMode.UPDATE || lockfileMode == LockfileMode.REFRESH) { - var envVariables = - ImmutableMap.>builder() - // The environment variable dependencies statically declared via the 'environ' - // attribute. - .putAll(RepoRecordedInput.EnvVar.wrap(extension.getStaticEnvVars())) - // The environment variable dependencies dynamically declared via the 'getenv' method. - .putAll(moduleExtensionResult.recordedEnvVarInputs()) - .buildKeepingLast(); - lockFileInfo = Optional.of( new LockFileModuleExtension.WithFactors( @@ -287,13 +278,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) .setUsagesDigest( SingleExtensionUsagesValue.hashForEvaluation( GsonTypeAdapterUtil.SINGLE_EXTENSION_USAGES_VALUE_GSON, usagesValue)) - .setRecordedFileInputs(moduleExtensionResult.recordedFileInputs()) - .setRecordedDirentsInputs(moduleExtensionResult.recordedDirentsInputs()) - .setEnvVariables(ImmutableSortedMap.copyOf(envVariables)) + .setRecordedInputs(moduleExtensionResult.recordedInputs()) .setGeneratedRepoSpecs(generatedRepoSpecs) .setModuleExtensionMetadata(lockfileModuleExtensionMetadata) - .setRecordedRepoMappingEntries( - moduleExtensionResult.recordedRepoMappingEntries()) .build())); } else { lockFileInfo = Optional.empty(); @@ -335,43 +322,23 @@ private SingleExtensionValue tryGettingValueFromLockFile( if (!Arrays.equals( extension.getBzlTransitiveDigest(), lockedExtension.getBzlTransitiveDigest())) { diffRecorder.record( - "The implementation of the extension '" + "the implementation of the extension '" + extensionId + "' or one of its transitive .bzl files has changed"); } - if (didRecordedInputsChange( - env, - directories, - // didRecordedInputsChange expects possibly null String values. - Maps.transformValues(lockedExtension.getEnvVariables(), v -> v.orElse(null)))) { - diffRecorder.record( - "The environment variables the extension '" - + extensionId - + "' depends on (or their values) have changed"); - } // Check extension data in lockfile is still valid, disregarding usage information that is not // relevant for the evaluation of the extension. if (!Arrays.equals( SingleExtensionUsagesValue.hashForEvaluation( GsonTypeAdapterUtil.SINGLE_EXTENSION_USAGES_VALUE_GSON, usagesValue), lockedExtension.getUsagesDigest())) { - diffRecorder.record("The usages of the extension '" + extensionId + "' have changed"); - } - if (didRepoMappingsChange(env, lockedExtension.getRecordedRepoMappingEntries())) { - diffRecorder.record( - "The repo mappings of certain repos used by the extension '" - + extensionId - + "' have changed"); + diffRecorder.record("the usages of the extension '" + extensionId + "' have changed"); } - if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedFileInputs())) { + Optional reason = + didRecordedInputsChange(env, directories, lockedExtension.getRecordedInputs()); + if (reason.isPresent()) { diffRecorder.record( - "One or more files the extension '" + extensionId + "' is using have changed"); - } - if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedDirentsInputs())) { - diffRecorder.record( - "One or more directory listings watched by the extension '" - + extensionId - + "' have changed"); + "an input to the extension '" + extensionId + "' changed: " + reason.get()); } } catch (DiffFoundEarlyExitException ignored) { // ignored @@ -423,63 +390,17 @@ public String getRecordedDiffMessages() { } } - private static boolean didRepoMappingsChange( - Environment env, ImmutableTable recordedRepoMappings) - throws InterruptedException, NeedsSkyframeRestartException { - // Request repo mappings for any 'source repos' in the recorded mapping entries. - // Note specially that the main repo needs to be treated differently: if any .bzl file from the - // main repo was used for module extension eval, it _has_ to be before WORKSPACE is evaluated - // (see relevant code in BzlLoadFunction#getRepositoryMapping), so we only request the main repo - // mapping _without_ WORKSPACE repos. See #20942 for more information. - SkyframeLookupResult result = - env.getValuesAndExceptions( - recordedRepoMappings.rowKeySet().stream() - .map( - repoName -> - repoName.isMain() - ? RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS - : RepositoryMappingValue.key(repoName)) - .collect(toImmutableSet())); - if (env.valuesMissing()) { - // This likely means that one of the 'source repos' in the recorded mapping entries is no - // longer there. - throw new NeedsSkyframeRestartException(); - } - for (Table.Cell cell : recordedRepoMappings.cellSet()) { - RepositoryMappingValue repoMappingValue = - (RepositoryMappingValue) - result.get( - cell.getRowKey().isMain() - ? RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS - : RepositoryMappingValue.key(cell.getRowKey())); - if (repoMappingValue == null) { - throw new NeedsSkyframeRestartException(); - } - // Very importantly, `repoMappingValue` here could be for a repo that's no longer existent in - // the dep graph. See - // bazel_lockfile_test.testExtensionRepoMappingChange_sourceRepoNoLongerExistent for a test - // case. - if (repoMappingValue.equals(RepositoryMappingValue.NOT_FOUND_VALUE) - || !cell.getValue() - .equals(repoMappingValue.repositoryMapping().get(cell.getColumnKey()))) { - // Wee woo wee woo -- diff detected! - return true; - } - } - return false; - } - - private static boolean didRecordedInputsChange( + private static Optional didRecordedInputsChange( Environment env, BlazeDirectories directories, - Map recordedInputs) + List recordedInputs) throws InterruptedException, NeedsSkyframeRestartException { Optional outdated = RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputs); if (env.valuesMissing()) { throw new NeedsSkyframeRestartException(); } - return outdated.isPresent(); + return outdated; } private SingleExtensionValue createSingleExtensionValue( @@ -533,7 +454,7 @@ private static SingleExtensionEvalFunctionException createOutdatedLockfileExcept return new SingleExtensionEvalFunctionException( ExternalDepsException.withMessage( Code.BAD_LOCKFILE, - "MODULE.bazel.lock is no longer up-to-date because: %s. Please run `bazel mod deps" + "MODULE.bazel.lock is no longer up-to-date because %s. Please run `bazel mod deps" + " --lockfile_mode=update` to update your lockfile.", reason)); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java index a53211fd4b8cec..89af2cde2a1f14 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java @@ -158,7 +158,7 @@ private Path ensureTrashDir() throws IOException { */ public Path moveToCache( Path fetchedRepoDir, Path fetchedRepoMarkerFile, String predeclaredInputHash) - throws IOException, InterruptedException { + throws IOException { Preconditions.checkState(path != null); Path entryDir = path.getRelative(predeclaredInputHash); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 17f3d25a6fbad7..5cf3e3069fcf0d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -14,15 +14,18 @@ package com.google.devtools.build.lib.bazel.repository.starlark; +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.base.Ascii; -import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.Futures; @@ -52,14 +55,12 @@ import com.google.devtools.build.lib.remote.RemoteExternalOverlayFileSystem; 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.RepoRecordedInput.Dirents; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.RepoCacheFriendlyPath; import com.google.devtools.build.lib.rules.repository.RepositoryFunction; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult; -import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; import com.google.devtools.build.lib.unsafe.StringUnsafe; import com.google.devtools.build.lib.util.OsUtils; import com.google.devtools.build.lib.util.io.OutErr; @@ -85,12 +86,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; -import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -167,9 +167,8 @@ private interface AsyncTask extends SilentCloseable { @Nullable private final ProcessWrapper processWrapper; protected final StarlarkSemantics starlarkSemantics; protected final String identifyingStringForLogging; - private final HashMap recordedFileInputs = new HashMap<>(); - private final HashMap recordedDirentsInputs = new HashMap<>(); - private final HashSet accumulatedEnvKeys = new HashSet<>(); + protected final Label.RepoMappingRecorder repoMappingRecorder; + private final LinkedHashMap recordedInputs = new LinkedHashMap<>(); private final RepositoryRemoteExecutor remoteExecutor; private final List asyncTasks; private final boolean allowWatchingPathsOutsideWorkspace; @@ -209,6 +208,13 @@ protected StarlarkBaseExternalContext( Thread.ofVirtual() .name("downloads[" + identifyingStringForLogging + "]-", 0) .factory()); + // This is used by the `Label()` constructor in Starlark, to record any attempts to resolve + // apparent repo names to canonical repo names. See #20721 for why this is necessary. + this.repoMappingRecorder = + (fromRepo, apparentRepoName, canonicalRepoName) -> + recordInput( + new RepoRecordedInput.RecordedRepoMapping(fromRepo, apparentRepoName), + canonicalRepoName.isVisible() ? canonicalRepoName.getName() : null); } /** @@ -241,6 +247,19 @@ public final void close() throws EvalException, IOException { } } + public void storeRepoMappingRecorderInThread(StarlarkThread thread) { + repoMappingRecorder.storeInThread(thread); + } + + protected void recordInput(RepoRecordedInput input, @Nullable String value) { + if (recordedInputs.containsKey(input) && !Objects.equals(recordedInputs.get(input), value)) { + throw new IllegalStateException( + "Conflicting values recorded for input %s: '%s' vs. '%s'" + .formatted(input, recordedInputs.get(input), value)); + } + recordedInputs.put(input, value); + } + private boolean cancelPendingAsyncTasks() { boolean hadPendingItems = false; for (AsyncTask task : asyncTasks) { @@ -270,21 +289,11 @@ private final void registerAsyncTask(AsyncTask task) { @ForOverride protected abstract boolean shouldDeleteWorkingDirectoryOnClose(boolean successful); - /** Returns the file digests used by this context object so far. */ - public ImmutableSortedMap getRecordedFileInputs() { - return ImmutableSortedMap.copyOf(recordedFileInputs); - } - - public ImmutableSortedMap getRecordedDirentsInputs() { - return ImmutableSortedMap.copyOf(recordedDirentsInputs); - } - - public ImmutableSortedMap> getRecordedEnvVarInputs() - throws InterruptedException { - // getEnvVarValues doesn't return null since the Skyframe dependencies have already been - // established by getenv calls. - return RepoRecordedInput.EnvVar.wrap( - RepositoryFunction.getEnvVarValues(env, accumulatedEnvKeys)); + /** Returns all recorded inputs in the order they were recorded. */ + public ImmutableList getRecordedInputs() { + return recordedInputs.entrySet().stream() + .map(e -> new RepoRecordedInput.WithValue(e.getKey(), e.getValue())) + .collect(toImmutableList()); } protected void checkInOutputDirectory(String operation, StarlarkPath path) @@ -1480,16 +1489,17 @@ private static T nullIfNone(Object object, Class type) { @Nullable public String getEnvironmentValue(String name, Object defaultValue) throws InterruptedException, NeedsSkyframeRestartException { - // Must look up via AEF, rather than solely copy from `this.repoEnvVariables`, in order to + // Must look up via Skyframe, rather than solely copy from `this.repoEnvVariables`, in order to // establish a SkyKey dependency relationship. - if (env.getValue(ActionEnvironmentFunction.key(name)) == null) { - throw new NeedsSkyframeRestartException(); + var nameAndValue = RepositoryFunction.getEnvVarValues(env, ImmutableSet.of(name)); + if (nameAndValue == null) { + return null; } - // However, to account for --repo_env we take the value from `this.repoEnvVariables`. // See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 . + var entry = Iterables.getOnlyElement(RepoRecordedInput.EnvVar.wrap(nameAndValue).entrySet()); String envVarValue = repoEnvVariables.get(name); - accumulatedEnvKeys.add(name); + recordInput(entry.getKey(), envVarValue); return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class); } @@ -1659,7 +1669,7 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) throw new NeedsSkyframeRestartException(); } - recordedFileInputs.put( + recordInput( recordedInput, RepoRecordedInput.File.fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue)); } catch (IOException e) { @@ -1678,8 +1688,7 @@ protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch) throw new NeedsSkyframeRestartException(); } try { - recordedDirentsInputs.put( - recordedInput, RepoRecordedInput.Dirents.getDirentsMarkerValue(path)); + recordInput(recordedInput, RepoRecordedInput.Dirents.getDirentsMarkerValue(path)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } @@ -1813,7 +1822,7 @@ private StarlarkExecutionResult executeRemote( boolean quiet, String workingDirectory) throws EvalException, InterruptedException { - Preconditions.checkState(canExecuteRemote()); + checkState(canExecuteRemote()); ImmutableSortedMap.Builder inputsBuilder = ImmutableSortedMap.naturalOrder(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 59b79202b21653..751d8305e94a58 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -19,7 +19,6 @@ import com.github.difflib.patch.PatchFailedException; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent; @@ -54,7 +53,6 @@ import java.io.IOException; import java.io.OutputStream; import java.nio.file.InvalidPathException; -import java.util.HashMap; import java.util.Map; import javax.annotation.Nullable; import net.starlark.java.annot.Param; @@ -85,7 +83,6 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext { private final StructImpl attrObject; private final IgnoredSubdirectories ignoredSubdirectories; private final SyscallCache syscallCache; - private final HashMap recordedDirTreeInputs = new HashMap<>(); /** * Create a new context (repository_ctx) object for a Starlark repository rule ({@code rule} @@ -142,10 +139,6 @@ protected boolean shouldDeleteWorkingDirectoryOnClose(boolean successful) { return !successful; } - public ImmutableSortedMap getRecordedDirTreeInputs() { - return ImmutableSortedMap.copyOf(recordedDirTreeInputs); - } - @StarlarkMethod( name = "name", structField = true, @@ -602,7 +595,7 @@ public void watchTree(Object path) throw new NeedsSkyframeRestartException(); } - recordedDirTreeInputs.put(recordedInput, digestValue.hexDigest()); + recordInput(recordedInput, digestValue.hexDigest()); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } 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 f1edc2b714d89e..c804ee6c9dc6c0 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 @@ -271,13 +271,13 @@ private FetchResult fetchInternal( "repository " + ((RepositoryName) key.argument()).getDisplayForm(mainRepoMapping), SymbolGenerator.create(key)); thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener())); - var repoMappingRecorder = new Label.RepoMappingRecorder(); - // For repos defined in Bzlmod, record any used repo mappings in the marker file. + var repoMappingRecorder = new Label.SimpleRepoMappingRecorder(); + // For repos defined in Bzlmod, record any repo mappings used by the rule's implementation in + // the marker file. The repo mappings recorded while loading the repo rule definition are + // instead folded into the predeclared input hash (see DigestWriter#computePredeclaredInputHash). // Repos defined in WORKSPACE are impossible to verify given the chunked loading (we'd have to // record which chunk the repo mapping was used in, and ain't nobody got time for that). if (!isWorkspaceRepo(rule)) { - repoMappingRecorder.mergeEntries( - rule.getRuleClassObject().getRuleDefinitionEnvironmentRepoMappingEntries()); thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder); } @@ -328,13 +328,10 @@ private FetchResult fetchInternal( } // Modify marker data to include the files/dirents/env vars used by the rule's implementation - // function. - recordedInputValues.putAll(starlarkRepositoryContext.getRecordedFileInputs()); - recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirentsInputs()); - recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirTreeInputs()); - recordedInputValues.putAll( - Maps.transformValues( - starlarkRepositoryContext.getRecordedEnvVarInputs(), v -> v.orElse(null))); + // function, in the order they were recorded. + for (RepoRecordedInput.WithValue wv : starlarkRepositoryContext.getRecordedInputs()) { + recordedInputValues.put(wv.input(), wv.value()); + } for (Table.Cell repoMappings : repoMappingRecorder.recordedEntries().cellSet()) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java index 8a89f9b6999cc0..78cf2c99720123 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java @@ -119,8 +119,8 @@ public StarlarkCallable repositoryRule( builder.setRuleDefinitionEnvironmentLabelAndDigest( moduleContext.label(), moduleContext.bzlTransitiveDigest()); } - Label.RepoMappingRecorder repoMappingRecorder = - thread.getThreadLocal(Label.RepoMappingRecorder.class); + Label.SimpleRepoMappingRecorder repoMappingRecorder = + (Label.SimpleRepoMappingRecorder) thread.getThreadLocal(Label.RepoMappingRecorder.class); if (repoMappingRecorder != null) { builder.setRuleDefinitionEnvironmentRepoMappingEntries(repoMappingRecorder.recordedEntries()); } diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index bdf5c89104cc36..d6922efa3f3a3b 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java @@ -232,7 +232,7 @@ public static Label parseWithPackageContext( Parts parts = Parts.parse(raw); Label parsed = parseWithPackageContextInternal(parts, packageContext); if (repoMappingRecorder != null && parts.repo() != null && !parts.repoIsCanonical()) { - repoMappingRecorder.entries.put( + repoMappingRecorder.record( packageContext.currentRepo(), parts.repo(), parsed.getRepository()); } return parsed; @@ -252,12 +252,31 @@ private static Label parseWithPackageContextInternal(Parts parts, PackageContext } /** Records repo mapping entries used by {@link #parseWithPackageContext}. */ - public static final class RepoMappingRecorder { - /** {@code } */ + public interface RepoMappingRecorder { + void record(RepositoryName fromRepo, String apparentRepoName, RepositoryName canonicalRepoName); + + default void record(Table entries) { + for (Table.Cell cell : entries.cellSet()) { + record(cell.getRowKey(), cell.getColumnKey(), cell.getValue()); + } + } + + default void storeInThread(StarlarkThread thread) { + thread.setThreadLocal(RepoMappingRecorder.class, this); + } + } + + /** + * A {@link RepoMappingRecorder} backed by a {@link Table} that is used for BUILD and .bzl load + * threads. + */ + public static final class SimpleRepoMappingRecorder implements RepoMappingRecorder { Table entries = HashBasedTable.create(); - public void mergeEntries(Table entries) { - this.entries.putAll(entries); + @Override + public void record( + RepositoryName fromRepo, String apparentRepoName, RepositoryName canonicalRepoName) { + entries.put(fromRepo, apparentRepoName, canonicalRepoName); } public ImmutableTable recordedEntries() { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java index 89c7b61f75b256..39fdaf35d58744 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java @@ -145,6 +145,11 @@ public void beforeCommand( } public void afterCommand() { + if (cache == null) { + // Not all commands cause beforeCommand to be called, but afterCommand is called + // unconditionally. + return; + } this.cache = null; this.inputPrefetcher = null; this.reporter = null; @@ -270,6 +275,9 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) var repoPath = externalDirectory.getChild(repo.getName()); var remoteRepo = externalFs.getPath(repoPath); var walkResult = walk(remoteRepo); + for (var directory : walkResult.directories()) { + nativeFs.getPath(directory.asFragment()).createDirectory(); + } var unused = getFromFuture( inputPrefetcher.prefetchFiles( @@ -280,11 +288,10 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) ActionInputPrefetcher.Priority.CRITICAL, ActionInputPrefetcher.Reason.INPUTS)); // Create symlinks last as some platforms don't allow creating a symlink to a non-existent - // target. + // target. A symlink may have already been created as an input to an action. for (var remoteSymlink : walkResult.symlinks()) { var nativeSymlink = nativeFs.getPath(remoteSymlink.asFragment()); - nativeSymlink.getParentDirectory().createDirectoryAndParents(); - nativeSymlink.createSymbolicLink(remoteSymlink.readSymbolicLink()); + FileSystemUtils.ensureSymbolicLink(nativeSymlink, remoteSymlink.readSymbolicLink()); } // After the repo has been copied, atomically materialize the marker file. This ensures that the @@ -297,10 +304,10 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter) markerFileSibling.renameTo(markerFile); } - private record WalkResult(List files, List symlinks) {} + private record WalkResult(List files, List symlinks, List directories) {} private static WalkResult walk(Path root) throws IOException { - var result = new WalkResult(new ArrayList<>(), new ArrayList<>()); + var result = new WalkResult(new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); walk(root, result); return result; } @@ -311,7 +318,10 @@ private static void walk(Path root, WalkResult result) throws IOException { switch (dirent.getType()) { case FILE -> result.files.add(fromChild); case SYMLINK -> result.symlinks.add(fromChild); - case DIRECTORY -> walk(fromChild, result); + case DIRECTORY -> { + result.directories.add(fromChild); + walk(fromChild, result); + } default -> throw new IOException("Unsupported file type: " + dirent); } } @@ -515,7 +525,8 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) @Override public Path resolveSymbolicLinks(PathFragment path) throws IOException { - return fsForPath(path).resolveSymbolicLinks(path); + // Ensure that the return value doesn't leave the overlay file system. + return getPath(fsForPath(path).resolveSymbolicLinks(path).asFragment()); } @Nullable 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 index bc6c39f4855a30..055b4e329ac32a 100644 --- 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 @@ -15,14 +15,15 @@ package com.google.devtools.build.lib.rules.repository; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap; -import static java.util.Comparator.naturalOrder; +import static java.util.Map.Entry.comparingByKey; import static java.util.Objects.requireNonNull; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.ImmutableMap; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -44,7 +45,6 @@ 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.Map; import java.util.Objects; @@ -67,7 +67,7 @@ * 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 sealed class RepoRecordedInput implements Comparable { +public abstract sealed class RepoRecordedInput { /** Represents a parser for a specific type of recorded inputs. */ public abstract static class Parser { /** @@ -84,14 +84,6 @@ public abstract static class Parser { public abstract RepoRecordedInput parse(String s); } - private static final Comparator COMPARATOR = - (o1, o2) -> - o1 == o2 - ? 0 - : Comparator.comparing((RepoRecordedInput rri) -> rri.getParser().getPrefix()) - .thenComparing(RepoRecordedInput::toStringInternal) - .compare(o1, o2); - /** * Parses a recorded input from its string representation. * @@ -115,27 +107,80 @@ public static RepoRecordedInput parse(String s) { return NeverUpToDateRepoRecordedInput.PARSE_FAILURE; } + /** A recorded input along with its recorded value. */ + public record WithValue(RepoRecordedInput input, @Nullable String value) { + /** Parses a {@link WithValue} from its string representation. */ + public static Optional parse(String s) { + int sChar = s.indexOf(' '); + if (sChar > 0) { + var input = RepoRecordedInput.parse(unescape(s.substring(0, sChar))); + if (!input.equals(NeverUpToDateRepoRecordedInput.PARSE_FAILURE)) { + return Optional.of(new WithValue(input, unescape(s.substring(sChar + 1)))); + } + } + return Optional.empty(); + } + + /** Converts this {@link WithValue} to a string in a format compatible with {@link #parse}. */ + @Override + public String toString() { + return escape(input.toString()) + " " + escape(value); + } + + @VisibleForTesting + static String escape(String str) { + return str == null + ? "\\0" + : str.replace("\\", "\\\\").replace("\n", "\\n").replace(" ", "\\s"); + } + + @VisibleForTesting + @Nullable + static String unescape(String str) { + if (str.equals("\\0")) { + return null; // \0 == null string + } + StringBuilder result = new StringBuilder(); + boolean escaped = false; + for (int i = 0; i < str.length(); i++) { + char c = str.charAt(i); + if (escaped) { + if (c == 'n') { // n means new line + result.append("\n"); + } else if (c == 's') { // s means space + result.append(" "); + } else { // Any other escaped characters are just un-escaped + result.append(c); + } + escaped = false; + } else if (c == '\\') { + escaped = true; + } else { + result.append(c); + } + } + return result.toString(); + } + } + /** * Returns whether all values are still up-to-date for each recorded input. If Skyframe values are * missing, the return value should be ignored; callers are responsible for checking {@code * env.valuesMissing()} and triggering a Skyframe restart if needed. */ public static Optional isAnyValueOutdated( - Environment env, - BlazeDirectories directories, - Map recordedInputValues) + Environment env, BlazeDirectories directories, List recordedInputValues) throws InterruptedException { env.getValuesAndExceptions( - recordedInputValues.keySet().stream() - .map(rri -> rri.getSkyKey(directories)) + recordedInputValues.stream() + .map(riv -> riv.input().getSkyKey(directories)) .collect(toImmutableSet())); if (env.valuesMissing()) { return UNDECIDED; } - for (Map.Entry recordedInputValue : - recordedInputValues.entrySet()) { + for (var recordedInput : recordedInputValues) { Optional reason = - recordedInputValue.getKey().isOutdated(env, directories, recordedInputValue.getValue()); + recordedInput.input().isOutdated(env, directories, recordedInput.value()); if (reason.isPresent()) { return reason; } @@ -154,11 +199,6 @@ 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}. @@ -546,12 +586,11 @@ public RepoRecordedInput parse(String s) { final String name; - public static ImmutableSortedMap> wrap( + public static ImmutableMap> wrap( Map> envVars) { return envVars.entrySet().stream() - .collect( - toImmutableSortedMap( - naturalOrder(), e -> new EnvVar(e.getKey()), Map.Entry::getValue)); + .sorted(comparingByKey()) + .collect(toImmutableMap(e -> new EnvVar(e.getKey()), Map.Entry::getValue)); } private EnvVar(String name) { @@ -600,7 +639,7 @@ public Optional isOutdated( // Note that `oldValue` can be null if the env var was not set. if (!Objects.equals(oldValue, v)) { return Optional.of( - "value of %s changed: %s -> %s" + "environment variable %s changed: %s -> %s" .formatted( name, oldValue == null ? "" : "'%s'".formatted(oldValue), @@ -639,6 +678,10 @@ public RecordedRepoMapping(RepositoryName sourceRepo, String apparentName) { this.apparentName = apparentName; } + public String apparentName() { + return apparentName; + } + @Override public boolean equals(Object o) { if (this == o) { 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 57e40d770200f8..958be029590d9a 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 @@ -18,11 +18,11 @@ import static com.google.devtools.build.lib.skyframe.RepositoryMappingFunction.REPOSITORY_OVERRIDES; import static java.nio.charset.StandardCharsets.ISO_8859_1; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Table; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue; @@ -64,7 +64,7 @@ import java.io.IOException; import java.util.Map; import java.util.Optional; -import java.util.TreeMap; +import java.util.LinkedHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -268,10 +268,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) try { if (remoteRepoContentsCache.lookupCache( repositoryName, repoRoot, digestWriter.predeclaredInputHash, env.getListener())) { - env.getListener() - .handle( - Event.debug( - "Got %s from the remote repo contents cache".formatted(repositoryName))); return new RepositoryDirectoryValue.Success( repoRoot, /* isFetchingDelayed= */ false, excludeRepoFromVendoring); } @@ -675,41 +671,6 @@ public static RepositoryDirectoryValue symlinkRepoRoot( source, /* isFetchingDelayed= */ false, /* excludeFromVendoring= */ true); } - // Escape a value for the marker file - @VisibleForTesting - static String escape(String str) { - return str == null ? "\\0" : str.replace("\\", "\\\\").replace("\n", "\\n").replace(" ", "\\s"); - } - - // Unescape a value from the marker file - @Nullable - @VisibleForTesting - static String unescape(String str) { - if (str.equals("\\0")) { - return null; // \0 == null string - } - StringBuilder result = new StringBuilder(); - boolean escaped = false; - for (int i = 0; i < str.length(); i++) { - char c = str.charAt(i); - if (escaped) { - if (c == 'n') { // n means new line - result.append("\n"); - } else if (c == 's') { // s means space - result.append(" "); - } else { // Any other escaped characters are just un-escaped - result.append(c); - } - escaped = false; - } else if (c == '\\') { - escaped = true; - } else { - result.append(c); - } - } - return result.toString(); - } - private static class DigestWriter { // Input value map to force repo invalidation upon an invalid marker file. private static final ImmutableMap PARSE_FAILURE = @@ -750,10 +711,10 @@ void writeMarkerFile(Map recordedInputValue StringBuilder builder = new StringBuilder(); builder.append(predeclaredInputHash).append("\n"); for (Map.Entry recordedInput : - new TreeMap(recordedInputValues).entrySet()) { - String key = recordedInput.getKey().toString(); - String value = recordedInput.getValue(); - builder.append(escape(key)).append(" ").append(escape(value)).append("\n"); + new LinkedHashMap(recordedInputValues).entrySet()) { + builder + .append(new RepoRecordedInput.WithValue(recordedInput.getKey(), recordedInput.getValue())) + .append("\n"); } String content = builder.toString(); try { @@ -831,15 +792,13 @@ private static Map readMarkerFile( ""); } firstLineVerified = true; - recordedInputValues = new TreeMap<>(); + recordedInputValues = new LinkedHashMap<>(); } else { - int sChar = line.indexOf(' '); - if (sChar > 0) { - RepoRecordedInput input = RepoRecordedInput.parse(unescape(line.substring(0, sChar))); - if (!input.equals(NeverUpToDateRepoRecordedInput.PARSE_FAILURE)) { - recordedInputValues.put(input, unescape(line.substring(sChar + 1))); - continue; - } + Optional withValue = + RepoRecordedInput.WithValue.parse(line); + if (withValue.isPresent()) { + recordedInputValues.put(withValue.get().input(), withValue.get().value()); + continue; } // On parse failure, just forget everything else and mark the whole input out of date. return PARSE_FAILURE; @@ -872,6 +831,17 @@ static String computePredeclaredInputHash( .addInt(environ.size()); environ.forEach( (key, value) -> fp.addString(key.toString()).addNullableString(value.orElse(null))); + var repoMappingEntries = + rule.getRuleClassObject().getRuleDefinitionEnvironmentRepoMappingEntries(); + var repoMappingCells = + repoMappingEntries == null ? ImmutableSet.>of() : repoMappingEntries.cellSet(); + fp.addInt(repoMappingCells.size()); + repoMappingCells.forEach( + entry -> { + fp.addString(entry.getRowKey().getName()); + fp.addString(entry.getColumnKey()); + fp.addString(entry.getValue().getName()); + }); return fp.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 13b997951b7444..4471325f4649ac 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 @@ -239,7 +239,12 @@ public Optional isAnyRecordedInputOutdated( Map recordedInputValues, Environment env) throws InterruptedException { - return RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputValues); + return RepoRecordedInput.isAnyValueOutdated( + env, + directories, + recordedInputValues.entrySet().stream() + .map(e -> new RepoRecordedInput.WithValue(e.getKey(), e.getValue())) + .collect(com.google.common.collect.ImmutableList.toImmutableList())); } public static RootedPath getRootedPathFromLabel(Label label, Environment env) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index 0570badd89e5e6..6d7ad863a6c183 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -806,7 +806,7 @@ private BzlLoadValue computeInternalWithCompiledBzl( if (mainRepoMapping == null) { return null; } - Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder(); + var repoMappingRecorder = new Label.SimpleRepoMappingRecorder(); ImmutableList> programLoads = getLoadsFromProgram(prog); ImmutableList