-
Notifications
You must be signed in to change notification settings - Fork 655
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
Use repo-relative labels where easily possible #3038
Conversation
4c8b92b
to
3511672
Compare
4fd8a58
to
60ed628
Compare
Looks like a good change to me! Can you address that one comment and then I'll merge it? |
This macro can now be reused as a module extension by patching out the native.existing_rules() call in _maybe.
60ed628
to
72cebab
Compare
Looks good to me but the changes from string -> Label(string) might worth explaining for future reference. |
I think this is a great question. @fmeum, if someone started using the incorrect form of one of these would we get a test case that failed, or would we only catch it with a user report? |
At the point where we are ready to submit a version of rules_go to the Bazel Central Registry, all occurences of My current plan for adding module support roughly goes as follows:
|
Honestly, it's long been on the roadmap to rename everything to Do you think that's a good end state? Hopefully that will keep the number of patches needed to get us into BCR to a minimum. |
It's a good but not necessary end state. What degree of backwards compatibility would you like to offer throughout the rename? The situation for rules_go seems somewhat more complicated due to the tight coupling with Gazelle's BUILD file generation. Also, do you plan to increase the minimum Bazel version to 5.0.0 soon after its release? That would allow using repo-relative labels in toolchain attributes as well as get rid of the special handling for transition labels. At that point, it may be possible to go with patches for the third-party BUILD files only. |
We avoid bumping minimum versions too quickly because it can result in a situation where users want to upgrade their Go version (requiring a new rules_go) but are not able to upgrade Bazel yet. If this provided significant benefit, we could consider something out of the ordinary -- adding patch releases to the last pre-5.0 rules_go for new go versions so they aren't left behind. That probably wouldn't be so bad |
Okay, then how about the following:
|
I'm a big fan of moving us to the more canonical Do you know, @fmeum, is it possible to expose a workspace dependency by multiple names? Could we continue to be loadable from |
There is no direct functionality for this I'm aware of, but we could add a |
FWIW, the way rules_nodejs is doing a similar migration where they created a I thought about doing it for rules_go as there are quite some dead code around the place, and we might want to clean them up to implement new features(coverage, tinygo etc...) a bit easier. But it would require a huge effort invested into rules_go. |
I can help with the bzlmod packaging, but these kind of large-scale refactorings are out of scope for me. But I don't think that these are necessarily coupled: rules_go can be released as a |
What type of PR is this?
Refactoring, but required for a new feature
What does this PR do? Why is it needed?
It refactors many of the places where hardcoded references to the repository name
io_bazel_rules_go
are used.This makes it easier to publish rules_go as a bzlmod module called
rules_go
without large patches.All changes in this PR should be fully backwards compatible with Bazel 4.2.1.
Which issues(s) does this PR fix?
Works towards #3020
Other notes for review
If it helps with the review, I can submit every commit as an individual PR. They serve a common purpose, but are otherwise almost completely independent of each other.