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

Use repo-relative labels where easily possible #3038

Merged
merged 8 commits into from
Jan 14, 2022

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 28, 2021

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.

@fmeum fmeum changed the title Use repo-relative labels Use repo-relative labels where easily possible Dec 28, 2021
@fmeum fmeum marked this pull request as ready for review December 28, 2021 11:33
@achew22
Copy link
Member

achew22 commented Dec 28, 2021

Looks like a good change to me! Can you address that one comment and then I'll merge it?

@sluongng
Copy link
Contributor

Looks good to me but the changes from string -> Label(string) might worth explaining for future reference.

@achew22
Copy link
Member

achew22 commented Jan 14, 2022

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?

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 14, 2022

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 io_bazel_rules_go would have to be replaced by rules_go. Luckily, since the registry allows patches, this last step can always be taken care of by a simple "replace all". In fact, since rules_go contains patches that add BUILD files to third-party Go repositories, which have to refer to rules_go in some hard-coded way, rules_go will not be able to go "patch-free" anyway.

My current plan for adding module support roughly goes as follows:

  1. Get rid of as many hard-coded instances of io_bazel_rules_go as possible to keep the required patches small. It's completely fine if new such instances are added or we miss something, since patches will be required anyway.
  2. Implement a module extension that replaces the current repository rules for Go SDKs. Ideally, this could be merged into master rather than being patched in. Since this would only require adding files that would be ignored by current versions of Bazel, that shouldn't be an issue.
  3. Submit the module to the registry with patches for the parts that couldn't be handled in this repo without breaking backwards compatibility.
  4. Add a few tests to the BCR that verify that it works as a module. Proper testing support has only been added a few days ago and is still a bit of a work in progress.

@achew22
Copy link
Member

achew22 commented Jan 14, 2022

Honestly, it's long been on the roadmap to rename everything to rules_go from the current abomination. The end state I would like out of this set of PRs would be that rules_go is known by that name globally (at least in the OSS ecosystem).

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.

@achew22 achew22 merged commit 979c3c6 into bazelbuild:master Jan 14, 2022
@fmeum fmeum deleted the relative-repo-labels branch January 15, 2022 10:15
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 15, 2022

Honestly, it's long been on the roadmap to rename everything to rules_go from the current abomination. The end state I would like out of this set of PRs would be that rules_go is known by that name globally (at least in the OSS ecosystem).

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.

@robfig
Copy link
Contributor

robfig commented Jan 15, 2022

Also, do you plan to increase the minimum Bazel version to 5.0.0 soon after its release?

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

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 15, 2022

Okay, then how about the following:

  1. We try to use repo-relative labels where possible while maintaining backwards compatibility with Bazel 4.
  2. We work on setting up rules_go as a module called io_bazel_rules_go within this repo. This will only require adding files that are ignored by Bazel 4, so there are no backwards compatibility concerns for this step.
  3. We submit rules_go to the Bazel Central Registry as a module called rules_go with the help of patches.
  4. Some time after Bazel 5 and Go 1.18 have been released, we consider renaming rules_go also for WORKSPACE users.

@robfig @achew22 What do you think about this approach?

@achew22
Copy link
Member

achew22 commented Jan 20, 2022

I'm a big fan of moving us to the more canonical rules_go name. We should definitely update Gazelle to prefer that name as part of the the transitional process.

Do you know, @fmeum, is it possible to expose a workspace dependency by multiple names? Could we continue to be loadable from io_bazel_rules_go indefinitely while we prefer the name rules_go?

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 20, 2022

There is no direct functionality for this I'm aware of, but we could add a @io_bazel_rules_go local_repository that just contains a go/def.bzl that imports the rules used by Gazelle from @rules_go and reexports them. We could then make this repository's creation configurable in go_rules_dependencies, so users could decide to drop it once they have migrated. How does that sound?

@sluongng
Copy link
Contributor

FWIW, the way rules_nodejs is doing a similar migration where they created a rules_nodejs_core which is loaded by rules_nodejs, then slowly move things into core and eventually deprecate the none-core stuff.

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.

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 20, 2022

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 rules_go bzlmod module easily with the help of patches that can shrink over time and bzlmod's design ensures that users are always able to rename repositories as they wish (e.g., use rules_go as a module but call the resulting repository io_bazel_rules_go for backwards compatibility).

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.

None yet

4 participants