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

Replace go_name_hack with a usage of Label #3035

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

fmeum
Copy link
Member

@fmeum fmeum commented Dec 19, 2021

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

@fmeum fmeum closed this Dec 19, 2021
@fmeum fmeum reopened this Dec 19, 2021
@fmeum fmeum force-pushed the remove-go-name-hack branch 2 times, most recently from b26e0a8 to c536218 Compare December 20, 2021 07:23
@fmeum fmeum marked this pull request as ready for review December 20, 2021 07:33
@fmeum fmeum force-pushed the remove-go-name-hack branch from c536218 to be988df Compare December 20, 2021 07:35
@fmeum
Copy link
Member Author

fmeum commented Dec 20, 2021

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

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.

@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

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
Copy link
Member

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.

Copy link
Member Author

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.
@fmeum fmeum force-pushed the remove-go-name-hack branch from be988df to 4f0fcbd Compare December 20, 2021 15:08
@achew22 achew22 merged commit 80d5afe into bazel-contrib:master Dec 28, 2021
@achew22
Copy link
Member

achew22 commented Dec 28, 2021

Thanks @fmeum!

@fmeum fmeum deleted the remove-go-name-hack branch December 28, 2021 17:35
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