intellij
intellij copied to clipboard
Support embedded go_proto_library
Checklist
- [x] I have filed an issue about this change and discussed potential changes with the maintainers.
- [x] I have received the approval from the maintainers to make this change.
- [x] This is not a stylistic, refactoring, or cleanup change.
Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See the Contributions section in the README for more details.
Discussion thread for this change
Issue number: https://github.com/bazelbuild/intellij/issues/5566
Description of this change
Support embedded go_proto_library as source files in the same Go package.
With this change, proto refers to the pb.go as source files which is what we intend to do:
cat bazel-bin/proto/proto-106940904.intellij-info.txt
build_file_artifact_location {
is_external: false
is_source: true
relative_path: "proto/BUILD.bazel"
root_execution_path_fragment: ""
}
deps {
dependency_type: 0
target {
label: "//proto:fooproto_go_proto"
}
}
go_ide_info {
import_path: "github.com/bazelbuild/intellij/examples/go/with_proto/proto"
sources {
is_external: false
is_source: true
relative_path: "proto/translators.go"
root_execution_path_fragment: ""
}
sources {
is_external: false
is_source: false
relative_path: "proto/fooproto_go_proto_/github.com/bazelbuild/intellij/examples/go/with_proto/proto/fooserver.pb.go"
root_execution_path_fragment: "bazel-out/darwin_arm64-fastbuild/bin"
}
}
key {
label: "//proto:proto"
}
kind_string: "go_library"
Hi! Thanks @tingilee for your contribution. Could you please write a test for this?
Hi! Thanks @tingilee for your contribution. Could you please write a test for this?
Added tests and also include dependencies needed in the WORKSPACE.
LGTM, but there's a conflict.
@tpasternak want to have a final look? @mai93 what's the policy for introducing new dependencies to WORKSPACE?
@tpasternak want to have a final look? So I just don't fully understand why it is required to add all the dependencies to the WORKSPACE file. How do we use them?
@tpasternak
So I just don't fully understand why it is required to add all the dependencies to the WORKSPACE file. How do we use them?
These dependencies where added similarly to https://github.com/bazelbuild/intellij/blob/f49aaf253d0bd3a8a03eb35e32b0d2bb4a98e0af/WORKSPACE#L865-L881. Previously, there was no tests for Bazel + Go integration in IntelliJ. To address the PR comments, I've added tests for Bazel + Go integrations, which means we need the golang dependencies, specifically all of the dependencies to support aspect/testing/tests/src/com/google/idea/blaze/aspect/go/go_proto_library/BUILD
@tingilee I've rebased this PR and addressed the comments on this branch: https://github.com/blorente/intellij/tree/blorente/address-comments. Should be ready to merge.
If you're okay with it, feel free to rebase this PR on top of mine. Or I can just merge my version if you're busy, whatever works. This functionality would be very useful for us.
@tpasternak @mai93 , would you be okay if I open a PR with the cleanups and we merge this? Some of our users really liked the feature, and it seems generally useful.
@tpasternak @mai93 , would you be okay if I open a PR with the cleanups and we merge this? Some of our users really liked the feature, and it seems generally useful.
Why would I have anything against it? 😅 Are there any disadvantages?
@tingilee I opened a PR with the cleanup here: https://github.com/bazelbuild/intellij/pull/6030 Please let us know in a couple of days if you'd like to incorporate the changes to yours, otherwise we'll merge it.
@blorente Hi!
I've tried out our plugin version and unfortunately it doesn't seem to solve
https://github.com/bazelbuild/intellij/issues/5538
Specifically, If an embedded proto library depends on an embedded proto library, resolution still fails. I have not yet had the time to create a more complex use-case in my demo repo, which I will try to do this week, but I'd be happy to demonstrate the problem. I'm also not against contributing a fix myself, but I have no idea how intellij works and how the plugin is structured, so it would take me really-really long to do so.
@ramilmsh I've tried to reproduce the case you mention in https://github.com/blorente/intellij/blob/blorente/repro-oss-issue-with-proto-in-proto/examples/go/with_proto/proto/BUILD.bazel, and haven't been able to. Since this PR, though it may be incomplete, is useful on its own, I'd like to merge it soon. Let's hop over to the issue you opened to discuss if it's missing some features, and how to implement them.
Closing since https://github.com/bazelbuild/intellij/pull/6030 merged.