Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove autoload warnings, improve errors and fix legacy repo name support #24601

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Remove autoload warnings, improve errors and fix legacy repo name support #24601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e662a9b
Remove autoload warnings and improve errors
fmeum Dec 6, 2024
7b392a6
WIP
fmeum Dec 9, 2024
d25bb4b
apple_support test
fmeum Dec 9, 2024
87c4e89
Fix tests
fmeum Dec 9, 2024
74d9820
Fix more tests
fmeum Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 49 additions & 38 deletions src/main/java/com/google/devtools/build/lib/packages/AutoloadSymbols.java
View file
Open in desktop
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
Open in desktop
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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed at some point, but doesn't matter too much right now as apple_support doesn't have any of the Starlarkified rules yet.

# __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