Fixup importpath #1209
Fixup importpath #1209
Conversation
Infer test import path correctly from deps Remove importpath from all go_binary and go_test rules Add importpath to all go_library rules Delete go_prefix occurences Progress on bazel-contrib#721
go/private/context.bzl
Outdated
| @@ -121,14 +121,30 @@ def _infer_importpath(ctx): | |||
| VENDOR_PREFIX = "/vendor/" | |||
| # Check if import path was explicitly set | |||
| path = getattr(ctx.attr, "importpath", "") | |||
| # are we in forced infer mode? | |||
| if path == "~auto~": | |||
go/private/context.bzl
Outdated
| return embed[GoLibrary].importpath, EXPLICIT_PATH | ||
| # If we are a test, and we have a dep in the same package, presume | ||
| # we should be named the same with an _test suffix | ||
| if ctx.label.name.endswith("_test~library~"): |
There was a problem hiding this comment.
This seems brittle. What if the name passed to the go_test wrapper doesn't end with _test? What if the test doesn't have any dependencies in the same package (after go_prefix is removed)?
Since this package wouldn't be imported by anything other than the generated test main, can we just give it some constant import path?
Also, I think the string "~library~" should be extracted into a global, since it's used across multiple files.
There was a problem hiding this comment.
Test rules are required (by bazel) to end in _test
Tests always either have a dependency on the package under test, or they have an embed of the package under test. If neither of those is true, the import path does not matter anyway, but will be inferred from the directory structure. Inferring a correct import path when we can is a good idea because it helps things that attempt to recover a gopath structure (like the vet rule) but it's really just a best effort thing.
There was a problem hiding this comment.
Test rules are required (by bazel) to end in _test
I think that's true for the rule kind (we couldn't call go_test something else), but not for the rules themselves.
If neither of those is true, the import path does not matter anyway, but will be inferred from the directory structure. Inferring a correct import path when we can is a good idea because it helps things that attempt to recover a gopath structure (like the vet rule) but it's really just a best effort thing.
So should we allow importpath to be optional then as long as go_path is supported (rather than deprecating)? It doesn't seem like we can always infer it from the directory structure (especially for go_binary), so we should allow it to be specified explicitly in cases where it's needed.
There was a problem hiding this comment.
I could take out the _test part, just trying to be as specific as possible. If there was a way to check the rule kind I would rather do that, but ctx does not seem to expose that information.
I will do it in a follow up rather than wait for CI again...
I have plans for longer term support of gopath, but I need to see how things pan out. buildv2 will remove most of the need, and then we either add an optional exportpath to rules to allow an override, we add export rules that don't build but are only used during export, or we add a package remap rule to the gopath logic.
Infer test import path correctly from deps
Remove importpath from all go_binary and go_test rules
Add importpath to all go_library rules
Delete go_prefix occurences
Progress on #721