Skip to content
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

fix: plugin_output in py_proto_library rule #1280

Merged
merged 2 commits into from
Jun 20, 2023

Conversation

comius
Copy link
Contributor

@comius comius commented Jun 20, 2023

plugin_output was wrong in case multiple repositories are involved and/or _virtual_imports.

The code is taken from cc_proto_library and has been verified in practice.

@comius comius requested a review from rickeylev as a code owner June 20, 2023 16:43
Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue somewhere describing the bug in more detail? Or did you just come across this accidentally?

Otherwise lgtm

python/private/proto/py_proto_library.bzl Show resolved Hide resolved
@comius
Copy link
Contributor Author

comius commented Jun 20, 2023

Is there an issue somewhere describing the bug in more detail? Or did you just come across this accidentally?

Otherwise lgtm

Accidentally. We had the same problem internally, reported on b/277335175.

And I spent a month fixing _virtual_imports and otherwise generally cleaning up/optimising proto_library implementation.

@chrislovecnm chrislovecnm added this pull request to the merge queue Jun 20, 2023
Merged via the queue into bazelbuild:main with commit 1a333ce Jun 20, 2023
eed3si9n pushed a commit to eed3si9n/rules_python that referenced this pull request Aug 24, 2023
plugin_output was wrong in case multiple repositories are involved
and/or _virtual_imports.

The code is taken from `cc_proto_library` and has been verified in
practice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants