java_grpc_library.bzl: Work with proto_library rules using strip_import_prefix / import_prefix #5959
Conversation
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
…rt_prefix / import_prefix In addition to this welcome functionality, this change also makes it possible to fix http://github.com/bazelbuild/bazel/issues/7157 by tolerating proto_library rules using a virtual import directory.
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
|
@lberki, CLA bot is unhappy. To retain authorship it should be as easy as clicking the link the CLA bot posted, then "Sign up to contribute to this project as an employee" and then "Log in with Google". If that is annoying for you, I can be marked as the author. |
| return f.path[f.path.find(f.owner.workspace_root) + len(f.owner.workspace_root) + 1:] | ||
|
|
||
| # Bazel creates a _virtual_imports directory in case the .proto source files | ||
| # need to a accessed at a path that's different from their source path: |
|
Argh, PEBKAC. I had actually signed up for a Linux Foundation ID but forgot to connect my GitHub account. Do I need to do something special to make the CLA bot run its check again? |
|
Apparently not, clabot is now happy. |
| @@ -62,7 +62,20 @@ java_rpc_toolchain = rule( | |||
| def _path_ignoring_repository(f): | |||
| if len(f.owner.workspace_root) == 0: | |||
There was a problem hiding this comment.
This should probably be nested in the else clause or *import_prefix will not work for non-external protos
There was a problem hiding this comment.
Hmm... you're probably right that something is broken there, as it had only been tested when a single workspace was in place.
Am I right in thinking that this PR doesn't break anything? It mainly doesn't work in a case? If so, I think I may merge this and then do a follow-up (I'm not 100% confident I understand the change you're suggesting, so it may also be a good change for you to do). This version of the patch started in bazelbuild/bazel#8814 and has gone in internally and in bazelbuild/bazel-buildfarm#272 . Those other places are probably fine with the limitation, but it'd be a bit clearer to track as a separate commit.
There was a problem hiding this comment.
The change shouldn't break anything and the problem is fixable.
Is it actually used somewhere with the *import_prefix arg set?
Edit:
The problem is that for a proto file where owner.workspace_root == <empty-string> (any file not in another workspace) the first if clause is immediately triggered.
For proto_library rules with the *import_prefix arg set this then becomes <path-to-BUILD-file>/_virtual_imports/<rule-name>/<stripped-path> while it should be <stripped-path>.
(I am only now discovering _virtual_imports so I could be wrong in this)
There was a problem hiding this comment.
It doesn't break anything that I know of (that is to say, anything in the Bazel presubmit :) )
That said, you have a point -- however, since this change is already submitted within Google, how about merging this and creating a followup change with the suggested grammar fix and moving this check after the _virtual_imports one?
|
@lberki, "make a comment" has the CLA bot re-check, so your question is what triggered it 😄. |
Would love to avoid calling protoc directly! At the moment I have to maintain 5 proto related rules all sort of doing the same thing |
|
Merging as-is. There is a follow-up already en route (should be a PR soon). |
In addition to this welcome functionality, this change also makes it
possible to fix http://github.com/bazelbuild/bazel/issues/7157 by
tolerating proto_library rules using a virtual import directory.
CC @lberki @mjduijn
This is an export of cl/256951455.
@mjduijn, you may be interested to test with this. Medium/long-term, @lberki mentioned that proto_common will be receiving a function for us to use to avoid calling protoc directly.