Skip to content

Commit

Permalink
[8.0.1] Remove autoload warnings, improve errors and fix legacy repo …
Browse files Browse the repository at this point in the history 
…name support (#24856)

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

Commit
eba2054

Co-authored-by: Fabian Meumertzheim <[email protected]>
  • Loading branch information
@bazel-io @fmeum
bazel-io and fmeum authored Jan 8, 2025
1 parent b746d66 commit 253ceff
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 @@ -617,7 +607,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 @@ -628,6 +627,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 @@ -665,15 +668,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 @@ -850,14 +854,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 253ceff

Please sign in to comment.