Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@fmeum
fmeum committed Apr 23, 2024
1 parent d4ecb99 commit ace82a1
Show file tree
Hide file tree
Showing 11 changed files with 59 additions and 49 deletions.
14 changes: 7 additions & 7 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelFetchAllFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,16 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
// 2. Run every extension found in the modules & collect its generated repos
ImmutableSet<ModuleExtensionId> extensionIds =
depGraphValue.getExtensionUsagesTable().rowKeySet();
ImmutableSet<SkyKey> singleEvalKeys =
ImmutableSet<SkyKey> singleExtensionKeys =
extensionIds.stream().map(SingleExtensionValue::key).collect(toImmutableSet());
SkyframeLookupResult singleEvalValues = env.getValuesAndExceptions(singleEvalKeys);
for (SkyKey singleEvalKey : singleEvalKeys) {
SingleExtensionValue singleEvalValue =
(SingleExtensionValue) singleEvalValues.get(singleEvalKey);
if (singleEvalValue == null) {
SkyframeLookupResult singleExtensionValues = env.getValuesAndExceptions(singleExtensionKeys);
for (SkyKey singleExtensionKey : singleExtensionKeys) {
SingleExtensionValue singleExtensionValue =
(SingleExtensionValue) singleExtensionValues.get(singleExtensionKey);
if (singleExtensionValue == null) {
return null;
}
reposToFetch.addAll(singleEvalValue.getCanonicalRepoNameToInternalNames().keySet());
reposToFetch.addAll(singleExtensionValue.getCanonicalRepoNameToInternalNames().keySet());
}

// 3. If this is fetch configure, get repo rules and only collect repos marked as configure
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@ public void afterCommand() throws AbruptExitException {
// validation later fails due to invalid imports.
var newExtensionInfos =
evaluator.getDoneValues().entrySet().stream()
.filter(entry -> entry.getKey() instanceof SingleExtensionValue.UnvalidatedKey)
.filter(entry -> entry.getKey() instanceof SingleExtensionValue.EvalKey)
.collect(
toImmutableMap(
entry -> ((SingleExtensionValue.UnvalidatedKey) entry.getKey()).argument(),
entry -> ((SingleExtensionValue.EvalKey) entry.getKey()).argument(),
entry ->
((SingleExtensionValue) entry.getValue()).getLockFileInfo().get()));

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
.stream()
// Use the unvalidated key to avoid errors caused by incorrect imports - we can fix
// them.
.map(SingleExtensionValue::unvalidatedKey)
.map(SingleExtensionValue::evalKey)
.collect(toImmutableSet());
SkyframeLookupResult result = env.getValuesAndExceptions(extensionsUsedByRootModule);
if (env.valuesMissing()) {
Expand Down
14 changes: 7 additions & 7 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,20 +179,20 @@ private ImmutableSetMultimap<ModuleExtensionId, String> computeExtensionToRepoIn
BazelDepGraphValue depGraphValue, Environment env) throws InterruptedException {
ImmutableSet<ModuleExtensionId> extensionEvalKeys =
depGraphValue.getExtensionUsagesTable().rowKeySet();
ImmutableList<SingleExtensionValue.Key> singleEvalKeys =
ImmutableList<SingleExtensionValue.Key> singleExtensionKeys =
extensionEvalKeys.stream().map(SingleExtensionValue::key).collect(toImmutableList());
SkyframeLookupResult singleEvalValues = env.getValuesAndExceptions(singleEvalKeys);
SkyframeLookupResult singleExtensionValues = env.getValuesAndExceptions(singleExtensionKeys);

ImmutableSetMultimap.Builder<ModuleExtensionId, String> extensionToRepoInternalNames =
ImmutableSetMultimap.builder();
for (SingleExtensionValue.Key singleEvalKey : singleEvalKeys) {
SingleExtensionValue singleEvalValue =
(SingleExtensionValue) singleEvalValues.get(singleEvalKey);
if (singleEvalValue == null) {
for (SingleExtensionValue.Key singleExtensionKey : singleExtensionKeys) {
SingleExtensionValue singleExtensionValue =
(SingleExtensionValue) singleExtensionValues.get(singleExtensionKey);
if (singleExtensionValue == null) {
return null;
}
extensionToRepoInternalNames.putAll(
singleEvalKey.argument(), singleEvalValue.getGeneratedRepoSpecs().keySet());
singleExtensionKey.argument(), singleExtensionValue.getGeneratedRepoSpecs().keySet());
}
return extensionToRepoInternalNames.build();
}
Expand Down
15 changes: 11 additions & 4 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
import com.google.devtools.build.skyframe.SkyFunction;
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.SkyValue;
Expand All @@ -34,16 +35,15 @@ public class SingleExtensionFunction implements SkyFunction {
@Nullable
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
throws InterruptedException,
SingleExtensionEvalFunction.SingleExtensionEvalFunctionException {
throws InterruptedException, SingleExtensionFunctionException {
ModuleExtensionId extensionId = (ModuleExtensionId) skyKey.argument();
SingleExtensionUsagesValue usagesValue =
(SingleExtensionUsagesValue) env.getValue(SingleExtensionUsagesValue.key(extensionId));
if (usagesValue == null) {
return null;
}
SingleExtensionValue unvalidatedValue =
(SingleExtensionValue) env.getValue(SingleExtensionValue.unvalidatedKey(extensionId));
(SingleExtensionValue) env.getValue(SingleExtensionValue.evalKey(extensionId));
if (unvalidatedValue == null) {
return null;
}
Expand All @@ -52,7 +52,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
for (ModuleExtensionUsage usage : usagesValue.getExtensionUsages().values()) {
for (Entry<String, String> repoImport : usage.getImports().entrySet()) {
if (!unvalidatedValue.getGeneratedRepoSpecs().containsKey(repoImport.getValue())) {
throw new SingleExtensionEvalFunction.SingleExtensionEvalFunctionException(
throw new SingleExtensionFunctionException(
ExternalDepsException.withMessage(
Code.INVALID_EXTENSION_IMPORT,
"module extension \"%s\" from \"%s\" does not generate repository \"%s\", yet it"
Expand All @@ -71,4 +71,11 @@ public SkyValue compute(SkyKey skyKey, Environment env)

return unvalidatedValue;
}

static final class SingleExtensionFunctionException extends SkyFunctionException {

SingleExtensionFunctionException(ExternalDepsException cause, Transience transience) {
super(cause, transience);
}
}
}
26 changes: 13 additions & 13 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,13 @@ public static Key key(ModuleExtensionId id) {
* repositories generated by the extension. This is only meant for special applications such as
* {@code bazel mod tidy}.
*/
static UnvalidatedKey unvalidatedKey(ModuleExtensionId id) {
return UnvalidatedKey.create(id);
static EvalKey evalKey(ModuleExtensionId id) {
return EvalKey.create(id);
}

/**
* The {@link com.google.devtools.build.skyframe.SkyKey} of a {@link
* SingleExtensionValue}
* The {@link SkyKey} of a {@link SingleExtensionValue} containing the result of extension
* evaluation.
*/
@AutoCodec
public static class Key extends AbstractSkyKey<ModuleExtensionId> {
Expand Down Expand Up @@ -116,24 +116,24 @@ public SkyKeyInterner<Key> getSkyKeyInterner() {
}

/**
* The {@link com.google.devtools.build.skyframe.SkyKey} of an unvalidated {@link
* SingleExtensionValue}
* The {@link SkyKey} of an {@link SingleExtensionValue} containing the <b>unvalidated</b> result
* of extension evaluation.
*/
@AutoCodec
static class UnvalidatedKey extends AbstractSkyKey<ModuleExtensionId> {
private static final SkyKeyInterner<UnvalidatedKey> interner = SkyKey.newInterner();
static class EvalKey extends AbstractSkyKey<ModuleExtensionId> {
private static final SkyKeyInterner<EvalKey> interner = SkyKey.newInterner();

protected UnvalidatedKey(ModuleExtensionId arg) {
protected EvalKey(ModuleExtensionId arg) {
super(arg);
}

private static UnvalidatedKey create(ModuleExtensionId arg) {
return interner.intern(new UnvalidatedKey(arg));
private static EvalKey create(ModuleExtensionId arg) {
return interner.intern(new EvalKey(arg));
}

@VisibleForSerialization
@AutoCodec.Interner
static UnvalidatedKey intern(UnvalidatedKey key) {
static EvalKey intern(EvalKey key) {
return interner.intern(key);
}

Expand All @@ -143,7 +143,7 @@ public SkyFunctionName functionName() {
}

@Override
public SkyKeyInterner<UnvalidatedKey> getSkyKeyInterner() {
public SkyKeyInterner<EvalKey> getSkyKeyInterner() {
return interner;
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,17 +133,17 @@ public SkyValue compute(SkyKey skyKey, Environment env)
return BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE;
}

SingleExtensionValue extensionEval =
SingleExtensionValue extensionValue =
(SingleExtensionValue) env.getValue(SingleExtensionValue.key(extensionId.get()));
if (extensionEval == null) {
if (extensionValue == null) {
return null;
}

String internalRepo = extensionEval.getCanonicalRepoNameToInternalNames().get(repositoryName);
String internalRepo = extensionValue.getCanonicalRepoNameToInternalNames().get(repositoryName);
if (internalRepo == null) {
return BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE;
}
RepoSpec extRepoSpec = extensionEval.getGeneratedRepoSpecs().get(internalRepo);
RepoSpec extRepoSpec = extensionValue.getGeneratedRepoSpecs().get(internalRepo);
return createRuleFromSpec(extRepoSpec, repositoryName, starlarkSemantics, env);
}

Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package com.google.devtools.build.lib.skyframe;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.devtools.build.lib.bazel.bzlmod.Version;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ protected MemoizingEvaluator createEvaluator(
progressReceiver,
inconsistencyReceiver,
trackIncrementalState
? (shouldStoreTransitivePackagesInLoadingAndAnalysis()
? (externalRepositoriesEnabled()
? DEFAULT_EVENT_FILTER_WITH_ACTIONS_AND_EXTERNAL_REPOS
: DEFAULT_EVENT_FILTER_WITH_ACTIONS)
: EventFilter.NO_STORAGE,
Expand Down
6 changes: 5 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1161,7 +1161,11 @@ public Root getForcedSingleSourceRootIfNoExecrootSymlinkCreation() {
return null;
}

protected boolean shouldStoreTransitivePackagesInLoadingAndAnalysis() {
final boolean externalRepositoriesEnabled() {
return shouldStoreTransitivePackagesInLoadingAndAnalysis();
}

private boolean shouldStoreTransitivePackagesInLoadingAndAnalysis() {
// Transitive packages may be needed for either RepoMappingManifestAction or Skymeld with
// external repository support. They are never needed if external repositories are disabled. To
// avoid complexity from toggling this, just choose a setting for the lifetime of the server.
Expand Down
16 changes: 8 additions & 8 deletions src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1913,7 +1913,7 @@ public void extensionMetadata() throws Exception {
evaluator
.getDoneValues()
.get(
SingleExtensionValue.unvalidatedKey(
SingleExtensionValue.evalKey(
ModuleExtensionId.create(
Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty())));
assertThat(evalValue.getFixup()).isPresent();
Expand Down Expand Up @@ -2005,7 +2005,7 @@ public void extensionMetadata_all() throws Exception {
evaluator
.getDoneValues()
.get(
SingleExtensionValue.unvalidatedKey(
SingleExtensionValue.evalKey(
ModuleExtensionId.create(
Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty())));
assertThat(evalValue.getFixup()).isPresent();
Expand Down Expand Up @@ -2099,7 +2099,7 @@ public void extensionMetadata_allDev() throws Exception {
evaluator
.getDoneValues()
.get(
SingleExtensionValue.unvalidatedKey(
SingleExtensionValue.evalKey(
ModuleExtensionId.create(
Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty())));
assertThat(evalValue.getFixup()).isPresent();
Expand Down Expand Up @@ -2162,7 +2162,7 @@ public void extensionMetadata_noRootUsage() throws Exception {
evaluator
.getDoneValues()
.get(
SingleExtensionValue.unvalidatedKey(
SingleExtensionValue.evalKey(
ModuleExtensionId.create(
Label.parseCanonical("@@ext~//:defs.bzl"), "ext", Optional.empty())));
assertThat(evalValue.getFixup()).isEmpty();
Expand Down Expand Up @@ -2248,7 +2248,7 @@ public void extensionMetadata_isolated() throws Exception {
evaluator
.getDoneValues()
.get(
SingleExtensionValue.unvalidatedKey(
SingleExtensionValue.evalKey(
ModuleExtensionId.create(
Label.parseCanonical("@@ext~//:defs.bzl"),
"ext",
Expand All @@ -2265,7 +2265,7 @@ public void extensionMetadata_isolated() throws Exception {
evaluator
.getDoneValues()
.get(
SingleExtensionValue.unvalidatedKey(
SingleExtensionValue.evalKey(
ModuleExtensionId.create(
Label.parseCanonical("@@ext~//:defs.bzl"),
"ext",
Expand Down Expand Up @@ -2358,7 +2358,7 @@ public void extensionMetadata_isolatedDev() throws Exception {
evaluator
.getDoneValues()
.get(
SingleExtensionValue.unvalidatedKey(
SingleExtensionValue.evalKey(
ModuleExtensionId.create(
Label.parseCanonical("@@ext~//:defs.bzl"),
"ext",
Expand All @@ -2375,7 +2375,7 @@ public void extensionMetadata_isolatedDev() throws Exception {
evaluator
.getDoneValues()
.get(
SingleExtensionValue.unvalidatedKey(
SingleExtensionValue.evalKey(
ModuleExtensionId.create(
Label.parseCanonical("@@ext~//:defs.bzl"),
"ext",
Expand Down

0 comments on commit ace82a1

Please sign in to comment.