diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/BUILD index 68ec09587e81e9..6d8abb60844550 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/BUILD @@ -41,6 +41,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", "//src/main/java/com/google/devtools/build/lib/bazel/repository/starlark", "//src/main/java/com/google/devtools/build/lib/bazel/rules/android", + "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/pkgcache", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index 58ce1c66eef484..5a998b6e6446b3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -80,6 +80,7 @@ import com.google.devtools.build.lib.bazel.rules.android.AndroidNdkRepositoryRule; import com.google.devtools.build.lib.bazel.rules.android.AndroidSdkRepositoryFunction; import com.google.devtools.build.lib.bazel.rules.android.AndroidSdkRepositoryRule; +import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.pkgcache.PackageOptions; @@ -121,6 +122,7 @@ import com.google.devtools.common.options.OptionsParsingResult; import java.io.IOException; import java.lang.reflect.InvocationTargetException; +import java.time.Instant; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -159,6 +161,8 @@ public class BazelRepositoryModule extends BlazeModule { private CheckDirectDepsMode checkDirectDepsMode = CheckDirectDepsMode.WARNING; private BazelCompatibilityMode bazelCompatibilityMode = BazelCompatibilityMode.ERROR; private LockfileMode bazelLockfileMode = LockfileMode.UPDATE; + private Clock clock; + private Instant lastRegistryInvalidation = Instant.EPOCH; private Optional vendorDirectory; private List allowedYankedVersions = ImmutableList.of(); @@ -526,6 +530,21 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { starlarkRepositoryFunction.setRepositoryRemoteExecutor(remoteExecutor); singleExtensionEvalFunction.setRepositoryRemoteExecutor(remoteExecutor); delegatingDownloader.setDelegate(env.getRuntime().getDownloaderSupplier().get()); + + clock = env.getClock(); + try { + var lastRegistryInvalidationValue = + (PrecomputedValue) + env.getSkyframeExecutor() + .getEvaluator() + .getExistingValue(RegistryFunction.LAST_INVALIDATION.getKeyForTesting()); + if (lastRegistryInvalidationValue != null) { + lastRegistryInvalidation = (Instant) lastRegistryInvalidationValue.get(); + } + } catch (InterruptedException e) { + // Not thrown in Bazel. + throw new IllegalStateException(e); + } } } @@ -556,6 +575,10 @@ private String getAbsolutePath(String path, CommandEnvironment env) { @Override public ImmutableList getPrecomputedValues() { + Instant now = clock.now(); + if (now.isAfter(lastRegistryInvalidation.plus(RegistryFunction.INVALIDATION_INTERVAL))) { + lastRegistryInvalidation = now; + } return ImmutableList.of( PrecomputedValue.injected(RepositoryDelegatorFunction.REPOSITORY_OVERRIDES, overrides), PrecomputedValue.injected(ModuleFileFunction.MODULE_OVERRIDES, moduleOverrides), @@ -582,7 +605,8 @@ public ImmutableList getPrecomputedValues() { PrecomputedValue.injected( YankedVersionsUtil.ALLOWED_YANKED_VERSIONS, allowedYankedVersions), PrecomputedValue.injected( - RepositoryDelegatorFunction.DISABLE_NATIVE_REPO_RULES, disableNativeRepoRules)); + RepositoryDelegatorFunction.DISABLE_NATIVE_REPO_RULES, disableNativeRepoRules), + PrecomputedValue.injected(RegistryFunction.LAST_INVALIDATION, lastRegistryInvalidation)); } @Override 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 39573a5383f05a..9f6abaadb38df2 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 @@ -71,7 +71,7 @@ public void afterCommand() { // Check the Skyframe value instead of the option since some commands (e.g. shutdown) don't // propagate the options to Skyframe, but we can only operate on Skyframe values that were // generated in UPDATE mode. - if (lockfileMode != LockfileMode.UPDATE) { + if (lockfileMode != LockfileMode.UPDATE && lockfileMode != LockfileMode.REFRESH) { return; } moduleResolutionValue = diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java index 9de56c594550a1..7c01d12651fcca 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/IndexRegistry.java @@ -58,8 +58,26 @@ public class IndexRegistry implements Registry { * registry. */ public enum KnownFileHashesMode { + /** + * Neither use nor update any file hashes. All registry downloads will go out to the network. + */ IGNORE, + /** + * Use file hashes from the lockfile if available and add hashes for new files to the lockfile. + * Avoid revalidation of mutable registry information (yanked versions in metadata.json and + * modules that previously 404'd) by using these hashes and recording absent files in the + * lockfile. + */ USE_AND_UPDATE, + /** + * Use file hashes from the lockfile if available and add hashes for new files to the lockfile. + * Always revalidate mutable registry information. + */ + USE_IMMUTABLE_AND_UPDATE, + /** + * Require file hashes for all registry downloads. In particular, mutable registry files such as + * metadata.json can't be downloaded in this mode. + */ ENFORCE } @@ -115,7 +133,9 @@ private Optional grabFile( String url, ExtendedEventHandler eventHandler, boolean useChecksum) throws IOException, InterruptedException { var maybeContent = doGrabFile(url, eventHandler, useChecksum); - if (knownFileHashesMode == KnownFileHashesMode.USE_AND_UPDATE && useChecksum) { + if ((knownFileHashesMode == KnownFileHashesMode.USE_AND_UPDATE + || knownFileHashesMode == KnownFileHashesMode.USE_IMMUTABLE_AND_UPDATE) + && useChecksum) { eventHandler.post(RegistryFileDownloadEvent.create(url, maybeContent)); } return maybeContent; @@ -139,8 +159,14 @@ private Optional doGrabFile( // This is a new file, download without providing a checksum. checksum = Optional.empty(); } else if (knownChecksum.isEmpty()) { - // The file is known to not exist, so don't attempt to download it. - return Optional.empty(); + // The file didn't exist when the lockfile was created, but it may exist now. + if (knownFileHashesMode == KnownFileHashesMode.USE_IMMUTABLE_AND_UPDATE) { + // Attempt to download the file again. + checksum = Optional.empty(); + } else { + // Guarantee reproducibility by assuming that the file still doesn't exist. + return Optional.empty(); + } } else { // The file is known, download with a checksum to potentially obtain a repository cache hit // and ensure that the remote file hasn't changed. @@ -441,6 +467,10 @@ public Optional> getYankedVersions( @Override public Optional tryGetYankedVersionsFromLockfile( ModuleKey selectedModuleKey) { + if (knownFileHashesMode == KnownFileHashesMode.USE_IMMUTABLE_AND_UPDATE) { + // Yanked version information is inherently mutable, so always refresh it when requested. + return Optional.empty(); + } String yankedInfo = previouslySelectedYankedVersions.get(selectedModuleKey); if (yankedInfo != null) { // The module version was selected when the lockfile was created, but known to be yanked diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Registry.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Registry.java index 77420ee02c74c9..0fc178244a09ff 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Registry.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Registry.java @@ -17,12 +17,12 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.events.ExtendedEventHandler; -import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.NotComparableSkyValue; import java.io.IOException; import java.util.Optional; /** A database where module metadata is stored. */ -public interface Registry extends SkyValue { +public interface Registry extends NotComparableSkyValue { /** The URL that uniquely identifies the registry. */ String getUrl(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFactoryImpl.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFactoryImpl.java index f202b650db17ec..32c34daa0c4d4a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFactoryImpl.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFactoryImpl.java @@ -60,9 +60,11 @@ public Registry createRegistry( var knownFileHashesMode = switch (uri.getScheme()) { case "http", "https" -> - lockfileMode == LockfileMode.ERROR - ? KnownFileHashesMode.ENFORCE - : KnownFileHashesMode.USE_AND_UPDATE; + switch (lockfileMode) { + case ERROR -> KnownFileHashesMode.ENFORCE; + case REFRESH -> KnownFileHashesMode.USE_IMMUTABLE_AND_UPDATE; + case OFF, UPDATE -> KnownFileHashesMode.USE_AND_UPDATE; + }; case "file" -> KnownFileHashesMode.IGNORE; default -> throw new URISyntaxException(uri.toString(), "Unrecognized registry URL protocol"); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFunction.java index 121027c658e2aa..4e72d565bca8fe 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryFunction.java @@ -17,16 +17,32 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; import com.google.devtools.build.lib.server.FailureDetails; +import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.net.URISyntaxException; +import java.time.Duration; +import java.time.Instant; import javax.annotation.Nullable; /** A simple SkyFunction that creates a {@link Registry} with a given URL. */ public class RegistryFunction implements SkyFunction { + /** + * Set to the current time in {@link com.google.devtools.build.lib.bazel.BazelRepositoryModule} + * after {@link #INVALIDATION_INTERVAL} has passed. This is used to refresh the mutable registry + * contents cached in memory from time to time. + */ + public static final Precomputed LAST_INVALIDATION = + new Precomputed<>("last_registry_invalidation"); + + /** + * The interval after which the mutable registry contents cached in memory should be refreshed. + */ + public static final Duration INVALIDATION_INTERVAL = Duration.ofHours(1); + private final RegistryFactory registryFactory; private final Path workspaceRoot; @@ -41,6 +57,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException, RegistryException { LockfileMode lockfileMode = BazelLockFileFunction.LOCKFILE_MODE.get(env); + if (lockfileMode == LockfileMode.REFRESH) { + RegistryFunction.LAST_INVALIDATION.get(env); + } + BazelLockFileValue lockfile = (BazelLockFileValue) env.getValue(BazelLockFileValue.KEY); if (lockfile == null) { return null; 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 ccd9ec0a1b3b9a..61bb7b7b081024 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 @@ -215,7 +215,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) // influence the evaluation of the extension and the validation also runs when the extension // result is taken from the lockfile, we can already populate the lockfile info. This is // necessary to prevent the extension from rerunning when only the imports change. - if (lockfileMode.equals(LockfileMode.UPDATE)) { + if (lockfileMode == LockfileMode.UPDATE || lockfileMode == LockfileMode.REFRESH) { lockFileInfo = Optional.of( new LockFileModuleExtension.WithFactors( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java index 63a527d89887ee..fdd08a97b192ca 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java @@ -320,9 +320,11 @@ public Converter() { effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, help = "Specifies how and whether or not to use the lockfile. Valid values are `update` to" - + " use the lockfile and update it if there are changes, `error` to use the lockfile" - + " but throw an error if it's not up-to-date, or `off` to neither read from or write" - + " to the lockfile.") + + " use the lockfile and update it if there are changes, `refresh` to additionally" + + " refresh mutable information (yanked versions and previously missing modules)" + + " from remote registries from time to time, `error` to use the lockfile but throw" + + " an error if it's not up-to-date, or `off` to neither read from or write to the" + + " lockfile.") public LockfileMode lockfileMode; @Option( @@ -369,10 +371,11 @@ public Converter() { /** An enum for specifying how to use the lockfile. */ public enum LockfileMode { OFF, // Don't use the lockfile at all. - UPDATE, // Update the lockfile when it mismatches the module. - ERROR; // Throw an error when it mismatches the module. + UPDATE, // Update the lockfile wh + REFRESH, + ERROR; // Throw an error when it mismatc - /** Converts to {@link BazelLockfileMode}. */ + /** Converts to {@link LockfileMode}. */ public static class Converter extends EnumConverter { public Converter() { super(LockfileMode.class, "Lockfile mode"); diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py index d96e3735a382f3..bf460a0c7c04ed 100644 --- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py +++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py @@ -171,6 +171,41 @@ def testChangeModuleInRegistryWithLockfile(self): # Even adding a new dependency should not fail due to the registry change self.RunBazel(['build', '--nobuild', '//:all']) + def testChangeModuleInRegistryWithLockfileInRefreshMode(self): + # Add module 'sss' to the registry with dep on 'aaa' + self.main_registry.createCcModule('sss', '1.3', {'aaa': '1.1'}) + # Create a project with deps on 'sss' + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name = "sss", version = "1.3")', + ], + ) + self.ScratchFile('BUILD', ['filegroup(name = "hello")']) + self.RunBazel(['build', '--nobuild', '//:all']) + + # Change registry -> update 'sss' module file (corrupt it) + module_dir = self.main_registry.root.joinpath('modules', 'sss', '1.3') + scratchFile(module_dir.joinpath('MODULE.bazel'), ['whatever!']) + + # Shutdown bazel to empty any cache of the deps tree + self.RunBazel(['shutdown']) + # Running with the lockfile, should not recognize the registry changes + # hence find no errors + self.RunBazel(['build', '--nobuild', '--lockfile_mode=refresh', '//:all']) + + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name = "sss", version = "1.3")', + 'bazel_dep(name = "bbb", version = "1.1")', + ], + ) + # Shutdown bazel to empty any cache of the deps tree + self.RunBazel(['shutdown']) + # Even adding a new dependency should not fail due to the registry change + self.RunBazel(['build', '--nobuild', '--lockfile_mode=refresh', '//:all']) + def testAddModuleToRegistryWithLockfile(self): # Create a project with deps on the BCR's 'platforms' module self.ScratchFile( @@ -206,6 +241,46 @@ def testAddModuleToRegistryWithLockfile(self): # Even adding a new dependency should not fail due to the registry change self.RunBazel(['build', '--nobuild', '//:all']) + def testAddModuleToRegistryWithLockfileInRefreshMode(self): + # Create a project with deps on the BCR's 'platforms' module + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name = "platforms", version = "0.0.9")', + ], + ) + self.ScratchFile('BUILD', ['filegroup(name = "hello")']) + self.RunBazel(['build', '--nobuild', '//:all']) + + # Add a broken 'platforms' module to the first registry + module_dir = self.main_registry.root.joinpath( + 'modules', 'platforms', '0.0.9' + ) + scratchFile(module_dir.joinpath('MODULE.bazel'), ['whatever!']) + + # Shutdown bazel to empty any cache of the deps tree + self.RunBazel(['shutdown']) + # Running with the lockfile in refresh mode should recognize the addition + # to the first registry + exit_code, _, stderr = self.RunBazel( + [ + 'build', + '--nobuild', + '--lockfile_mode=refresh', + '//:all', + ], + allow_failure=True, + ) + self.AssertExitCode(exit_code, 48, stderr) + self.assertIn( + ( + 'ERROR: Error computing the main repository mapping: in module ' + 'dependency chain -> platforms@0.0.9: error parsing ' + 'MODULE.bazel file for platforms@0.0.9' + ), + stderr, + ) + def testChangeFlagWithLockfile(self): # Create a project with an outdated direct dep on aaa self.ScratchFile( diff --git a/src/test/py/bazel/bzlmod/bazel_yanked_versions_test.py b/src/test/py/bazel/bzlmod/bazel_yanked_versions_test.py index 3cf795726b621c..e39ef7c06d016b 100644 --- a/src/test/py/bazel/bzlmod/bazel_yanked_versions_test.py +++ b/src/test/py/bazel/bzlmod/bazel_yanked_versions_test.py @@ -330,6 +330,145 @@ def testYankedVersionsFetchedIncrementally(self): ''.join(stderr), ) + def testYankedVersionsRefreshedOnModeSwitch(self): + self.writeBazelrcFile(allow_yanked_versions=False) + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name = "aaa", version = "1.0")', + ], + ) + self.ScratchFile( + 'BUILD', + [ + 'cc_binary(', + ' name = "main",', + ' srcs = ["main.cc"],', + ' deps = ["@aaa//:lib_aaa"],', + ')', + ], + ) + + # Verify that when switching to refresh mode, yanked version information is + # always updated immediately, even if it was fetched previously. + self.RunBazel(['build', '--nobuild', '--lockfile_mode=refresh', '//:main']) + self.RunBazel(['build', '--nobuild', '//:main']) + + # Yank aaa@1.0. + self.main_registry.addMetadata( + 'aaa', yanked_versions={'1.0': 'already dodgy'} + ) + + exit_code, _, stderr = self.RunBazel( + ['build', '--nobuild', '--lockfile_mode=refresh', '//:main'], + allow_failure=True, + ) + self.AssertExitCode(exit_code, 48, stderr) + self.assertIn( + 'Yanked version detected in your resolved dependency graph: ' + + 'aaa@1.0, for the reason: already dodgy.', + ''.join(stderr), + ) + + def testYankedVersionsRefreshedAfterShutdown(self): + self.writeBazelrcFile(allow_yanked_versions=False) + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name = "aaa", version = "1.0")', + ], + ) + self.ScratchFile( + 'BUILD', + [ + 'cc_binary(', + ' name = "main",', + ' srcs = ["main.cc"],', + ' deps = ["@aaa//:lib_aaa"],', + ')', + ], + ) + + self.RunBazel(['build', '--nobuild', '--lockfile_mode=refresh', '//:main']) + + # Yank aaa@1.0. + self.main_registry.addMetadata( + 'aaa', yanked_versions={'1.0': 'already dodgy'} + ) + self.RunBazel(['shutdown']) + + exit_code, _, stderr = self.RunBazel( + ['build', '--nobuild', '--lockfile_mode=refresh', '//:main'], + allow_failure=True, + ) + self.AssertExitCode(exit_code, 48, stderr) + self.assertIn( + 'Yanked version detected in your resolved dependency graph: ' + + 'aaa@1.0, for the reason: already dodgy.', + ''.join(stderr), + ) + + def testYankedVersionsRefreshedAfterAllowed(self): + self.writeBazelrcFile(allow_yanked_versions=False) + self.ScratchFile( + 'MODULE.bazel', + [ + 'bazel_dep(name = "aaa", version = "1.0")', + ], + ) + self.ScratchFile( + 'BUILD', + [ + 'cc_binary(', + ' name = "main",', + ' srcs = ["main.cc"],', + ' deps = ["@aaa//:lib_aaa"],', + ')', + ], + ) + self.RunBazel(['build', '--nobuild', '//:main']) + + # Yank aaa@1.0. + self.main_registry.addMetadata( + 'aaa', yanked_versions={'1.0': 'already dodgy'} + ) + + # Without any changes, even a warm build should fail. + exit_code, _, stderr = self.RunBazel( + ['build', '--nobuild', '--lockfile_mode=refresh', '//:main'], + allow_failure=True, + ) + self.AssertExitCode(exit_code, 48, stderr) + self.assertIn( + 'Yanked version detected in your resolved dependency graph: ' + + 'aaa@1.0, for the reason: already dodgy.', + ''.join(stderr), + ) + + # If the yanked version is allowed, the build should pass. + self.RunBazel( + ['build', '--nobuild', '--lockfile_mode=refresh', '//:main'], + env_add={'BZLMOD_ALLOW_YANKED_VERSIONS': 'aaa@1.0'}, + ) + + # Yank aaa@1.0 with a different message. + self.main_registry.addMetadata( + 'aaa', yanked_versions={'1.0': 'even more dodgy'} + ) + + # After temporarily allowing a yanked version, the yanked info is + # still refreshed. + exit_code, _, stderr = self.RunBazel( + ['build', '--nobuild', '--lockfile_mode=refresh', '//:main'], + allow_failure=True, + ) + self.AssertExitCode(exit_code, 48, stderr) + self.assertIn( + 'Yanked version detected in your resolved dependency graph: ' + + 'aaa@1.0, for the reason: even more dodgy.', + ''.join(stderr), + ) + if __name__ == '__main__': absltest.main()