From 8a53cd2348efd4a637d9152dd0133435849503d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Fri, 16 Feb 2024 17:36:09 -0500 Subject: [PATCH] Do not record or check repo mapping entries for repos defined in WORKSPACE Mappings for repos defined in WORKSPACE are extremely annoying to verify given the chunked loading (we'd have to record which chunk the repo mapping was used in, and then load that chunk while verifying). This is extremely not worth the effort (especially since nobody really uses repo mappings in WORKSPACE) so we just don't record it. This also means, during verification, we can safely use the main repo mapping without WORKSPACE (since repos defined in Bzlmod can't see stuff from WORKSPACE anyway). Fixes #21289. --- .../starlark/StarlarkRepositoryFunction.java | 17 +++++++++++------ .../lib/rules/repository/RepoRecordedInput.java | 7 ++++++- .../repository/RepositoryDelegatorFunction.java | 10 +++------- .../rules/repository/RepositoryFunction.java | 6 ++++++ .../shell/bazel/starlark_repository_test.sh | 4 ++++ 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index d350edef1cff03..b711378b51f0ee 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java @@ -319,12 +319,17 @@ private RepositoryDirectoryValue.Builder fetchInternal( new RepoRecordedInput.EnvVar(envKey), clientEnvironment.get(envKey)); } - for (Table.Cell repoMappings : - repoMappingRecorder.recordedEntries().cellSet()) { - recordedInputValues.put( - new RepoRecordedInput.RecordedRepoMapping( - repoMappings.getRowKey(), repoMappings.getColumnKey()), - repoMappings.getValue().getName()); + // For repos defined in Bzlmod, record any used repo mappings in the marker file. + // Repos defined in WORKSPACE are impossible to verify given the chunked loading (we'd have to + // record which chunk the repo mapping was used in, and ain't nobody got time for that). + if (!isWorkspaceRepo(rule)) { + for (Table.Cell repoMappings : + repoMappingRecorder.recordedEntries().cellSet()) { + recordedInputValues.put( + new RepoRecordedInput.RecordedRepoMapping( + repoMappings.getRowKey(), repoMappings.getColumnKey()), + repoMappings.getValue().getName()); + } } env.getListener().post(resolved); diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index 9c8a6369109119..605bd65a7ce3c5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -559,7 +559,12 @@ public String toStringInternal() { @Override public SkyKey getSkyKey(BlazeDirectories directories) { - return RepositoryMappingValue.key(sourceRepo); + // Since we only record repo mapping entries for repos defined in Bzlmod, we can request the + // WORKSPACE-less version of the main repo mapping (as no repos defined in Bzlmod can see + // stuff from WORKSPACE). + return sourceRepo.isMain() + ? RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS + : RepositoryMappingValue.key(sourceRepo); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 63578d8b1efdc0..4d8c46f8d5faa9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.rules.repository; -import static com.google.devtools.build.lib.cmdline.LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.annotations.VisibleForTesting; @@ -394,12 +393,9 @@ private boolean shouldUseVendorRepos(Environment env, RepositoryFunction handler } private boolean shouldExcludeRepoFromVendoring(RepositoryFunction handler, Rule rule) { - return handler.isLocal(rule) || handler.isConfigure(rule) || isWorkspaceRepo(rule); - } - - private boolean isWorkspaceRepo(Rule rule) { - // All workspace repos are under //external, while bzlmod repo rules are not - return rule.getPackage().getPackageIdentifier().equals(EXTERNAL_PACKAGE_IDENTIFIER); + return handler.isLocal(rule) + || handler.isConfigure(rule) + || RepositoryFunction.isWorkspaceRepo(rule); } @Nullable diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index 1533eaca58c46e..591211ce441f77 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.rules.repository; import static com.google.common.base.Preconditions.checkState; +import static com.google.devtools.build.lib.cmdline.LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -137,6 +138,11 @@ public static void setupRepoRoot(Path repoRoot) throws RepositoryFunctionExcepti } } + public static boolean isWorkspaceRepo(Rule rule) { + // All workspace repos are under //external, while bzlmod repo rules are not + return rule.getPackage().getPackageIdentifier().equals(EXTERNAL_PACKAGE_IDENTIFIER); + } + protected void setupRepoRootBeforeFetching(Path repoRoot) throws RepositoryFunctionException { setupRepoRoot(repoRoot); } diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 06e70124d12120..e9c38fe1bc1c6d 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2627,10 +2627,12 @@ bazel_dep(name="bar") local_path_override(module_name="bar", path="bar") EOF touch BUILD + echo 'load("@r//:r.bzl", "pi"); print(pi)' > WORKSPACE.bzlmod cat > r.bzl < WORKSPACE.bzlmod cat > r.bzl <