Skip to content

Commit

Permalink
Do not rerun module extensions when only imports or locations change
Browse files Browse the repository at this point in the history 
When using the lockfile, extension evaluation results are now persisted
even if the imports (`use_repo`s) fail validation against the actually
generated repos and reused if only imports or Starlark locations change.

This requires storing `ModuleExtensionMetadata` in the lockfile to
ensure that the correct fixup warnings are shown if imports are updated
but the extension isn't rerun.
  • Loading branch information
@fmeum
fmeum committed Aug 15, 2023
1 parent 5806691 commit d43b4bd
Show file tree
Hide file tree
Showing 7 changed files with 236 additions and 49 deletions.
24 changes: 23 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ java_library(
":common",
":inspection",
":module_extension",
":module_extension_metadata",
":registry",
":repo_rule_creator",
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
Expand Down Expand Up @@ -168,7 +169,6 @@ java_library(
"Discovery.java",
"GsonTypeAdapterUtil.java",
"ModuleExtensionContext.java",
"ModuleExtensionMetadata.java",
"ModuleFileFunction.java",
"ModuleFileGlobals.java",
"Selection.java",
Expand All @@ -182,6 +182,7 @@ java_library(
":common",
":exception",
":module_extension",
":module_extension_metadata",
":registry",
":resolution",
"//src/main/java/com/google/devtools/build/docgen/annot",
Expand Down Expand Up @@ -282,3 +283,24 @@ java_library(
"//third_party:jsr305",
],
)

java_library(
name = "module_extension_metadata",
srcs = [
"ModuleExtensionMetadata.java",
],
deps = [
":common",
":module_extension",
"//src/main/java/com/google/devtools/build/docgen/annot",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/net/starlark/java/annot",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
"//third_party:auto_value",
"//third_party:gson",
"//third_party:guava",
"//third_party:jsr305",
],
)
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public ImmutableList<String> getModuleExtensionDiff(
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 + "' has changed");
extDiff.add("The usages of the extension '" + extensionId + "' have changed");
}
if (!envVariables.equals(lockedExtension.getEnvVariables())) {
extDiff.add(
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
import java.util.Optional;

/**
* This object serves as a container for the transitive digest (obtained from transitive .bzl files)
Expand All @@ -33,7 +34,8 @@ public static Builder builder() {
return new AutoValue_LockFileModuleExtension.Builder()
// TODO(salmasamy) can be removed when updating lockfile version
.setEnvVariables(ImmutableMap.of())
.setAccumulatedFileDigests(ImmutableMap.of());
.setAccumulatedFileDigests(ImmutableMap.of())
.setModuleExtensionMetadata(Optional.empty());
}

@SuppressWarnings("mutable")
Expand All @@ -45,6 +47,8 @@ public static Builder builder() {

public abstract ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs();

public abstract Optional<ModuleExtensionMetadata> getModuleExtensionMetadata();

public abstract Builder toBuilder();

/** Builder type for {@link LockFileModuleExtension}. */
Expand All @@ -59,6 +63,8 @@ public abstract static class Builder {

public abstract Builder setGeneratedRepoSpecs(ImmutableMap<String, RepoSpec> value);

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

public abstract LockFileModuleExtension build();
}
}
55 changes: 31 additions & 24 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.stream.Collectors.joining;

import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
Expand All @@ -27,6 +28,7 @@
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedHashSet;
Expand All @@ -49,24 +51,29 @@
doc =
"Return values of this type from a module extension's implementation function to "
+ "provide metadata about the repositories generated by the extension to Bazel.")
public class ModuleExtensionMetadata implements StarlarkValue {
@Nullable private final ImmutableSet<String> explicitRootModuleDirectDeps;
@Nullable private final ImmutableSet<String> explicitRootModuleDirectDevDeps;
private final UseAllRepos useAllRepos;
@AutoValue
@GenerateTypeAdapter
public abstract class ModuleExtensionMetadata implements StarlarkValue {
@Nullable
abstract ImmutableSet<String> getExplicitRootModuleDirectDeps();

private ModuleExtensionMetadata(
@Nullable
abstract ImmutableSet<String> getExplicitRootModuleDirectDevDeps();

abstract UseAllRepos getUseAllRepos();

private static ModuleExtensionMetadata create(
@Nullable Set<String> explicitRootModuleDirectDeps,
@Nullable Set<String> explicitRootModuleDirectDevDeps,
UseAllRepos useAllRepos) {
this.explicitRootModuleDirectDeps =
return new AutoValue_ModuleExtensionMetadata(
explicitRootModuleDirectDeps != null
? ImmutableSet.copyOf(explicitRootModuleDirectDeps)
: null;
this.explicitRootModuleDirectDevDeps =
: null,
explicitRootModuleDirectDevDeps != null
? ImmutableSet.copyOf(explicitRootModuleDirectDevDeps)
: null;
this.useAllRepos = useAllRepos;
: null,
useAllRepos);
}

static ModuleExtensionMetadata create(
Expand All @@ -76,19 +83,19 @@ static ModuleExtensionMetadata create(
throws EvalException {
if (rootModuleDirectDepsUnchecked == Starlark.NONE
&& rootModuleDirectDevDepsUnchecked == Starlark.NONE) {
return new ModuleExtensionMetadata(null, null, UseAllRepos.NO);
return create(null, null, UseAllRepos.NO);
}

// When root_module_direct_deps = "all", accept both root_module_direct_dev_deps = None and
// root_module_direct_dev_deps = [], but not root_module_direct_dev_deps = ["some_repo"].
if (rootModuleDirectDepsUnchecked.equals("all")
&& rootModuleDirectDevDepsUnchecked.equals(StarlarkList.immutableOf())) {
return new ModuleExtensionMetadata(null, null, UseAllRepos.REGULAR);
return create(null, null, UseAllRepos.REGULAR);
}

if (rootModuleDirectDevDepsUnchecked.equals("all")
&& rootModuleDirectDepsUnchecked.equals(StarlarkList.immutableOf())) {
return new ModuleExtensionMetadata(null, null, UseAllRepos.DEV);
return create(null, null, UseAllRepos.DEV);
}

if (rootModuleDirectDepsUnchecked.equals("all")
Expand Down Expand Up @@ -146,8 +153,7 @@ static ModuleExtensionMetadata create(
}
}

return new ModuleExtensionMetadata(
explicitRootModuleDirectDeps, explicitRootModuleDirectDevDeps, UseAllRepos.NO);
return create(explicitRootModuleDirectDeps, explicitRootModuleDirectDevDeps, UseAllRepos.NO);
}

public void evaluate(
Expand Down Expand Up @@ -358,18 +364,18 @@ private static Optional<String> makeUseRepoCommand(

private Optional<ImmutableSet<String>> getRootModuleDirectDeps(Set<String> allRepos)
throws EvalException {
switch (useAllRepos) {
switch (getUseAllRepos()) {
case NO:
if (explicitRootModuleDirectDeps != null) {
Set<String> invalidRepos = Sets.difference(explicitRootModuleDirectDeps, allRepos);
if (getExplicitRootModuleDirectDeps() != null) {
Set<String> invalidRepos = Sets.difference(getExplicitRootModuleDirectDeps(), allRepos);
if (!invalidRepos.isEmpty()) {
throw Starlark.errorf(
"root_module_direct_deps contained the following repositories "
+ "not generated by the extension: %s",
String.join(", ", invalidRepos));
}
}
return Optional.ofNullable(explicitRootModuleDirectDeps);
return Optional.ofNullable(getExplicitRootModuleDirectDeps());
case REGULAR:
return Optional.of(ImmutableSet.copyOf(allRepos));
case DEV:
Expand All @@ -380,18 +386,19 @@ private Optional<ImmutableSet<String>> getRootModuleDirectDeps(Set<String> allRe

private Optional<ImmutableSet<String>> getRootModuleDirectDevDeps(Set<String> allRepos)
throws EvalException {
switch (useAllRepos) {
switch (getUseAllRepos()) {
case NO:
if (explicitRootModuleDirectDevDeps != null) {
Set<String> invalidRepos = Sets.difference(explicitRootModuleDirectDevDeps, allRepos);
if (getExplicitRootModuleDirectDevDeps() != null) {
Set<String> invalidRepos =
Sets.difference(getExplicitRootModuleDirectDevDeps(), allRepos);
if (!invalidRepos.isEmpty()) {
throw Starlark.errorf(
"root_module_direct_dev_deps contained the following "
+ "repositories not generated by the extension: %s",
String.join(", ", invalidRepos));
}
}
return Optional.ofNullable(explicitRootModuleDirectDevDeps);
return Optional.ofNullable(getExplicitRootModuleDirectDevDeps());
case REGULAR:
return Optional.of(ImmutableSet.of());
case DEV:
Expand All @@ -400,7 +407,7 @@ private Optional<ImmutableSet<String>> getRootModuleDirectDevDeps(Set<String> al
throw new IllegalStateException("not reached");
}

private enum UseAllRepos {
enum UseAllRepos {
NO,
REGULAR,
DEV,
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ public abstract class ModuleExtensionUsage {
*/
public abstract boolean getHasNonDevUseExtension();

public abstract Builder toBuilder();

public static Builder builder() {
return new AutoValue_ModuleExtensionUsage.Builder();
}
Expand Down
Loading

0 comments on commit d43b4bd

Please sign in to comment.