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

Jump to bottom

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

wants to merge 5 commits into from
+119 −45

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 6, 2024 •
edited
Loading

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

@fmeum
Remove autoload warnings and improve errors
e662a9b 
This reduces noise while giving users more actionable information when they actually use a symbol that failed to autoload.
@fmeum fmeum requested review from comius and meteorcloudy December 6, 2024 22:01
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Dec 6, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 6, 2024

FYI @keith

Based on the test, it looks like apple_support isn't loaded as bazel_build_apple_support in WORKSPACE mode. Should we also fix that?

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 6, 2024

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 6, 2024
@keith
Copy link
Member

keith commented Dec 6, 2024

huh i hadn't seen that test at all..im not sure

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 6, 2024

The same may be true for protobuf.

@comius
Copy link
Contributor

comius commented Dec 9, 2024

I tried adding @apple_support to the --incompatible_autoload_externally, but I had some problems with it. I can't find a PR with that change. Trying to remember, the problems might have been due to apple_common, that doesn't exist anywhere (and won't). But autoloading all 3 XCode rules could work.

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 9, 2024

@comius I think I'm close to getting something to work, I'll push an update soon.

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 9, 2024 •
edited
Loading

@comius Protobuf depends on apple_support via its WORKSPACE macro (https://github.com/protocolbuffers/protobuf/blob/2d4414f384dc499af113b5991ce3eaa9df6dd931/protobuf_deps.bzl#L158-L162), but not via its BCR MODULE.bazel file. The WORKSPACE version it loads doesn't have the autoloaded symbols.

@fmeum fmeum changed the title Remove autoload warnings and improve errors Remove autoload warnings, improve errors and fix legacy repo name support Dec 9, 2024
@fmeum fmeum requested a review from comius December 9, 2024 14:59
fmeum
fmeum commented Dec 9, 2024
View reviewed changes
src/test/shell/integration/incompatible_autoload_externally_test.sh
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.

@iancha1992
Copy link
Member

@bazel-io fork 8.1.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 10, 2024
@bazel-io bazel-io mentioned this pull request Dec 10, 2024
Closed
@iancha1992 iancha1992 added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts team-Android Issues for Android team labels Dec 10, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 11, 2024

@bazel-io fork 8.0.1

@bazel-io bazel-io mentioned this pull request Dec 11, 2024
Open
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 19, 2024

@comius Friendly ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meteorcloudy meteorcloudy Awaiting requested review from meteorcloudy

@comius comius Awaiting requested review from comius

Assignees
No one assigned
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Android Issues for Android team team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WARNING: Couldn't auto load rules or symbols Noisy rules_android autoload warning
5 participants
@fmeum @keith @comius @iancha1992 @bazel-io