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.

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

feat: add skylib-gazelle compatible target for //android/rules.bzl #270

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vpanta
Copy link

@vpanta vpanta commented Oct 29, 2024

This adds a bzl_library target for //android/rules.bzl which should match the expected target name when using bazelbuild/bazel-skylib's gazelle plugin. Without this, users of the plugin who also depend on these rules will need to manually adjust their configuration.

This adds a `bzl_library` target for `//android/rules.bzl` which should match the expected target name when using [`bazelbuild/bazel-skylib`'s gazelle plugin](https://registry.bazel.build/modules/bazel_skylib_gazelle_plugin).
Without this, users of the plugin who also depend on these rules will need to manually adjust their configuration.
@ahumesky
Copy link
Collaborator

The //android package is for compatibility with the (very) obsolete 0.1.1 release which put the rules under //android. The new releases use //rules. Can gazelle use the new package? We'd like to get rid of //android at some point.

@ahumesky
Copy link
Collaborator

(it's not clear to me from a cursory search how gazelle is picking up //android https://github.com/search?q=repo%3Abazel-contrib%2Fbazel-gazelle%20android&type=code)

@vpanta
Copy link
Author

vpanta commented Oct 29, 2024

The //android package is for compatibility with the (very) obsolete 0.1.1 release which put the rules under //android. The new releases use //rules. Can gazelle use the new package? We'd like to get rid of //android at some point.

Hrm, k, then yeah that'll be trickier. The code generating this is actually a gazelle extension here, but AFAICT it's a fairly "dumb" tool: it just assumes every .bzl has a matching, single-source bzl_library target. That's why it generates/expects //android:rules for android/rules.bzl. It'd be nice to make it smarter and figure out the correct bzl_target based on the BUILD files, but doing so would take notably more time then adding one BUILD target in the short term.

Since using //rules:rules.bzl is the more appropriate way, then another possible upstream answer would to do the same as this but in rules/BUILD instead?

And if this is a no-go, no worries, I can just override it locally while I see if there's a better fix to the gazelle plugin.

And just some context: I'm making a new ruleset based on bazel-contrib/rules-template, and it uses that gazelle plugin. The ruleset will depend on rules_android.

@ahumesky
Copy link
Collaborator

Thanks for the details. Seems then that whether gazelle generates / expects a //android:rules or //rules:rules bzl_library depends on whether the codebase loads from //android:rules.bzl or //rules:rules.bzl.

So presumably then codesbases that are currently loading from //android:rules.bzl that migrate to //rules:rules would also stop using //android:rules bzl_library when gazelle regenerates the BUILD files

And presumably then this would also fail for //rules:rules.bzl because there is not a rules bzl_library target in https://github.com/bazelbuild/rules_android/blob/f0de7b98621a0bae1b4bf1adde1378c4e83c0da1/rules/BUILD ? So maybe both need to be added.

@vpanta
Copy link
Author

vpanta commented Oct 31, 2024

Yep, exactly, it generates it based on the load statement, but it assumes the target is the same name as the file.

If y'all are ready for folks to migrate to //rules:rules.bzl and //android:rules.bzl is effectively a deprecated path, then I'd say only make the change for that directory. I think it's much more likely that new users will start with the gazelle-plugin, in which case they should be using the new path. Older repos picking up the plugin should probably update anyways, and can adjust the load path then.

I do wish there were a way we could warn folks using the old load though.

For now I've changed this to doing //rules:rules only. But if you'd like me to add both, I'm happy with that.

@vpanta vpanta marked this pull request as ready for review November 4, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants