Skip to content

Commit

Permalink
Do not record or check repo mapping entries for repos defined in WORK…
Browse files Browse the repository at this point in the history
…SPACE

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.

Closes #21393.

PiperOrigin-RevId: 608258493
Change-Id: Ife6a01221e6f5e1685d859eaa5acc8b370ab8483
  • Loading branch information
Wyverald authored and copybara-github committed Feb 19, 2024
1 parent 9cc125e commit 1cbf09d
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -319,12 +319,17 @@ private RepositoryDirectoryValue.Builder fetchInternal(
new RepoRecordedInput.EnvVar(envKey), clientEnvironment.get(envKey));
}

for (Table.Cell<RepositoryName, String, RepositoryName> 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<RepositoryName, String, RepositoryName> repoMappings :
repoMappingRecorder.recordedEntries().cellSet()) {
recordedInputValues.put(
new RepoRecordedInput.RecordedRepoMapping(
repoMappings.getRowKey(), repoMappings.getColumnKey()),
repoMappings.getValue().getName());
}
}

env.getListener().post(resolved);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
4 changes: 4 additions & 0 deletions src/test/shell/bazel/starlark_repository_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2660,10 +2660,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 <<EOF
def _r(rctx):
print("I see: " + str(Label("@data")))
rctx.file("BUILD", "filegroup(name='r')")
rctx.file("r.bzl", "pi=3.14")
r=repository_rule(_r)
EOF
mkdir foo
Expand Down Expand Up @@ -2704,11 +2706,13 @@ 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 <<EOF
CONSTANT = Label("@data")
def _r(rctx):
print("I see: " + str(CONSTANT))
rctx.file("BUILD", "filegroup(name='r')")
rctx.file("r.bzl", "pi=3.14")
r=repository_rule(_r)
EOF
mkdir foo
Expand Down

0 comments on commit 1cbf09d

Please sign in to comment.