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.

Sign up for GitHub

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

java_grpc_library.bzl: Work with proto_library rules using strip_import_prefix / import_prefix #5959

Merged
merged 1 commit into from
Jul 9, 2019
Merged

java_grpc_library.bzl: Work with proto_library rules using strip_import_prefix / import_prefix #5959

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion java_grpc_library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,20 @@ java_rpc_toolchain = rule(
def _path_ignoring_repository(f):
if len(f.owner.workspace_root) == 0:
Copy link

@mjduijn mjduijn Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This should probably be nested in the else clause or *import_prefix will not work for non-external protos

Copy link
Member Author

Choose a reason for hiding this comment

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

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 buildfarm/buildfarm#272 . Those other places are probably fine with the limitation, but it'd be a bit clearer to track as a separate commit.

Copy link

@mjduijn mjduijn Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

return f.short_path
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:
Copy link
Member

Choose a reason for hiding this comment

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

fix grammar

# https://github.com/bazelbuild/bazel/blob/0.27.1/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java#L289
#
# In that case, the import path of the .proto file is the path relative to
# the virtual imports directory of the rule in question.
virtual_imports = "/_virtual_imports/"
if virtual_imports in f.path:
return f.path.split(virtual_imports)[1].split("/", 1)[1]
else:
# If |f| is a generated file, it will have "bazel-out/*/genfiles" prefix
# before "external/workspace", so we need to add the starting index of "external/workspace"
return f.path[f.path.find(f.owner.workspace_root) + len(f.owner.workspace_root) + 1:]

def _java_rpc_library_impl(ctx):
if len(ctx.attr.srcs) != 1:
Expand Down