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 Aug 16, 2023
1 parent d43b4bd commit 1614884
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 44 deletions.
57 changes: 28 additions & 29 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
lockfileMode,
lockfile);
if (singleExtensionEvalValue != null) {
ImmutableMap<String, RepoSpec> generatedRepoSpecs =
singleExtensionEvalValue.getGeneratedRepoSpecs();
Optional<ModuleExtensionMetadata> moduleExtensionMetadata =
lockfile.getModuleExtensions().get(extensionId).getModuleExtensionMetadata();
validateExtensionResult(
generatedRepoSpecs, moduleExtensionMetadata, extensionId, usagesValue, env);
return singleExtensionEvalValue;
}
}
Expand Down Expand Up @@ -220,9 +214,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
.setModuleExtensionMetadata(moduleExtensionMetadata)
.build()));
}
validateExtensionResult(
return validateAndCreateSingleExtensionEvalValue(
generatedRepoSpecs, moduleExtensionMetadata, extensionId, usagesValue, env);
return createSingleExtensionValue(generatedRepoSpecs, usagesValue);
}

@Nullable
Expand Down Expand Up @@ -274,7 +267,12 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile(
&& Arrays.equals(bzlTransitiveDigest, lockedExtension.getBzlTransitiveDigest())
&& trimmedUsages.equals(trimmedLockedUsages)
&& envVariables.equals(lockedExtension.getEnvVariables())) {
return createSingleExtensionValue(lockedExtension.getGeneratedRepoSpecs(), usagesValue);
return validateAndCreateSingleExtensionEvalValue(
lockedExtension.getGeneratedRepoSpecs(),
lockedExtension.getModuleExtensionMetadata(),
extensionId,
usagesValue,
env);
} else if (lockfileMode.equals(LockfileMode.ERROR)) {
ImmutableList<String> extDiff =
lockfile.getModuleExtensionDiff(
Expand Down Expand Up @@ -331,27 +329,15 @@ private Boolean didFilesChange(
return false;
}

private SingleExtensionEvalValue createSingleExtensionValue(
ImmutableMap<String, RepoSpec> generatedRepoSpecs, SingleExtensionUsagesValue usagesValue) {
return SingleExtensionEvalValue.create(
generatedRepoSpecs,
generatedRepoSpecs.keySet().stream()
.collect(
toImmutableBiMap(
e ->
RepositoryName.createUnvalidated(
usagesValue.getExtensionUniqueName() + "~" + e),
Function.identity())));
}

/**
* Validates the result of the module extension evaluation against the declared imports, throwing
* an exception if validation fails.
* an exception if validation fails, and returns a SingleExtensionEvalValue otherwise.
*
* <p>Since extension evaluation does not depend on the declared imports, the result of the
* evaluation can be reused and persisted in the lockfile even if validation fails.
* evaluation of the extension implementation function can be reused and persisted in the lockfile
* even if validation fails.
*/
private void validateExtensionResult(
private SingleExtensionEvalValue validateAndCreateSingleExtensionEvalValue(
ImmutableMap<String, RepoSpec> generatedRepoSpecs,
Optional<ModuleExtensionMetadata> moduleExtensionMetadata,
ModuleExtensionId extensionId,
Expand All @@ -362,6 +348,8 @@ private void validateExtensionResult(
// emitted in case of an error.
if (moduleExtensionMetadata.isPresent()) {
try {
// TODO: ModuleExtensionMetadata#evaluate should throw ExternalDepsException instead of
// EvalException.
moduleExtensionMetadata
.get()
.evaluate(
Expand Down Expand Up @@ -399,11 +387,21 @@ private void validateExtensionResult(
}
}
}

return SingleExtensionEvalValue.create(
generatedRepoSpecs,
generatedRepoSpecs.keySet().stream()
.collect(
toImmutableBiMap(
e ->
RepositoryName.createUnvalidated(
usagesValue.getExtensionUniqueName() + "~" + e),
Function.identity())));
}

/**
* @return usages with all information removed that does not influence the evaluation of the
* extension, only the validation against imports that comes afterward.
* Returns usages with all information removed that does not influence the evaluation of the
* extension.
*/
private static ImmutableMap<ModuleKey, ModuleExtensionUsage> trimUsagesForEvaluation(
Map<ModuleKey, ModuleExtensionUsage> usages) {
Expand All @@ -416,8 +414,9 @@ private static ImmutableMap<ModuleKey, ModuleExtensionUsage> trimUsagesForEvalua
// the parts that do, this preserves correctness in case new fields are added to
// ModuleExtensionUsage without updating this code.
usage.toBuilder()
// Locations are only used for error reporting and thus didn't influence the
// successful evaluation of the extension.
// Locations are only used for error reporting and thus don't influence whether
// the evaluation of the extension is successful and what its result is
// in case of success.
.setLocation(Location.BUILTIN)
// Extension implementation functions do not see the imports, they are only
// validated against the set of generated repos in a validation step that comes
Expand Down
33 changes: 18 additions & 15 deletions src/test/py/bazel/bzlmod/bazel_lockfile_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -876,20 +876,22 @@ def testExtensionEvaluationDoesNotRerunOnChangedImports(self):
# The build fails due to the "invalid_dep" import, which is not generated by the extension.
# Warnings should still be shown.
_, _, stderr = self.RunBazel(['build', '@dep//:all'], allow_failure = True)
self.assertIn('I am being evaluated', '\n'.join(stderr))
self.assertIn('Imported, but not created by the extension (will cause the build to fail):\ninvalid_dep', '\n'.join(stderr))
self.assertIn('Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\nmissing_dep', '\n'.join(stderr))
self.assertIn('Imported, but reported as indirect dependencies by the extension:\nindirect_dep', '\n'.join(stderr))
self.assertIn('ERROR: module extension "lockfile_ext" from "//:extension.bzl" does not generate repository "invalid_dep"', '\n'.join(stderr))
stderr = '\n'.join(stderr)
self.assertIn('I am being evaluated', stderr)
self.assertIn('Imported, but not created by the extension (will cause the build to fail):\ninvalid_dep', stderr)
self.assertIn('Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\nmissing_dep', stderr)
self.assertIn('Imported, but reported as indirect dependencies by the extension:\nindirect_dep', stderr)
self.assertIn('ERROR: module extension "lockfile_ext" from "//:extension.bzl" does not generate repository "invalid_dep"', stderr)

# Shut down bazel to empty cache and run with no changes to verify that the warnings are still shown.
self.RunBazel(['shutdown'])
_, _, stderr = self.RunBazel(['build', '@dep//:all'], allow_failure = True)
self.assertNotIn('I am being evaluated', '\n'.join(stderr))
self.assertIn('Imported, but not created by the extension (will cause the build to fail):\ninvalid_dep', '\n'.join(stderr))
self.assertIn('Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\nmissing_dep', '\n'.join(stderr))
self.assertIn('Imported, but reported as indirect dependencies by the extension:\nindirect_dep', '\n'.join(stderr))
self.assertIn('ERROR: module extension "lockfile_ext" from "//:extension.bzl" does not generate repository "invalid_dep"', '\n'.join(stderr))
stderr = '\n'.join(stderr)
self.assertNotIn('I am being evaluated', stderr)
self.assertIn('Imported, but not created by the extension (will cause the build to fail):\ninvalid_dep', stderr)
self.assertIn('Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\nmissing_dep', stderr)
self.assertIn('Imported, but reported as indirect dependencies by the extension:\nindirect_dep', stderr)
self.assertIn('ERROR: module extension "lockfile_ext" from "//:extension.bzl" does not generate repository "invalid_dep"', stderr)

# Fix the imports, which should not trigger a rerun of the extension even though imports and locations changed.
self.ScratchFile(
Expand All @@ -901,11 +903,12 @@ def testExtensionEvaluationDoesNotRerunOnChangedImports(self):
],
)
_, _, stderr = self.RunBazel(['build', '@dep//:all'])
self.assertNotIn('I am being evaluated', '\n'.join(stderr))
self.assertNotIn('Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\nmissing_dep', '\n'.join(stderr))
self.assertNotIn('Imported, but reported as indirect dependencies by the extension:\nindirect_dep', '\n'.join(stderr))
self.assertNotIn('Imported, but reported as indirect dependencies by the extension:\nindirect_dep', '\n'.join(stderr))
self.assertNotIn('ERROR: module extension "lockfile_ext" from "//:extension.bzl" does not generate repository "invalid_dep"', '\n'.join(stderr))
stderr = '\n'.join(stderr)
self.assertNotIn('I am being evaluated', stderr)
self.assertNotIn('Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\nmissing_dep', stderr)
self.assertNotIn('Imported, but reported as indirect dependencies by the extension:\nindirect_dep', stderr)
self.assertNotIn('Imported, but reported as indirect dependencies by the extension:\nindirect_dep', stderr)
self.assertNotIn('ERROR: module extension "lockfile_ext" from "//:extension.bzl" does not generate repository "invalid_dep"', stderr)


if __name__ == '__main__':
Expand Down

0 comments on commit 1614884

Please sign in to comment.