hack for proto in case of many-repo with relative package #628
Conversation
natansil
changed the title
scala_proto/scala_proto.bzl
Outdated
| @@ -331,14 +331,28 @@ def scala_proto_repositories( | |||
| name = 'io_bazel_rules_scala/dependency/proto/netty_handler_proxy', | |||
| actual = '@scala_proto_rules_netty_handler_proxy//jar') | |||
|
|
|||
| def _root_path(f): | |||
| def _root_path(f, transitive_proto_paths): | |||
| # print("f.is_source is {}".format(f.is_source)) | |||
scala_proto/scala_proto.bzl
Outdated
| package_len = 0 | ||
| for package_set in transitive_proto_paths: | ||
| for package in package_set.to_list(): | ||
| if str(package) in f.path: |
There was a problem hiding this comment.
Is it in instead of starts_with so that the external/<REPO> part at the beginning is ignored? If that's the case, prefer explicitly checking for that prefix because otherwise you'll run into all sorts of problems where package is randomly in the middle of the path of the .proto file.
There was a problem hiding this comment.
ok, I'll do something like:
| if str(package) in f.path: | |
| if f.path.startswith(f.owner.workspace_root + "/" + str(package)): |
| val file = if (root.isEmpty) { | ||
| parsed(1) | ||
| } else { | ||
| parsed(1).substring(root.length + 1) |
There was a problem hiding this comment.
This is literally the first time I'm reading Scala, but doesn't this assume that root is at the beginning of parsed(1), that is, the path to the .proto file? If so, how can this work with files under external/ ?
There was a problem hiding this comment.
Note this line is under condition where there is no external in root...
scala_proto/scala_proto.bzl
Outdated
| return ':'.join([ | ||
| "{root},{path}".format(root = _root_path(f), path = f.path) | ||
| "{root},{path}".format(root = _root_path(f, transitive_proto_paths), path = f.path) |
There was a problem hiding this comment.
In general, it's kind of odd that the code is separated between Starlark and Scala. Can you make it so that the whole thing is in one place (preferably, Starlark?)
There was a problem hiding this comment.
In rules_scala this pattern of mixed scala and Starlark is quite common...
| parsed(1) | ||
| } else { | ||
| parsed(1).substring(root.length + 1) | ||
| if (parsed(1).contains("external")) { |
There was a problem hiding this comment.
@ittaiz, I've found the bug manifested in OriP's branch. This line had the error.
changing it to startsWith:
| if (parsed(1).contains("external")) { | |
| if (parsed(1).startsWith("external")) { |
fixed it.
next step is to add E2E test.
should we do this in Wix's fork or on bazelbuild?
natansil
force-pushed
the
hack_proto_many_repo
branch
from
9c6aaf0
to
77f9525
Compare
natansil
changed the title
|
@ittaiz please review |
|
@natansil can we close this? |
This hack solves an issue with proto_library target depending on proto_library from external repo.
scalapb_proto_libraryfailed to find the proto from the external repo.This hack can be reverted when bazelbuild/bazel#4665 is resolved.