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

Move Gazelle extension to //gazelle/bzl and change package name #261

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

Conversation

jayconrod
Copy link
Contributor

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 (bazelbuild/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.

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 (bazelbuild/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.
@jayconrod
Copy link
Contributor Author

cc @juliexxia

If moving the package isn't an option, renaming //gazelle:gazelle would be a narrower fix.

Copy link
Member

@achew22 achew22 left a 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?

@jayconrod
Copy link
Contributor Author

@achew22 Good idea. I've started bazelbuild/bazel-gazelle#857 for that, but I can't finish it until this one is merged.

@achew22
Copy link
Member

achew22 commented Aug 6, 2020

@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.

@jayconrod
Copy link
Contributor Author

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.

@juliexxia
Copy link
Contributor

I believe there's a way to import a single PR and runs tests. I'll do that now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants