From f15e0c7224ecc5473d4972afc436e28df35c4e5a Mon Sep 17 00:00:00 2001 From: Brentley Jones Date: Mon, 7 Mar 2022 12:44:17 -0600 Subject: [PATCH] Add --experimental_repository_cache_urls_as_default_canonical_id to help detect broken repository URLs (#14989) This new flag can be used to force redownloading when repository URLs are changed. Otherwise, it's possible broken URLs can be masked by the presence of a repository cache entry with the same hash. Specifying a `canonical_id` works as before, overriding this behavior. Closes #14128. Closes #14268. PiperOrigin-RevId: 420976730 (cherry picked from commit f9882e41382fff23f4862841631da593c9fe2344) Co-authored-by: Ken Micklas --- .../lib/bazel/BazelRepositoryModule.java | 1 + .../bazel/repository/RepositoryOptions.java | 13 ++++ .../downloader/DownloadManager.java | 10 +++ .../bazel/bazel_repository_cache_test.sh | 64 +++++++++++++++++++ 4 files changed, 88 insertions(+) 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 8f489b3dbc09a7..cf161827de5e18 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 @@ -274,6 +274,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { if (repoOptions.repositoryDownloaderRetries >= 0) { downloadManager.setRetries(repoOptions.repositoryDownloaderRetries); } + downloadManager.setUrlsAsDefaultCanonicalId(repoOptions.urlsAsDefaultCanonicalId); repositoryCache.setHardlink(repoOptions.useHardlinks); if (repoOptions.experimentalScaleTimeouts > 0.0) { 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 5f28b960fe798d..6a0c5c6f550014 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 @@ -215,6 +215,19 @@ public class RepositoryOptions extends OptionsBase { + " to escalate it to a resolution failure.") public CheckDirectDepsMode checkDirectDependencies; + @Option( + name = "experimental_repository_cache_urls_as_default_canonical_id", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.EXPERIMENTAL}, + help = + "If true, use a string derived from the URLs of repository downloads as the canonical_id " + + "if not specified. This causes a change in the URLs to result in a redownload even " + + "if the cache contains a download with the same hash. This can be used to verify " + + "that URL changes don't result in broken repositories being masked by the cache.") + public boolean urlsAsDefaultCanonicalId; + /** An enum for specifying different modes for checking direct dependency accuracy. */ public enum CheckDirectDepsMode { OFF, // Don't check direct dependency accuracy. diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java index 1548ea7b0bc1f7..b8c1fdf2401c42 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java @@ -36,6 +36,7 @@ import java.net.URL; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import javax.annotation.Nullable; /** @@ -52,6 +53,7 @@ public class DownloadManager { private final Downloader downloader; private boolean disableDownload = false; private int retries = 0; + private boolean urlsAsDefaultCanonicalId; public DownloadManager(RepositoryCache repositoryCache, Downloader downloader) { this.repositoryCache = repositoryCache; @@ -75,6 +77,10 @@ public void setRetries(int retries) { this.retries = retries; } + public void setUrlsAsDefaultCanonicalId(boolean urlsAsDefaultCanonicalId) { + this.urlsAsDefaultCanonicalId = urlsAsDefaultCanonicalId; + } + /** * Downloads file to disk and returns path. * @@ -109,6 +115,10 @@ public Path download( throw new InterruptedException(); } + if (Strings.isNullOrEmpty(canonicalId) && urlsAsDefaultCanonicalId) { + canonicalId = originalUrls.stream().map(URL::toExternalForm).collect(Collectors.joining(" ")); + } + ImmutableList rewrittenUrls = ImmutableList.copyOf(originalUrls); Map> rewrittenAuthHeaders = authHeaders; diff --git a/src/test/shell/bazel/bazel_repository_cache_test.sh b/src/test/shell/bazel/bazel_repository_cache_test.sh index 11b9eedfdc606a..28235a0557c272 100755 --- a/src/test/shell/bazel/bazel_repository_cache_test.sh +++ b/src/test/shell/bazel/bazel_repository_cache_test.sh @@ -465,4 +465,68 @@ EOF expect_log "Error downloading" } +function test_break_url() { + setup_repository + + bazel fetch --repository_cache="$repo_cache_dir" //zoo:breeding-program >& $TEST_log \ + || echo "Expected fetch to succeed" + + shutdown_server + + bazel fetch --repository_cache="$repo_cache_dir" //zoo:breeding-program >& $TEST_log \ + || echo "Expected fetch to succeed" + + # Break url in WORKSPACE + rm WORKSPACE + cat >> $(create_workspace_with_default_repos WORKSPACE) <& $TEST_log \ + || echo "Expected fetch to succeed" +} + +function test_experimental_repository_cache_urls_as_default_canonical_id() { + setup_repository + + bazel fetch --repository_cache="$repo_cache_dir" \ + --experimental_repository_cache_urls_as_default_canonical_id \ + //zoo:breeding-program >& $TEST_log \ + || echo "Expected fetch to succeed" + + shutdown_server + + bazel fetch --repository_cache="$repo_cache_dir" \ + --experimental_repository_cache_urls_as_default_canonical_id \ + //zoo:breeding-program >& $TEST_log \ + || echo "Expected fetch to succeed" + + # Break url in WORKSPACE + rm WORKSPACE + cat >> $(create_workspace_with_default_repos WORKSPACE) <& $TEST_log \ + && fail "expected failure" || : +} + run_suite "repository cache tests"