-
-
Notifications
You must be signed in to change notification settings - Fork 678
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
Replace go_name_hack with a usage of Label #3035
Conversation
b26e0a8
to
c536218
Compare
c536218
to
be988df
Compare
@robfig Would you be interested in getting rules_go into the Bazel Central Registry as a Bazel module? I could work on it using only patches at first, but of course would prefer to upstream (backwards compatible) changes at some point. This is one of them which I found to be generally useful. |
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.
@fmeum thanks so much for the contribution! I'm really excited to have this in place. I've been bitten a number of times by this hack (skylib was made difficult by it) and would love to get rid of it. Just a small comment
go/private/rules/transition.bzl
Outdated
return label[len("@io_bazel_rules_go"):] | ||
else: | ||
if label.startswith("//command_line_option:"): | ||
# This is a special label used to refer to Bazel command-line options in |
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.
Can you add a note about why are these special and when it could be removed? My experience is that hacks like this get put in place and I forget to write down the condition where it can be removed safely so I carry forward scar tissue.
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.
Good point, I expanded the comment. Could you give it another read?
The Label constructor can be used to transform a label string such as @io_bazel_rules_go//foo:bar into the canonical label referencing this target from the repository in which it is called. For example, `str(Label("@io_bazel_rules_go//foo:bar"))` returns `"@io_bazel_rules_go//foo:bar"` if rules_go is used as an external repository from another main workspace, but returns `"//foo:bar"` when rules_go is the main repository (e.g., when running the tests in the CI). This requires raising the minimum version of Bazel from 4.2.0 to 4.2.1 due to bazelbuild/bazel#13890.
be988df
to
4f0fcbd
Compare
Thanks @fmeum! |
What type of PR is this?
Other
What does this PR do? Why is it needed?
The Label constructor can be used to transform a label string such as @io_bazel_rules_go//foo:bar into the canonical label referencing this target from the repository in which it is called.
For example,
str(Label("@io_bazel_rules_go//foo:bar"))
returns"@io_bazel_rules_go//foo:bar"
if rules_go is used as an external repository from another main workspace, but returns"//foo:bar"
when rules_go is the main repository (e.g., when running the tests in the CI).Which issues(s) does this PR fix?
It doesn't resolve a bug, but simplifies the rules_go repository setup. This simplification will be especially useful when converting
rules_go
to a Bazel module in the future.Other notes for review