intellij icon indicating copy to clipboard operation
intellij copied to clipboard

Support embedded go_proto_library

Open tingilee opened this issue 1 year ago • 11 comments

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"

tingilee avatar Oct 30 '23 19:10 tingilee

Hi! Thanks @tingilee for your contribution. Could you please write a test for this?

agluszak avatar Nov 06 '23 11:11 agluszak

Hi! Thanks @tingilee for your contribution. Could you please write a test for this?

Added tests and also include dependencies needed in the WORKSPACE.

tingilee avatar Nov 09 '23 07:11 tingilee

LGTM, but there's a conflict.

@tpasternak want to have a final look? @mai93 what's the policy for introducing new dependencies to WORKSPACE?

agluszak avatar Nov 10 '23 14:11 agluszak

@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 avatar Nov 13 '23 14:11 tpasternak

@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 avatar Nov 14 '23 01:11 tingilee

@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.

blorente avatar Jan 29 '24 14:01 blorente

@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.

blorente avatar Feb 05 '24 10:02 blorente

@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?

tpasternak avatar Feb 05 '24 11:02 tpasternak

@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 avatar Feb 06 '24 09:02 blorente

@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 avatar Feb 06 '24 10:02 ramilmsh

@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.

blorente avatar Feb 12 '24 10:02 blorente

Closing since https://github.com/bazelbuild/intellij/pull/6030 merged.

blorente avatar Mar 19 '24 17:03 blorente