Skip to content

Commit

Permalink
[7.0.1] Let module extensions track calls to Label() (#20750)
Browse files Browse the repository at this point in the history
... that use repo mapping. This is a rather obscure case of the lockfile
being stale; if the `Label()` constructor is called in an extension impl
function, and that call uses repo mapping of any form (i.e. the argument
looks like `@foo//bar`), then we need to be ready to rerun the extension
if `@foo` were to suddenly map to something else.

I also did a minor refactoring in `SingleExtensionEvalFunction` around
the logic to decide whether the locked extension is up-to-date. Right
now we perform a "diff" between the locked extension and what we expect
to be up-to-date, and if a "diff" is found *and*
`--lockfile_mode=error`, we basically perform a diff again. We also
don't short circuit; that is, if the transitive bzl digest has changed,
there's no point in seeing whether any files have changed, but we do
right now.

A follow-up will be sent to fix the analogous bug for repo rules.

Fixes #20721.

Closes #20742.

PiperOrigin-RevId: 595818144
Change-Id: Id660b7a659a7f2e4dde19c16784c2ab18a9ceb69

Co-authored-by: Xdng Yng <wyverald@gmail.com>
  • Loading branch information
iancha1992 and Wyverald authored Jan 5, 2024
1 parent 99dcdb0 commit b5fa492
Show file tree
Hide file tree
Showing 9 changed files with 378 additions and 74 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1277,7 +1277,10 @@ public Label label(Object input, StarlarkThread thread) throws EvalException {
// environment across .bzl files. Hence, we opt for stack inspection.
BazelModuleContext moduleContext = BazelModuleContext.ofInnermostBzlOrFail(thread, "Label()");
try {
return Label.parseWithPackageContext((String) input, moduleContext.packageContext());
return Label.parseWithPackageContext(
(String) input,
moduleContext.packageContext(),
thread.getThreadLocal(Label.RepoMappingRecorder.class));
} catch (LabelSyntaxException e) {
throw Starlark.errorf("invalid label in Label(): %s", e.getMessage());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:os",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
import java.util.Arrays;
import java.util.Map;

/**
Expand Down Expand Up @@ -118,36 +117,4 @@ public ImmutableList<String> getModuleAndFlagsDiff(
}
return moduleDiff.build();
}

/** Returns the differences between an extension and its locked data */
public ImmutableList<String> getModuleExtensionDiff(
ModuleExtensionId extensionId,
LockFileModuleExtension lockedExtension,
byte[] transitiveDigest,
boolean filesChanged,
ImmutableMap<String, String> envVariables,
ImmutableList<Map.Entry<ModuleKey, ModuleExtensionUsage>> extensionUsages,
ImmutableList<Map.Entry<ModuleKey, ModuleExtensionUsage>> lockedExtensionUsages) {

ImmutableList.Builder<String> extDiff = new ImmutableList.Builder<>();
if (!Arrays.equals(transitiveDigest, lockedExtension.getBzlTransitiveDigest())) {
extDiff.add(
"The implementation of the extension '"
+ extensionId
+ "' or one of its transitive .bzl files has changed");
}
if (filesChanged) {
extDiff.add("One or more files the extension '" + extensionId + "' is using have changed");
}
if (!extensionUsages.equals(lockedExtensionUsages)) {
extDiff.add("The usages of the extension '" + extensionId + "' have changed");
}
if (!envVariables.equals(lockedExtension.getEnvVariables())) {
extDiff.add(
"The environment variables the extension '"
+ extensionId
+ "' depends on (or their values) have changed");
}
return extDiff.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,12 @@
import com.google.auto.value.AutoValue;
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.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.vfs.Path;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
Expand Down Expand Up @@ -92,7 +95,7 @@ public ModuleKey read(JsonReader jsonReader) throws IOException {
};

public static final TypeAdapter<Label> LABEL_TYPE_ADAPTER =
new TypeAdapter<Label>() {
new TypeAdapter<>() {
@Override
public void write(JsonWriter jsonWriter, Label label) throws IOException {
jsonWriter.value(label.getUnambiguousCanonicalForm());
Expand All @@ -104,6 +107,19 @@ public Label read(JsonReader jsonReader) throws IOException {
}
};

public static final TypeAdapter<RepositoryName> REPOSITORY_NAME_TYPE_ADAPTER =
new TypeAdapter<>() {
@Override
public void write(JsonWriter jsonWriter, RepositoryName repoName) throws IOException {
jsonWriter.value(repoName.getName());
}

@Override
public RepositoryName read(JsonReader jsonReader) throws IOException {
return RepositoryName.createUnvalidated(jsonReader.nextString());
}
};

public static final TypeAdapter<ModuleExtensionId> MODULE_EXTENSION_ID_TYPE_ADAPTER =
new TypeAdapter<>() {
@Override
Expand Down Expand Up @@ -283,6 +299,70 @@ public Optional<T> 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 <T> TypeAdapter<T> create(Gson gson, TypeToken<T> 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<Object>) gson.getAdapter(TypeToken.get(typeArgs[0]));
var colTypeAdapter = (TypeAdapter<Object>) gson.getAdapter(TypeToken.get(typeArgs[1]));
var valTypeAdapter = (TypeAdapter<Object>) gson.getAdapter(TypeToken.get(typeArgs[2]));
if (rowTypeAdapter == null || colTypeAdapter == null || valTypeAdapter == null) {
return null;
}
return (TypeAdapter<T>)
new TypeAdapter<ImmutableTable<Object, Object, Object>>() {
@Override
public void write(JsonWriter jsonWriter, ImmutableTable<Object, Object, Object> t)
throws IOException {
jsonWriter.beginArray();
for (Table.Cell<Object, Object, Object> 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<Object, Object, Object> 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();
}
};
}
};

/**
* A variant of {@link Location} that converts the absolute path to the root module file to a
* constant and back.
Expand Down Expand Up @@ -371,8 +451,10 @@ public static Gson createLockFileGson(Path moduleFilePath) {
.registerTypeAdapterFactory(IMMUTABLE_BIMAP)
.registerTypeAdapterFactory(IMMUTABLE_SET)
.registerTypeAdapterFactory(OPTIONAL)
.registerTypeAdapterFactory(IMMUTABLE_TABLE)
.registerTypeAdapterFactory(new LocationTypeAdapterFactory(moduleFilePath))
.registerTypeAdapter(Label.class, LABEL_TYPE_ADAPTER)
.registerTypeAdapter(RepositoryName.class, REPOSITORY_NAME_TYPE_ADAPTER)
.registerTypeAdapter(Version.class, VERSION_TYPE_ADAPTER)
.registerTypeAdapter(ModuleKey.class, MODULE_KEY_TYPE_ADAPTER)
.registerTypeAdapter(ModuleExtensionId.class, MODULE_EXTENSION_ID_TYPE_ADAPTER)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableTable;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
import java.util.Optional;
Expand All @@ -32,7 +34,8 @@ public abstract class LockFileModuleExtension implements Postable {

public static Builder builder() {
return new AutoValue_LockFileModuleExtension.Builder()
.setModuleExtensionMetadata(Optional.empty());
.setModuleExtensionMetadata(Optional.empty())
.setRecordedRepoMappingEntries(ImmutableTable.of());
}

@SuppressWarnings("mutable")
Expand All @@ -46,6 +49,9 @@ public static Builder builder() {

public abstract Optional<ModuleExtensionMetadata> getModuleExtensionMetadata();

public abstract ImmutableTable<RepositoryName, String, RepositoryName>
getRecordedRepoMappingEntries();

public abstract Builder toBuilder();

/** Builder type for {@link LockFileModuleExtension}. */
Expand All @@ -62,6 +68,9 @@ public abstract static class Builder {

public abstract Builder setModuleExtensionMetadata(Optional<ModuleExtensionMetadata> value);

public abstract Builder setRecordedRepoMappingEntries(
ImmutableTable<RepositoryName, String, RepositoryName> value);

public abstract LockFileModuleExtension build();
}
}
Loading

0 comments on commit b5fa492

Please sign in to comment.