Skip to content

Commit

Permalink
Remove autoload warnings, improve errors and fix legacy repo name sup…
Browse files Browse the repository at this point in the history 
…port

This reduces noise while giving users more actionable information when they actually use a symbol that failed to autoload.

Also ensures that modules with legacy repo names (`com_google_protobuf` and `build_bazel_apple_support`) are handled correctly.

Fixes #23929
Fixes #24597

Closes #24601.

PiperOrigin-RevId: 713208045
Change-Id: I81ae8e9e09dd3a935dca2897fe9fc58d52189a07
  • Loading branch information
@fmeum @copybara-github
fmeum authored and copybara-github committed Jan 8, 2025
1 parent e0fada9 commit eba2054
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 45 deletions.
87 changes: 49 additions & 38 deletions src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static java.util.Objects.requireNonNull;

import com.google.common.collect.ImmutableBiMap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand All @@ -31,7 +32,6 @@
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.lib.skyframe.BzlLoadValue;
Expand Down Expand Up @@ -417,27 +417,24 @@ public ImmutableMap<String, BzlLoadValue.Key> getLoadKeys(SkyFunction.Environmen
throws InterruptedException {

final RepoContext repoContext;
ImmutableMap<String, ModuleKey> highestVersions = ImmutableMap.of();
ImmutableMap<String, ModuleKey> highestVersionRepo = ImmutableMap.of();
if (bzlmodEnabled) {
BazelDepGraphValue bazelDepGraphValue =
(BazelDepGraphValue) env.getValue(BazelDepGraphValue.KEY);
if (bazelDepGraphValue == null) {
return null;
}

highestVersions =
highestVersionRepo =
bazelDepGraphValue.getCanonicalRepoNameLookup().values().stream()
.collect(
toImmutableMap(
moduleKey ->
moduleKey.name().equals("protobuf")
? "com_google_protobuf"
: moduleKey.name(),
SymbolRedirect::toRepoName,
moduleKey -> moduleKey,
(m1, m2) -> m1.version().compareTo(m2.version()) >= 0 ? m1 : m2));
RepositoryMapping repositoryMapping =
RepositoryMapping.create(
highestVersions.entrySet().stream()
highestVersionRepo.entrySet().stream()
.collect(
toImmutableMap(
Map.Entry::getKey,
Expand All @@ -464,18 +461,16 @@ public ImmutableMap<String, BzlLoadValue.Key> getLoadKeys(SkyFunction.Environmen
// Inject loads for rules and symbols removed from Bazel
ImmutableMap.Builder<String, BzlLoadValue.Key> loadKeysBuilder =
ImmutableMap.builderWithExpectedSize(autoloadedSymbols.size());
ImmutableSet.Builder<String> missingRepositories = ImmutableSet.builder();
for (String symbol : autoloadedSymbols) {
String requiredModule = AUTOLOAD_CONFIG.get(symbol).getModuleName();
var redirect = AUTOLOAD_CONFIG.get(symbol);
// Skip if version doesn't have the rules
if (highestVersions.containsKey(requiredModule)
&& requiredVersions.containsKey(requiredModule)) {
if (highestVersions
.get(requiredModule)
if (highestVersionRepo.containsKey(redirect.getRepoName())
&& requiredVersionForModules.containsKey(redirect.getModuleName())) {
if (highestVersionRepo
.get(redirect.getRepoName())
.version()
.compareTo(requiredVersions.get(requiredModule))
.compareTo(requiredVersionForModules.get(redirect.getModuleName()))
<= 0) {
missingRepositories.add(requiredModule);
continue;
}
}
Expand All @@ -497,20 +492,8 @@ public ImmutableMap<String, BzlLoadValue.Key> getLoadKeys(SkyFunction.Environmen
// Only load if the dependency is present
if (repositoryExists) {
loadKeysBuilder.put(symbol, BzlLoadValue.keyForBuild(label));
} else {
missingRepositories.add(label.getRepository().getName());
}
}
for (String missingRepository : missingRepositories.build()) {
env.getListener()
.handle(
Event.warn(
String.format(
"Couldn't auto load rules or symbols, because no dependency on"
+ " module/repository '%s' found. This will result in a failure if"
+ " there's a reference to those rules or symbols.",
missingRepository)));
}
return loadKeysBuilder.buildOrThrow();
}

Expand Down Expand Up @@ -569,9 +552,16 @@ public String getName() {
@Override
public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwargs)
throws EvalException {
String what =
bzlmodEnabled
? "a 'bazel_dep(name = \"%s\", ...)' in your MODULE.bazel file"
.formatted(AUTOLOAD_CONFIG.get(symbol).getModuleName())
: "an 'http_archive(name = \"%s\", ...)' in your WORKSPACE file"
.formatted(AUTOLOAD_CONFIG.get(symbol).getRepoName());
throw Starlark.errorf(
"Couldn't auto load '%s' from '%s'.",
getName(), AUTOLOAD_CONFIG.get(getName()).loadLabel());
"Couldn't auto load '%s' from '%s'. Ensure that you have %s or add an explicit"
+ " load statement to your BUILD file.",
getName(), AUTOLOAD_CONFIG.get(getName()).loadLabel(), what);
}
});
}
Expand Down Expand Up @@ -616,7 +606,16 @@ public record SymbolRedirect(
requireNonNull(rdeps, "rdeps");
}

String getModuleName() throws InterruptedException {
private static final ImmutableBiMap<String, String> moduleToRepoName =
ImmutableBiMap.of(
"apple_support", "build_bazel_apple_support", "protobuf", "com_google_protobuf");

String getModuleName() {
String repoName = getRepoName();
return moduleToRepoName.inverse().getOrDefault(repoName, repoName);
}

String getRepoName() {
return Label.parseCanonicalUnchecked(loadLabel()).getRepository().getName();
}

Expand All @@ -627,6 +626,10 @@ Label getLabel(RepoContext repoContext) throws InterruptedException {
throw new IllegalStateException(e);
}
}

static String toRepoName(ModuleKey moduleKey) {
return moduleToRepoName.getOrDefault(moduleKey.name(), moduleKey.name());
}
}

/** Indicates a problem performing automatic loads. */
Expand Down Expand Up @@ -664,15 +667,16 @@ private static SymbolRedirect renamedSymbolRedirect(
"rules_python_internal",
"rules_shell",
"apple_support",
"build_bazel_apple_support",
"bazel_skylib",
"bazel_tools",
"bazel_features");

private static final ImmutableMap<String, Version> requiredVersions;
private static final ImmutableMap<String, Version> requiredVersionForModules;

static {
try {
requiredVersions =
requiredVersionForModules =
ImmutableMap.of(
"protobuf", Version.parse("29.0-rc1"), //
"rules_android", Version.parse("0.6.0-rc1"));
Expand Down Expand Up @@ -849,14 +853,21 @@ private static SymbolRedirect renamedSymbolRedirect(
.put("sh_binary", ruleRedirect("@rules_shell//shell:sh_binary.bzl"))
.put("sh_library", ruleRedirect("@rules_shell//shell:sh_library.bzl"))
.put("sh_test", ruleRedirect("@rules_shell//shell:sh_test.bzl"))
.put("available_xcodes", ruleRedirect("@apple_support//xcode:available_xcodes.bzl"))
.put("xcode_config", ruleRedirect("@apple_support//xcode:xcode_config.bzl"))
.put("xcode_config_alias", ruleRedirect("@apple_support//xcode:xcode_config_alias.bzl"))
.put("xcode_version", ruleRedirect("@apple_support//xcode:xcode_version.bzl"))
.put(
"available_xcodes",
ruleRedirect("@build_bazel_apple_support//xcode:available_xcodes.bzl"))
.put("xcode_config", ruleRedirect("@build_bazel_apple_support//xcode:xcode_config.bzl"))
.put(
"xcode_config_alias",
ruleRedirect("@build_bazel_apple_support//xcode:xcode_config_alias.bzl"))
.put("xcode_version", ruleRedirect("@build_bazel_apple_support//xcode:xcode_version.bzl"))
// this redirect doesn't exists and probably never will, we still need a configuration for
// it, so that it can be removed from Bazels <= 7 if needed
.put(
"apple_common",
symbolRedirect("@apple_support//lib:apple_common.bzl", "objc_import", "objc_library"))
symbolRedirect(
"@build_bazel_apple_support//lib:apple_common.bzl",
"objc_import",
"objc_library"))
.buildOrThrow();
}
77 changes: 70 additions & 7 deletions src/test/shell/integration/incompatible_autoload_externally_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ source "${CURRENT_DIR}/../integration_test_setup.sh" \
set -e

# Used to pass --noenable_bzlmod, --enable_workpace flags
add_to_bazelrc "build $@"
if [[ "${1:-}" == "--enable_bzlmod" ]]; then
is_bzlmod_enabled=true
else
is_bzlmod_enabled=false
fi
add_to_bazelrc "common $@"

#### TESTS #############################################################

Expand Down Expand Up @@ -106,10 +111,52 @@ local_repository(
EOF
}

function mock_apple_support() {
apple_support_workspace="${TEST_TMPDIR}/apple_support_workspace"
mkdir -p "${apple_support_workspace}/xcode"
touch "${apple_support_workspace}/xcode/BUILD"
touch "${apple_support_workspace}/WORKSPACE"
cat > "${apple_support_workspace}/MODULE.bazel" << EOF
module(name = "apple_support", repo_name = "build_bazel_apple_support")
EOF
cat > "${apple_support_workspace}/xcode/xcode_version.bzl" << EOF
def _impl(ctx):
pass
xcode_version = rule(
implementation = _impl,
attrs = {
"version": attr.string(),
}
)
EOF

cat >> MODULE.bazel << EOF
bazel_dep(
name = "apple_support",
repo_name = "build_bazel_apple_support",
)
local_path_override(
module_name = "apple_support",
path = "${apple_support_workspace}",
)
EOF

cat > WORKSPACE << EOF
workspace(name = "test")
local_repository(
name = "build_bazel_apple_support",
path = "${apple_support_workspace}",
)
EOF
}

function test_missing_necessary_repo_fails() {
# Intentionally not adding apple_support to MODULE.bazel (and it's not in MODULE.tools)
cat > WORKSPACE << EOF
workspace(name = "test")
# TODO: protobuf's WORKSPACE macro has an unnecessary dependency on build_bazel_apple_support.
# __SKIP_WORKSPACE_SUFFIX__
EOF
cat > BUILD << EOF
xcode_version(
Expand All @@ -118,11 +165,15 @@ xcode_version(
)
EOF
bazel build --incompatible_autoload_externally=xcode_version :xcode_version >&$TEST_log 2>&1 && fail "build unexpectedly succeeded"
expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'apple_support' found. This will result in a failure if there's a reference to those rules or symbols."
if "$is_bzlmod_enabled"; then
expect_log "Couldn't auto load 'xcode_version' from '@build_bazel_apple_support//xcode:xcode_version.bzl'. Ensure that you have a 'bazel_dep(name = \"apple_support\", ...)' in your MODULE.bazel file or add an explicit load statement to your BUILD file."
else
expect_log "Couldn't auto load 'xcode_version' from '@build_bazel_apple_support//xcode:xcode_version.bzl'. Ensure that you have an 'http_archive(name = \"build_bazel_apple_support\", ...)' in your WORKSPACE file or add an explicit load statement to your BUILD file."
fi
}

function test_missing_unnecessary_repo_doesnt_fail() {
# Intentionally not adding apple_support to MODULE.bazel (and it's not in MODULE.tools)
# Intentionally not adding rules_android to MODULE.bazel (and it's not in MODULE.tools)
cat > WORKSPACE << EOF
workspace(name = "test")
EOF
Expand All @@ -132,8 +183,7 @@ filegroup(
srcs = [],
)
EOF
bazel build --incompatible_autoload_externally=xcode_version :filegroup >&$TEST_log 2>&1 || fail "build failed"
expect_log "WARNING: Couldn't auto load rules or symbols, because no dependency on module/repository 'apple_support' found. This will result in a failure if there's a reference to those rules or symbols."
bazel build --incompatible_autoload_externally=+@rules_android :filegroup >&$TEST_log 2>&1 || fail "build failed"
}

function test_removed_rule_loaded() {
Expand All @@ -151,6 +201,19 @@ EOF
bazel build --incompatible_autoload_externally=aar_import :aar >&$TEST_log 2>&1 || fail "build failed"
}

function test_removed_rule_loaded_from_legacy_repo_name() {
setup_module_dot_bazel
mock_apple_support

cat > BUILD << EOF
xcode_version(
name = 'xcode_version',
version = "5.1.2",
)
EOF
bazel build --incompatible_autoload_externally=xcode_version :xcode_version >&$TEST_log 2>&1 || fail "build failed"
}

function test_removed_rule_loaded_from_bzl() {
setup_module_dot_bazel
mock_rules_android
Expand Down Expand Up @@ -210,7 +273,7 @@ sh_library(
)
EOF
bazel query --incompatible_autoload_externally=+sh_library ':sh_library' --output=build >&$TEST_log 2>&1 || fail "build failed"
expect_log "rules_shell./shell/private/sh_library.bzl"
expect_log "rules_shell.\\?/shell/private/sh_library.bzl"
}

function test_existing_rule_is_redirected_in_bzl() {
Expand All @@ -227,7 +290,7 @@ macro()
EOF

bazel query --incompatible_autoload_externally=+sh_library ':sh_library' --output=build >&$TEST_log 2>&1 || fail "build failed"
expect_log "rules_shell./shell/private/sh_library.bzl"
expect_log "rules_shell.\\?/shell/private/sh_library.bzl"
}

function test_removed_rule_not_loaded() {
Expand Down

0 comments on commit eba2054

Please sign in to comment.