From eba205433aedcd55185f105adbf998ce501b84c8 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 8 Jan 2025 02:01:21 -0800 Subject: [PATCH] Remove autoload warnings, improve errors and fix legacy repo name support 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 --- .../build/lib/packages/AutoloadSymbols.java | 87 +++++++++++-------- .../incompatible_autoload_externally_test.sh | 77 ++++++++++++++-- 2 files changed, 119 insertions(+), 45 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java index 88bc48bf8a7d8f..b8b84a68430b8f 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java @@ -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; @@ -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; @@ -417,7 +417,7 @@ public ImmutableMap getLoadKeys(SkyFunction.Environmen throws InterruptedException { final RepoContext repoContext; - ImmutableMap highestVersions = ImmutableMap.of(); + ImmutableMap highestVersionRepo = ImmutableMap.of(); if (bzlmodEnabled) { BazelDepGraphValue bazelDepGraphValue = (BazelDepGraphValue) env.getValue(BazelDepGraphValue.KEY); @@ -425,19 +425,16 @@ public ImmutableMap getLoadKeys(SkyFunction.Environmen 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, @@ -464,18 +461,16 @@ public ImmutableMap getLoadKeys(SkyFunction.Environmen // Inject loads for rules and symbols removed from Bazel ImmutableMap.Builder loadKeysBuilder = ImmutableMap.builderWithExpectedSize(autoloadedSymbols.size()); - ImmutableSet.Builder 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; } } @@ -497,20 +492,8 @@ public ImmutableMap 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(); } @@ -569,9 +552,16 @@ public String getName() { @Override public Object call(StarlarkThread thread, Tuple args, Dict 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); } }); } @@ -616,7 +606,16 @@ public record SymbolRedirect( requireNonNull(rdeps, "rdeps"); } - String getModuleName() throws InterruptedException { + private static final ImmutableBiMap 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(); } @@ -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. */ @@ -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 requiredVersions; + private static final ImmutableMap requiredVersionForModules; static { try { - requiredVersions = + requiredVersionForModules = ImmutableMap.of( "protobuf", Version.parse("29.0-rc1"), // "rules_android", Version.parse("0.6.0-rc1")); @@ -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(); } diff --git a/src/test/shell/integration/incompatible_autoload_externally_test.sh b/src/test/shell/integration/incompatible_autoload_externally_test.sh index 4ce3d35c12f2de..822bd2085f6cab 100755 --- a/src/test/shell/integration/incompatible_autoload_externally_test.sh +++ b/src/test/shell/integration/incompatible_autoload_externally_test.sh @@ -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 ############################################################# @@ -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( @@ -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 @@ -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() { @@ -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 @@ -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() { @@ -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() {