-
Notifications
You must be signed in to change notification settings - Fork 378
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
bzlmod: Add macro to convert import paths to Bazel labels #1423
base: master
Are you sure you want to change the base?
Conversation
With the new `go` helper macro provided by the `go_deps` module extension, users can optionally specify their external Go dependencies in `deps` lists using their Go import paths rather than Bazel labels. For example, to depend on `github.com/stretchr/testify/require`, users can add either of the following to the `deps` of a `go_*` rule after loading the `go` macro from `@go_deps//:def.bzl`: - `"@com_github_stretchr_testify//require"` (old) - `go("github.com/stretchr/testify/require")` (new) When users specify an import path that isn't provided by a `go_repository`, they are shown the Go command that fetches the missing dependency (assuming they are already using `go_deps.from_file`). Note: This commit does *not* add support for this new deps format to Gazelle itself.
@alexeagle @gregmagolan @kormide This is what I came up with after our call. I stand corrected: Being able to use a macro does make a difference, but only due to additional hacky trickery (a generated registry of canonical repository names). I don't think there is a way to do this without a macro. |
Marked as draft just since I wouldn't want to commit |
@@ -10,6 +11,6 @@ go_test( | |||
srcs = ["mvs_test.go"], | |||
deps = [ | |||
"@com_github_pelletier_go_toml//:go-toml", | |||
"@com_github_stretchr_testify//require", | |||
go("github.com/stretchr/testify/require"), |
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.
This is an example usage.
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.
The last time I tried to give a function expression to a list was while developing the Python extension, Gazelle had problems performing merges on subsequent runs. Has it changed?
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.
That's a good question, I don't know. We could and should fix that if these kind of helper functions turn out to be the right approach.
internal/bzlmod/go_deps.bzl
Outdated
go_deps_canonical = Label("@bazel_gazelle_go_repository_directives//foo").workspace_name | ||
base_canonical = go_deps_canonical[:go_deps_canonical.rfind("~") + 1] | ||
return "@" + base_canonical + repo_name |
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.
This is the part that's hacky.
@Wyverald helped me unhack this. |
module_repo = module_path_to_repo.get(module_path, None) | ||
if module_repo: | ||
package_path = import_path[split_pos:].removeprefix("/") | ||
return canonicalize("@{}//{}:{}".format(module_repo, package_path, _lib_name_from_import_path(import_path))) |
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.
Is module_repo
an alias repo that points into each of the go dep repos?
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.
module_repo
is the apparent name of the repo used by the go_deps
extension. Since the go_deps
repo created by that extension passes its Label
constructor into this function, we can use it here to convert the apparent into a canonical repo name.
@@ -38,7 +38,7 @@ go_deps.module( | |||
use_repo( | |||
go_deps, | |||
"com_github_pelletier_go_toml", | |||
"com_github_stretchr_testify", |
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.
amazing, this means that users need only one use_repo call and matches the other languages I know of. Love it!
@@ -15,6 +15,26 @@ def _repo_name(importpath): | |||
candidate_name = "_".join(segments).replace("-", "_") | |||
return "".join([c.lower() if c.isalnum() else "_" for c in candidate_name.elems()]) | |||
|
|||
_GO_DEPS_HELPER_DEFS_BZL = """load("@bazel_gazelle//internal/bzlmod:go_helper.bzl", "go_helper") | |||
|
|||
def go(import_path): |
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.
nit: bike-shedding on the name go
here
- it's delightfully short which is nice, but not descriptive of what it does
- In the language, the keyword
go
is reserved for goroutines right? Is there anywhere else we use the bare symbolgo
in rules_go right now? label_for_package
is descriptive but will look ugly in dozens of consecutive lines of adeps
list- we're kinda giving a way to address a thing from https://pkg.go.dev/ so maybe
go_pkg
is a nice middle ground? go_get
is also a cute name that's associated with "bring a package from the registry into my build" but it would be confusing ifgo_get
gave an error saying "oh that's not installed, first you need togo get
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.
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.
Personally I don't view the name go
as an affront. If someone finds themselves confused by it they can alias it to be something different.
That said, I'm slightly concerned about having a macro that transmogrifies the import path github.com/bazelbuild/rules_go/foo
into the bzlmod form. This would mean that users are unable to grep their codebase for the string that actually shows up in their BUILD file.
Why have we decided to go this route now? It seems we were able to make the transition to bzlmod so this can't be a precondition. Is it?
If we don't think the bazel syntax is good enough, maybe we should turn go_library
into a macro and have it invoke the go
function when it detected something that isn't label shaped?
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.
Sorry, we had a meeting where we discussed some of the context for this, I'm happy to explain the motivation.
It seems we were able to make the transition to bzlmod so this can't be a precondition. Is it?
The transition to bzlmod is not really complete, because it currently requires users to add every third-party go repository to the use_repo
call in their MODULE.bazel
file.
#1404 is one proposed solution to this, which adds another code-generation mechanism to update this section of the file. However, it's making Go unique in the bazel ecosystem, which I think makes it harder for users.
Every other language that works well under bzlmod has a form where users only load from a single "trampoline" repo for the whole transitive closure they imported:
- Java: https://github.com/bazelbuild/rules_jvm_external/blob/master/examples/bzlmod/java/src/com/github/rules_jvm_external/examples/bzlmod/BUILD#L7
- JavaScript: https://github.com/aspect-build/rules_js/blob/main/e2e/bzlmod/BUILD.bazel#L4
- Python: https://github.com/bazelbuild/rules_python/blob/main/examples/bzlmod/BUILD.bazel#L18
This became really evident as I have started delivering a Bazel training course which is exclusively bzlmod. https://github.com/aspect-build/codelabs/tree/final is the final state of the codelab which illustrates how languages are used "kinda the same" but Go is the outlier right now, as the use_repo
calls in the MODULE.bazel file illustrate.
grep their codebase for the string that actually shows up in their BUILD file
doesn't this new option improve the situation, now that the string in the BUILD file is the same as the one in theimport
statement in the.go
file?
There's a whole separate discussion of whether we'd want Gazelle to support this form, but I think as written this PR is just adding a syntax sugar convenience and not tying us to any particular way forward.
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.
Sorry @achew22 for not providing the context right away, but @alexeagle did a great job explaining it.
Let me add that I can see #1404 being the right approach for the "do everything in Bazel" camp and this PR to please the "stay as close as possible to the native tools" demographic. They each have their own pros and cons and thus may have (but also can) coexist.
One nice feature I'm currently implementing for the go
macro is strict deps: If you depend on a Go package that is missing from your own module's go.mod
but contained in some Bazel dependency's go.mod
, you would see a descriptive error rather than risking having your build broken by changes in dependencies. As a result, the reliability afforded by the macro should be on par with the use_repo
approach.
No Go module in go.mod provides package '{import_path}'. | ||
Add the missing dependency via: | ||
|
||
go get {import_path} |
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.
I wonder if it's better to say bazel run @go_sdk//:bin/go get ...
- many users will prefer to have the same version, or maybe not rely on their team even installing the Go tooling.
- Add `.bcr` folder with requisite files. - Generate hub repository to aid in the resolution of the Bazel binary repositories. - Add `workspace_bazel_binaries` rule to allow repositories that use `rules_bazel_integration_test` to work with and without bzlmod enabled. Inspired by bazel-contrib/bazel-gazelle#1423. Related to #125.
…rib#128) - Add `.bcr` folder with requisite files. - Generate hub repository to aid in the resolution of the Bazel binary repositories. - Add `workspace_bazel_binaries` rule to allow repositories that use `rules_bazel_integration_test` to work with and without bzlmod enabled. Inspired by bazel-contrib/bazel-gazelle#1423. Related to bazel-contrib#125.
What type of PR is this?
Feature
What package or component does this PR mostly affect?
bzlmod
What does this PR do? Why is it needed?
With the new
go
helper macro provided by thego_deps
module extension, users can optionally specify their external Go dependencies indeps
lists using their Go import paths rather than Bazel labels.For example, to depend on
github.com/stretchr/testify/require
, users can add either of the following to thedeps
of ago_*
rule after loading thego
macro from@go_deps//:def.bzl
:"@com_github_stretchr_testify//require"
(old)go("github.com/stretchr/testify/require")
(new)When users specify an import path that isn't provided by a
go_repository
, they are shown the Go command that fetches the missing dependency (assuming they are already usinggo_deps.from_file
).Other notes for review
This commit does not add support for this new deps format to Gazelle itself.