-
Notifications
You must be signed in to change notification settings - Fork 180
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
Move Gazelle extension to //gazelle/bzl and change package name #261
base: main
Are you sure you want to change the base?
Conversation
This fixes an issue with importing bazel-skylib into google3. Currently, Glaze (internal Go build file generator) attempts to generate a target (//gazelle:gazelle) that conflicts with one that's already declared here. I think the right solution is actually to move the package into a subdirectory. In the future (bazel-contrib/bazel-gazelle#5), Gazelle's Go extension will generate target names similar to what Glaze does, so the same conflict will happen in open source. I think it's also logical to have a directory of packages in case more need to be added in the future, and for the extension to have a package name matching the language it works with. This is an incompatible change, but the //gazelle directory isn't part of a tagged release yet, so hopefully it won't break anyone.
cc @juliexxia If moving the package isn't an option, renaming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. As a matter of pattern spotting, is this a generic issue? Should plugin authors be avoiding being at //gazelle
? Could you update the docs on the gazelle side to reveal this?
@achew22 Good idea. I've started bazel-contrib/bazel-gazelle#857 for that, but I can't finish it until this one is merged. |
@jayconrod, feel free to merge this if you're confident that it'll work. You might try copybara importing the PR before merging though, just in case TAP hate you 😉 . You've got all the permissions you need to merge. |
I don't have write access in this repo. Someone else will need to hit the button. @juliexxia Could you confirm this will work with the Copybara import? I'm not sure if it's possible to import a PR rather than the main branch; if not, we can merge, see what goes wrong, and fix it in a follow-up. |
I believe there's a way to import a single PR and runs tests. I'll do that now. |
This fixes an issue with importing bazel-skylib into
google3. Currently, Glaze (internal Go build file generator) attempts
to generate a target (//gazelle:gazelle) that conflicts with one
that's already declared here.
I think the right solution is actually to move the package into a
subdirectory. In the future (bazel-contrib/bazel-gazelle#5), Gazelle's Go
extension will generate target names similar to what Glaze does, so
the same conflict will happen in open source. I think it's also
logical to have a directory of packages in case more need to be added
in the future, and for the extension to have a package name matching
the language it works with.
This is an incompatible change, but the //gazelle directory isn't part
of a tagged release yet, so hopefully it won't break anyone.