diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java index 50fbd82feb5a07..567a01bbea2c17 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java @@ -121,6 +121,11 @@ public void afterCommand() throws AbruptExitException { // Add the new resolved extensions for (var event : extensionResolutionEventsMap.values()) { + LockFileModuleExtension extension = event.getModuleExtension(); + if (!extension.shouldLockExtesnsion()) { + continue; + } + var oldExtensionEntries = updatedExtensionMap.get(event.getExtensionId()); ImmutableMap extensionEntries; if (oldExtensionEntries != null) { @@ -128,11 +133,11 @@ public void afterCommand() throws AbruptExitException { extensionEntries = new ImmutableMap.Builder() .putAll(oldExtensionEntries) - .put(event.getExtensionFactors(), event.getModuleExtension()) + .put(event.getExtensionFactors(), extension) .buildKeepingLast(); } else { // new extension - extensionEntries = ImmutableMap.of(event.getExtensionFactors(), event.getModuleExtension()); + extensionEntries = ImmutableMap.of(event.getExtensionFactors(), extension); } updatedExtensionMap.put(event.getExtensionId(), extensionEntries); } @@ -164,12 +169,13 @@ private boolean shouldKeepExtension( // If there is a new event for this extension, compare it with the existing ones ModuleExtensionResolutionEvent extEvent = extensionResolutionEventsMap.get(extensionId); if (extEvent != null) { + boolean doNotLockExtension = !extEvent.getModuleExtension().shouldLockExtesnsion(); boolean dependencyOnOsChanged = lockedExtensionKey.getOs().isEmpty() != extEvent.getExtensionFactors().getOs().isEmpty(); boolean dependencyOnArchChanged = lockedExtensionKey.getArch().isEmpty() != extEvent.getExtensionFactors().getArch().isEmpty(); - if (dependencyOnOsChanged || dependencyOnArchChanged) { + if (doNotLockExtension || dependencyOnOsChanged || dependencyOnArchChanged) { return false; } } 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 fea4715f7de5e8..88cdd5c2e7888d 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 @@ -54,6 +54,11 @@ public static Builder builder() { public abstract Builder toBuilder(); + public boolean shouldLockExtesnsion() { + return getModuleExtensionMetadata().isEmpty() + || !getModuleExtensionMetadata().get().getReproducible(); + } + /** Builder type for {@link LockFileModuleExtension}. */ @AutoValue.Builder public abstract static class Builder { 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 491427a6d3263a..cdddef1c3a1479 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 @@ -206,11 +206,24 @@ public boolean rootModuleHasNonDevDependency() { @ParamType(type = String.class), @ParamType(type = NoneType.class) }), + @Param( + name = "reproducible", + doc = + "States that this module extension ensures complete reproducibility, thereby it " + + "should not be stored in the lockfile.", + positional = false, + named = true, + defaultValue = "False", + allowedTypes = { + @ParamType(type = Boolean.class), + }), }) public ModuleExtensionMetadata extensionMetadata( - Object rootModuleDirectDepsUnchecked, Object rootModuleDirectDevDepsUnchecked) + Object rootModuleDirectDepsUnchecked, + Object rootModuleDirectDevDepsUnchecked, + boolean reproducible) throws EvalException { return ModuleExtensionMetadata.create( - rootModuleDirectDepsUnchecked, rootModuleDirectDevDepsUnchecked); + rootModuleDirectDepsUnchecked, rootModuleDirectDevDepsUnchecked, reproducible); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java index 79444ae4251d5e..539d7db8b26eaf 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java @@ -63,10 +63,13 @@ public abstract class ModuleExtensionMetadata implements StarlarkValue { abstract UseAllRepos getUseAllRepos(); + abstract boolean getReproducible(); + private static ModuleExtensionMetadata create( @Nullable Set explicitRootModuleDirectDeps, @Nullable Set explicitRootModuleDirectDevDeps, - UseAllRepos useAllRepos) { + UseAllRepos useAllRepos, + boolean reproducible) { return new AutoValue_ModuleExtensionMetadata( explicitRootModuleDirectDeps != null ? ImmutableSet.copyOf(explicitRootModuleDirectDeps) @@ -74,27 +77,30 @@ private static ModuleExtensionMetadata create( explicitRootModuleDirectDevDeps != null ? ImmutableSet.copyOf(explicitRootModuleDirectDevDeps) : null, - useAllRepos); + useAllRepos, + reproducible); } static ModuleExtensionMetadata create( - Object rootModuleDirectDepsUnchecked, Object rootModuleDirectDevDepsUnchecked) + Object rootModuleDirectDepsUnchecked, + Object rootModuleDirectDevDepsUnchecked, + boolean reproducible) throws EvalException { if (rootModuleDirectDepsUnchecked == Starlark.NONE && rootModuleDirectDevDepsUnchecked == Starlark.NONE) { - return create(null, null, UseAllRepos.NO); + return create(null, null, UseAllRepos.NO, reproducible); } // 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 create(null, null, UseAllRepos.REGULAR); + return create(null, null, UseAllRepos.REGULAR, reproducible); } if (rootModuleDirectDevDepsUnchecked.equals("all") && rootModuleDirectDepsUnchecked.equals(StarlarkList.immutableOf())) { - return create(null, null, UseAllRepos.DEV); + return create(null, null, UseAllRepos.DEV, reproducible); } if (rootModuleDirectDepsUnchecked.equals("all") @@ -152,7 +158,11 @@ static ModuleExtensionMetadata create( } } - return create(explicitRootModuleDirectDeps, explicitRootModuleDirectDevDeps, UseAllRepos.NO); + return create( + explicitRootModuleDirectDeps, + explicitRootModuleDirectDevDeps, + UseAllRepos.NO, + reproducible); } public void evaluate( 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 5a68996357ae9e..f709892010c453 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 @@ -151,20 +151,27 @@ public SkyValue compute(SkyKey skyKey, Environment env) } // Check the lockfile first for that module extension + LockFileModuleExtension lockedExtension = null; LockfileMode lockfileMode = BazelLockFileFunction.LOCKFILE_MODE.get(env); if (!lockfileMode.equals(LockfileMode.OFF)) { BazelLockFileValue lockfile = (BazelLockFileValue) env.getValue(BazelLockFileValue.KEY); if (lockfile == null) { return null; } - try { - SingleExtensionEvalValue singleExtensionEvalValue = - tryGettingValueFromLockFile(env, extensionId, extension, usagesValue, lockfile); - if (singleExtensionEvalValue != null) { - return singleExtensionEvalValue; + var lockedExtensionMap = lockfile.getModuleExtensions().get(extensionId); + lockedExtension = + lockedExtensionMap == null ? null : lockedExtensionMap.get(extension.getEvalFactors()); + if (lockedExtension != null) { + try { + SingleExtensionEvalValue singleExtensionEvalValue = + tryGettingValueFromLockFile( + env, extensionId, extension, usagesValue, lockfile, lockedExtension); + if (singleExtensionEvalValue != null) { + return singleExtensionEvalValue; + } + } catch (NeedsSkyframeRestartException e) { + return null; } - } catch (NeedsSkyframeRestartException e) { - return null; } } @@ -181,6 +188,24 @@ public SkyValue compute(SkyKey skyKey, Environment env) Optional moduleExtensionMetadata = moduleExtensionResult.getModuleExtensionMetadata(); + if (lockfileMode.equals(LockfileMode.ERROR)) { + boolean extensionShouldHaveBeenLocked = + moduleExtensionMetadata.map(metadata -> !metadata.getReproducible()).orElse(true); + // If this extension was not found in the lockfile, and after evaluation we found that it is + // not reproducible, then error indicating that it was expected to be in the lockfile. + if (lockedExtension == null && extensionShouldHaveBeenLocked) { + throw new SingleExtensionEvalFunctionException( + ExternalDepsException.withMessage( + Code.BAD_MODULE, + "The module extension '%s'%s does not exist in the lockfile", + extensionId, + extension.getEvalFactors().isEmpty() + ? "" + : " for platform " + extension.getEvalFactors()), + Transience.PERSISTENT); + } + } + // At this point the extension has been evaluated successfully, but SingleExtensionEvalFunction // may still fail if imported repositories were not generated. However, since imports do not // influence the evaluation of the extension and the validation also runs when the extension @@ -219,30 +244,13 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile( ModuleExtensionId extensionId, RunnableExtension extension, SingleExtensionUsagesValue usagesValue, - BazelLockFileValue lockfile) + BazelLockFileValue lockfile, + LockFileModuleExtension lockedExtension) throws SingleExtensionEvalFunctionException, InterruptedException, NeedsSkyframeRestartException { LockfileMode lockfileMode = BazelLockFileFunction.LOCKFILE_MODE.get(env); - var lockedExtensionMap = lockfile.getModuleExtensions().get(extensionId); - LockFileModuleExtension lockedExtension = - lockedExtensionMap == null ? null : lockedExtensionMap.get(extension.getEvalFactors()); - if (lockedExtension == null) { - if (lockfileMode.equals(LockfileMode.ERROR)) { - throw new SingleExtensionEvalFunctionException( - ExternalDepsException.withMessage( - Code.BAD_MODULE, - "The module extension '%s'%s does not exist in the lockfile", - extensionId, - extension.getEvalFactors().isEmpty() - ? "" - : " for platform " + extension.getEvalFactors()), - Transience.PERSISTENT); - } - return null; - } - ImmutableMap lockedExtensionUsages; try { // TODO(salmasamy) might be nicer to precompute this table when we construct diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index 4549e8837073c3..f8669910295f35 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -35,8 +35,6 @@ def setUp(self): 'aaa', '1.1' ).createCcModule('bbb', '1.0', {'aaa': '1.0'}).createCcModule( 'bbb', '1.1', {'aaa': '1.1'} - ).createCcModule( - 'ccc', '1.1', {'aaa': '1.1', 'bbb': '1.1'} ) self.ScratchFile( '.bazelrc', @@ -117,13 +115,7 @@ def testChangeModuleInRegistryWithLockfile(self): ], ) self.ScratchFile('BUILD', ['filegroup(name = "hello")']) - self.RunBazel( - [ - 'build', - '--nobuild', - '//:all', - ], - ) + self.RunBazel(['build', '--nobuild', '//:all']) # Change registry -> update 'sss' module file (corrupt it) module_dir = self.main_registry.root.joinpath('modules', 'sss', '1.3') @@ -146,9 +138,7 @@ def testChangeFlagWithLockfile(self): ], ) self.ScratchFile('BUILD', ['filegroup(name = "hello")']) - self.RunBazel( - ['build', '--nobuild', '//:all'], - ) + self.RunBazel(['build', '--nobuild', '//:all']) # Change registry -> update 'sss' module file (corrupt it) module_dir = self.main_registry.root.joinpath('modules', 'sss', '1.3') @@ -1865,6 +1855,131 @@ def testExtensionRepoMappingChange_mainRepoEvalCycleWithWorkspace(self): _, _, stderr = self.RunBazel(['build', '--enable_workspace', ':lol']) self.assertNotIn('ran the extension!', '\n'.join(stderr)) + def testReproducibleExtensionsIgnoredInLockfile(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'ext1 = use_extension("extension.bzl", "ext1")', + 'ext2 = use_extension("extension.bzl", "ext2")', + 'use_repo(ext1, "repo1")', + 'use_repo(ext2, "repo2")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _should_lock_impl(ctx): repo_rule(name="repo1")', + 'def _no_lock_impl(ctx):', + ' repo_rule(name="repo2")', + ' return ctx.extension_metadata(', + ' root_module_direct_deps=[],', + ' root_module_direct_dev_deps=[],', + ' reproducible=True', + ' )', + 'ext1 = module_extension(implementation=_should_lock_impl)', + 'ext2 = module_extension(implementation=_no_lock_impl)', + ], + ) + + self.RunBazel(['build', '@repo1//:all', '@repo2//:all']) + with open(self.Path('MODULE.bazel.lock'), 'r') as f: + lockfile = json.loads(f.read().strip()) + self.assertIn('//:extension.bzl%ext1', lockfile['moduleExtensions']) + self.assertNotIn('//:extension.bzl%ext2', lockfile['moduleExtensions']) + + # Update extensions implementations to the opposite + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _should_lock_impl(ctx): repo_rule(name="repo2")', + 'def _no_lock_impl(ctx):', + ' repo_rule(name="repo1")', + ' return ctx.extension_metadata(', + ' root_module_direct_deps=[],', + ' root_module_direct_dev_deps=[],', + ' reproducible=True', + ' )', + 'ext1 = module_extension(implementation=_no_lock_impl)', + 'ext2 = module_extension(implementation=_should_lock_impl)', + ], + ) + + # Assert updates in the lockfile + self.RunBazel(['build', '@repo1//:all', '@repo2//:all']) + with open(self.Path('MODULE.bazel.lock'), 'r') as f: + lockfile = json.loads(f.read().strip()) + self.assertNotIn('//:extension.bzl%ext1', lockfile['moduleExtensions']) + self.assertIn('//:extension.bzl%ext2', lockfile['moduleExtensions']) + + def testReproducibleExtensionInLockfileErrorMode(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'ext = use_extension("extension.bzl", "ext")', + 'use_repo(ext, "repo")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext_impl(ctx):', + ' repo_rule(name="repo")', + ' return ctx.extension_metadata(', + ' root_module_direct_deps=[],', + ' root_module_direct_dev_deps=[],', + ' reproducible=True', + ' )', + 'ext = module_extension(implementation=_ext_impl)', + ], + ) + + self.RunBazel(['build', '@repo//:all']) + with open(self.Path('MODULE.bazel.lock'), 'r') as f: + lockfile = json.loads(f.read().strip()) + self.assertNotIn('//:extension.bzl%ext', lockfile['moduleExtensions']) + + # Assert ext does NOT fail in error mode + self.RunBazel(['build', '@repo//:all', '--lockfile_mode=error']) + + # Update extension to not be reproducible + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext_impl(ctx): repo_rule(name="repo")', + 'ext = module_extension(implementation=_ext_impl)', + ], + ) + + # Assert ext does FAIL in error mode + _, _, stderr = self.RunBazel( + ['build', '@repo//:all', '--lockfile_mode=error'], allow_failure=True + ) + self.assertIn( + 'ERROR: The module extension ' + "'ModuleExtensionId{bzlFileLabel=//:extension.bzl, " + "extensionName=ext, isolationKey=Optional.empty}' does " + 'not exist in the lockfile', + stderr, + ) + if __name__ == '__main__': absltest.main()