-
Notifications
You must be signed in to change notification settings - Fork 304
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
Support embedded go_proto_library #5567
Conversation
Hi! Thanks @tingilee for your contribution. Could you please write a test for this? |
951d739
to
115d9ba
Compare
sha256 = "825c30d2cbcce405fd18fddf356eb1f425607e9c780f8eff95d21ac23f8d90fd", | ||
url = "https://plugins.jetbrains.com/maven/com/jetbrains/plugins/PythonCore/231.8770.65/PythonCore-231.8770.65.zip", | ||
) | ||
|
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 rule seems redundant so deleting here. I had to delete this otherwise I get errors:
Error in fail: generate_repo_config: generate_repo_config: /workdir/WORKSPACE: multiple rules have the name "python_2023_1"
See CI build failure: https://buildkite.com/bazel/intellij-plugin-aspect/builds/20467
Here are the duplicated rules:
Lines 320 to 336 in ae7424b
http_archive( | |
name = "python_2023_1", | |
build_file_content = _PYTHON_CE_BUILD_FILE, | |
sha256 = PYTHON_PLUGIN_231_SHA, | |
url = PYTHON_PLUGIN_231_URL, | |
) | |
PYTHON_PLUGIN_232_URL = "https://plugins.jetbrains.com/maven/com/jetbrains/plugins/PythonCore/232.10203.2/PythonCore-232.10203.2.zip" | |
PYTHON_PLUGIN_232_SHA = "027bc9e2322f21cb3093b3254035cfeaac587552f9db2f664b28280f172acfed" | |
http_archive( | |
name = "python_2023_2", | |
build_file_content = _PYTHON_CE_BUILD_FILE, | |
sha256 = PYTHON_PLUGIN_232_SHA, | |
url = PYTHON_PLUGIN_232_URL, | |
) |
Added tests and also include dependencies needed in the WORKSPACE. |
LGTM, but there's a conflict. @tpasternak want to have a final look? |
4d776f2
to
f49aaf2
Compare
|
These dependencies where added similarly to Lines 865 to 881 in f49aaf2
|
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.
@tingilee I think what @tpasternak meant was that we don't reference those dependencies anywhere in the BUILD files, so it's unclear how they are needed for the tests. For instance, I'd expect to see @org_golang_x_net
somewhere in the test build file.
@@ -0,0 +1,4 @@ | |||
def setup_tests(): |
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'm not sure what this file is for, it doesn't seem to be referenced anywhere.
@@ -0,0 +1 @@ | |||
package simple |
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.
Same with this file, it doesn't seem to be referenced.
@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. |
Why would I have anything against it? 😅 Are there any disadvantages? |
@blorente Hi! I've tried out our plugin version and unfortunately it doesn't seem to solve 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. |
Closing since #6030 merged. |
Checklist
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: #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: