Bazel: Allow to set source root for proto_library

[dslomov]

proto_library forces import path to start at workspace root. This means that, for example, the following setup (coming from a customer) is not supported:

Their current setup (unfortunately) has very coarse granularity of the entire source module (my-module/src/main/proto/**).
given:
my-module
src/main/proto/com/wix/
foo.proto
bar.proto
And given foo.proto needs to import bar.proto then currently in maven it imports "com/wix/bar.proto".
In Bazel it needs to import "my-module/src/main/proto/com/wix/bar.proto".

We should support one or both of

1. proto_source_root attribute on proto_library rule
2. making a package path to a proto_library an import root

Maven protobuf plugin has a protoSourceRoot parameter to specify this.

[dslomov]

/cc @iirina @cgrushko @lberki

[ittaiz]

Thanks!
Option 2 would be really helpful for us though we can also live with option
1
On Tue, 30 Jan 2018 at 16:49 Dmitry Lomov [e-mail] wrote:

/cc @iirina https://github.com/iirina @cgrushko
https://github.com/cgrushko @lberki https://github.com/lberki

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-361615739,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF3qx5EelM0RvYGneR-YcAql38aVlks5tPyvfgaJpZM4Rycpg
.

[lberki]

Adding multiple include roots is not a happy thing to do, since e.g. if you have the proto files

q/x/y.proto
q/a/b.proto

and the proto_library rule with the first proto adds an include root q, the second proto will be accessible as a/b.proto in addition to q/a/b.proto. Unfortunately, it appears that protoc doesn't allow more granular control than adding include roots, so I'd much rather go with something that does not add an include root for each proto_library rule.

How about doing (1), but limiting it such that it only allows the current package name, at least for now?

@dslomov : why is this a P1?

[ittaiz]

@lberki

I'd much rather go with something that does not add an include root for each proto_library rule.

it sounds to me you're suggesting the 1 and 2 version which actually sounds to me the worst option since you don't gain freedom but still have the receptiveness.
Are you trying to add an opt-in phase?
If so we can add a cmd-line flag or rule flag to turn on option 2's behavior.

[jmillikin-stripe]

See also https://github.com/bazelbuild/bazel/issues/3867, which suggests adding attributes similar to cc_library's import_prefix and strip_import_prefix.

A single attribute that sets the path to a fixed value would be simpler, but verbose+redundant when working with large numbers of protos under a common prefix.

[lberki]

Yeah, it would be nice to have some sort of consistency with the C++ rules and their import_prefix and strip_import_prefix attributes. They are an argument that doing the same with proto_library will not be that bad.

@ittaiz : what's "receptiveness"? I can't parse that sentence.

re: command line option, I think it's an option if we want to be reaaally careful, but I think the C++ rules are good argument that my worries about rules having effect on the import path of other rules are somewhat over-blown, so I was hoping we can get by without it.

[lberki]

(downgrading this to P2 for lack of data that would support this being P1)

[ittaiz]

I meant repetitive, sorry

[lberki]

Adding back the P1 label after in person discussion with @dslomov ; I wasn't aware that this is a blocker for you.

[ittaiz]

Thanks!
On Thu, 1 Feb 2018 at 16:46 lberki [e-mail] wrote:

Adding back the P1 label after in person discussion with @dslomov
https://github.com/dslomov ; I wasn't aware that this is a blocker for
you.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-362287178,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF29YXwNUT23aDljxe-XUpRv3G9Fmks5tQc5KgaJpZM4Rycpg
.

[thomasvl]

Drive by -

Seems like this will ripple though every [lang]_proto_library implementation since the relative paths change, they will need to insure whatever the language has in the way of search paths also tracks this differences in how things are generated.

If protoc finds something via q/a/b.proto and a/b.proto doesn't it error because of the collision on the package.type name and those have to be unique? Even if protoc were updated to realize they define the exact same things, it seems like you'd could be headed for multiple definitions as the proto_library chains gets followed with everything built/linked?

[jmillikin-stripe]

This won't impact the type names of protobuf messages, only the filepath-looking strings used in import statements.

[thomasvl]

This won't impact the type names of protobuf messages, only the filepath-looking strings used in import statements.

Right, but protoc makes sure all the types defined during a generation run are unique. So if the same files is loaded via two paths, it will appear to be a collision on defining those types.

[ittaiz]

I might be missing something but why is that a big issue? People should use
one form or another (at least for the same file)
On Thu, 1 Feb 2018 at 22:38 Thomas Van Lenten [e-mail]
wrote:

This won't impact the type names of protobuf messages, only the
filepath-looking strings used in import statements.

Right, but protoc makes sure all the types defined during a generation run
are unique. So if the same files is loaded via two paths, it will appear to
be a collision on defining those types.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-362394348,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIFy64mbjLZNv8WPnNIQ5GC_53Picgks5tQiCzgaJpZM4Rycpg
.

[sergiocampama]

one the issues is that protobuf might not be expecting this use case, which means that even if proto_library supports it, the underlying proto compiler might not, so you won't be able to use it. another issue is that import statements may be used by certain lang generators, like c++ or objc, which means that some expectations are going to change, so they also need to be vetted before rolling a feature like this.

I'm not aware of the details of the project, but could the original problem be worked around by creating a new workspace by adding a WORKSPACE file in my-module/src/main/proto? That should create a new root for importing in proto_library. again, this might not be feasible, but maybe worth looking into?

[ittaiz]

This isn’t feasible because of the accounting that’s needed between all of
the workspaces.
We’re talking about many such locations so that’s not really an option.
Re protoc I already hacked together a change to rules_scala to have that
use-I to also pass in the files locations as if they were from the package
and it didn’t have a problem.
On Fri, 2 Feb 2018 at 6:33 Sergio Campamá [e-mail] wrote:

one the issues is that protobuf might not be expecting this use case,
which means that even if proto_library supports it, the underlying proto
compiler might not, so you won't be able to use it. another issue is that
import statements may be used by certain lang generators, like c++ or objc,
which means that some expectations are going to change, so they also need
to be vetted before rolling a feature like this.

I'm not aware of the details of the project, but could the original
problem be worked around by creating a new workspace by adding a WORKSPACE
file in my-module/src/main/proto? That should create a new root for
importing in proto_library. again, this might not be feasible, but maybe
worth looking into?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-362483765,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIFztBFYf1aPSijbxKnnJBjkk98Ve2ks5tQpAcgaJpZM4Rycpg
.

[thomasvl]

Say you have:

foo/
  BUILD - defines proto_libray("foo")
  foo.proto - imports foo/bar/bar.proto
  bar/
    BUILD - defines proto_libray("bar")
    bar.proto - no imports

Now you add something new:

baz/
  BUILD - defines proto_library("baz")
  baz.proto - imports "foo.proto" and "bar.proto"

The imports on baz can be short because they use this new feature. bar.proto will be found as bar.proto for the direct import from baz and it will be found as foo/bar/bar.proto during the import of foo.proto. So all of those types will appear to be defined twice (the package.name is what defines the type). So generation would fail.

Yes, this case can be "fixed", but my concern is as a proto file evolves over time, a working setup could break; say when this was first done foo.proto didn't import bar so baz worked. But then when foo picks up the import on bar, even though foo builds, baz breaks. It seems like supporting this would make it easy for change to proto tree appear to work fine; but would actually break external files that depend on them because the external thing used import paths to pick up individual parts as so as the graph changes you get the conflicts since the paths aren't all the same.

[sergiocampama]

Another option, could maven's config for protos be modified to accept repo-relative paths for proto imports? not really sure how that works either

[ittaiz]

@thomasvl I think something got mixed up in the example.
You say

The imports on baz can be short because they use this new feature

but nothing imports baz.proto, am I correct?

I'm not versed enough in protobuf to say if you're correct or not.
If you are then maybe this should be opt-in via a flag or indeed via repetition (we'll probably hide it in a macro)

[thomasvl]

Typo on my part, I meant the imports _in_ baz could be short if the proto_library rule there use this new support.

[lberki]

@thomasvl : yes, I'm worried about that situation, but it seems that it's also a problem with Maven's protoSourceRoot, so we at least wouldn't be any worse, would it?

Ideally, protoc would disallow references by the "long" path if a short one exists., but absent that, what we could do in the long term is that instead of presenting the original source tree to protoc, we'd figure out which proto files there are, where they should be accessed at and then present an appropriate symlink tree. For example, in the above case, instead of having the input

foo/foo.proto
foo/bar/bar.proto
baz/baz.proto
--proto_path=.
--proto_path=foo
--proto_path=foo/bar

We'd have

1/foo.proto
2/bar.proto
3/baz.proto
--proto_path=1
--proto_path=2
--proto_path=3

, thus making it impossible for protoc to access files by their "wrong" path.

In the short term, we can just add the aforementioned options since they don't make the situation worse than Maven, do they?

[thomasvl]

The synlink tree would still have to use the real names, yes? Because the proto files them selves will have import directives with the original authors directory structure. Or are you talking about processing the files to rewrite those also?

Maven may have something like this now, but isn't that limited to to a single language then? Doing this means all of the [lang]_proto_library rules would have to under stand this not only for generation, but since the path the file is found by impacts generated #includes (or the languages version of that), they need do add -I[path] directives also to all the dependent compile actions. So it sounds like we're sorta saying just because Maven took on this, we're going to force it on all other languages and hope it works?

ps - one other potential complexity is the Well Known Types. Those proto files are bundled by most of the runtimes, but need to always be found have google/protobuf/[name].proto because that's how some of the generators might figure out they are the WKTs (I think some use the package directive, but it likely isn't consistent). descriptor.proto could be a trouble spot also because it isn't really a WKT, and only some of the runtimes ship it (others don't need it, so they don't include it).

[jmillikin-stripe]

Each .proto file would only have one path, right? Whether the final implementation sets the import path explicitly or uses the add/strip attributes, the resulting target would have only a single path associated with it. There should be no confusion between short and long paths, nor any requirement for symlinks.

[iirina]

I started work on this issue. I have no ETA yet, perhaps tomorrow I will be able to give more details.

I decided for @lberki's suggestion:

How about doing (1), but limiting it such that it only allows the current package name, at least for now?

which IMO is the least harmful.

[ittaiz]

@iirina did you see my comment?

it sounds to me you're suggesting the 1 and 2 version which actually sounds to me the worst option since you don't gain freedom but still have the receptiveness.
Are you trying to add an opt-in phase?
If so we can add a cmd-line flag or rule flag to turn on option 2's behavior.

I don't really understand why do it. Why not add a boolean flag? Or even a cmd-line boolean flag

[iirina]

Yes I saw it. I should have mentioned my I chose this option. My reasoning is:

1. In the future we could relax this limitation (current package name) if needed. If we add the flag as boolean from the beginning we tie our hands and future modifications/additions will be harder to make.
2. A command line flag would impose --proto_path=/path/to/proto/package for every proto_library and I think that is too powerful. If you want to override this behavior you need a flag at library level.
3. The current limitation conducts to more consistent proto files, thus less prone to errors.

[ittaiz]

ok. I meant a command-line flag only as a boolean one indeed.
@dslomov @iirina We can live with this but we will need to hide it with a macro which will set this property using %PACKAGE_NAME since we can't have everyone repeating this in tens of thousands of BUILD files. I know you're somewhat against macros so I wanted to surface it.

[iirina]

Thanks for letting us now. It's OK to use a macro for this situation.

since we can't have everyone repeating this in tens of thousands of BUILD files

Do you have that many BUILD files?

[ittaiz]

As I think you know we’re still moving so it’s hard to say but this is
where it seems it will go. Maybe very lower tens but yeah.
One correction is that we obviously will only have tens or lower hundreds
of places which use proto library so with respect to this specific dilemma
it’s much lower.
And still I can’t find any value in having developers specify it.
On Wed, 14 Feb 2018 at 18:44 Irina Iancu [e-mail] wrote:

Thanks for letting us now. It's OK to use a macro for this situation.

since we can't have everyone repeating this in tens of thousands of BUILD
files

Do you have that many BUILD files?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-365668461,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF50hb88D2DliwTIugiGkOeI7Zhuhks5tUw1xgaJpZM4Rycpg
.

[iirina]

As I think you know we’re still moving so it’s hard to say but this is
where it seems it will go. Maybe very lower tens but yeah.
One correction is that we obviously will only have tens or lower hundreds
of places which use proto library so with respect to this specific dilemma
it’s much lower.

I see. I was curious, I don't know much about the project.

And still I can’t find any value in having developers specify it.

I think the value comes from having more control about the proto libraries. Having a flag that affects every proto_library can become problematic very quickly. Maybe it works well for your setup, but for other projects it could be misused. The macros solve this problem nicely here - so from my point of view it's a win-win.

[ittaiz]

Indeed I meant that for our use-case I don’t find any value in it
On Wed, 14 Feb 2018 at 19:11 Irina Iancu [e-mail] wrote:

As I think you know we’re still moving so it’s hard to say but this is
where it seems it will go. Maybe very lower tens but yeah.
One correction is that we obviously will only have tens or lower hundreds
of places which use proto library so with respect to this specific dilemma
it’s much lower.

I see. I was curious, I don't know much about the project.

And still I can’t find any value in having developers specify it.

I think the value comes from having more control about the proto
libraries. Having a flag that affects every proto_library can become
problematic very quickly. Maybe it works well for your setup, but for other
projects it could be misused. The macros solve this problem nicely here -
so from my point of view it's a win-win.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-365677272,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF9lvRpOKoh8QAjdEKQgawKelich4ks5tUxOjgaJpZM4Rycpg
.

[iirina]

Update: the fix should be in this week.

[ittaiz]

This week is today or tomorrow? (Or this calendar week)
On Thu, 15 Feb 2018 at 15:48 Irina Iancu [e-mail] wrote:

Update: the fix should be in this week.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-365932133,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF3STs057VGGLxG9JHfAdTnX_FvrNks5tVDW6gaJpZM4Rycpg
.

[lberki]

(originally, today or tomorrow, but @iirina chose me as her code reviewer and I'm OOO tomorrow so it either has to be someone else or it'll slip till next week)

[ittaiz]

If there’s a chance it could get in tomorrow that would really help us. As
usual I trust you to make the right call.
On Thu, 15 Feb 2018 at 19:22 lberki [e-mail] wrote:

(originally, today or tomorrow, but @iirina https://github.com/iirina
chose me as her code reviewer and I'm OOO tomorrow so it either has to be
someone else or it'll slip till next week)

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-365999517,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF05TayL9t_qlnspe9MFa8Fo98Bmxks5tVGfjgaJpZM4Rycpg
.

[ittaiz]

I'm not sure this is resolved...
I tried to use it in the repro repo and using proto_source_root doesn't pass the build

[iirina]

Does the error look like this:

address.proto:9:19: "demo.Address.city" is already defined in file "src/address.proto".
address.proto:10:20: "demo.Address.zip_code" is already defined in file "src/address.proto".
address.proto:8:9: "demo.Address" is already defined in file "src/address.proto".
src/person.proto: Import "address.proto" was not found or had errors.
src/person.proto:12:12: "demo.Address" seems to be defined in "src/address.proto", which is not imported by "src/person.proto".  To use it here, please add the necessary import.

?

[iirina]

I identified 2 errors:

1. A strict proto deps bug. It looks like proto_library identifies (wrongly) the existence of a dependency and then realizes the "dependency" has no sources. The message produced by this error shoud look like this:

address.proto: zip_code.proto is imported, but //src:all doesn't directly depend on a proto_library that 'srcs' it.

A temporary workaround is using --strict_proto_deps=off. sigh

1. In Bazel the long paths are passed to protoc before the short paths and protoc will fail with an error similar to my previous comment. Easy fix is to first add the short paths and then the long paths. Long term fix should be to only add one or the other.

[ittaiz]

I think we can live with strict deps proto off for the meantime.
We’ll wait for the fix for #2
On Mon, 19 Feb 2018 at 11:44 Irina Iancu [e-mail] wrote:

I identified 2 errors:

1. A strict proto deps bug. It looks like proto_library identifies
   (wrongly) the existence of a dependency and then realizes the "dependency"
   has no sources. The message produced by this error shoud look like this:

address.proto: zip_code.proto is imported, but //src:all doesn't directly depend on a proto_library that 'srcs' it.

A temporary workaround is using --strict_proto_deps=off. sigh

1. In Bazel the long paths are passed to protoc before the short paths
   and protoc will fail with an error similar to my previous comment
   https://github.com/bazelbuild/bazel/issues/4544#issuecomment-366633751.
   Easy fix is to first add the short paths and then the long paths. Long term
   fix should be to only add one or the other.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-366636641,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF92-SvQHXbj0MFUXAIc7foSnnVOaks5tWUKUgaJpZM4Rycpg
.

[iirina]

I have a fix for both of the issues above. Preparing a change for review now. Hopefully it will be uploaded tomorrow.

[iirina]

@ittaiz 9c1feeb should fix 2. The strict deps issue should be in tomorrow.

[ittaiz]

@iirina thanks!
WDYT about exposing getTransitiveProtoPathFlags to skylark? I added it on my local bazel and it greatly simplified the song and dance I need in order to get to them.
Additionally WDYT about adding a test that protects this from regression and makes sure this continues to work? preferably one that shows this work transitively like the updated example in the repo I pasted above.

[iirina]

WDYT about exposing getTransitiveProtoPathFlags to skylark?

I don't see anything wrong with exposing this field to Skylark, especially since all the other fields in ProtoSourcesProvider are exposed.

Additionally WDYT about adding a test that protects this from regression and makes sure this continues to work? preferably one that shows this work transitively like the updated example in the repo I pasted above.

I did add tests internally that prevent regressions. There was no OS test setup for proto_library and I wanted to get the fix in quickly. I'm working now on open-sourcing the tests.

preferably one that shows this work transitively like the updated example in the repo I pasted above.

Have you tried the fix with the updated repo? I tested it on a similar setup and it worked.

[ittaiz]

I did try it, I’m sorry I wasn’t clear. It fixed it (apart from skylark)
and I really appreciate it!
I’m currently using a patched version of bazel which adds the
SkylarkCallable annotation and I was able to patch rules Scala to utilize
it as well.
On Tue, 20 Feb 2018 at 11:00 Irina Iancu [e-mail] wrote:

WDYT about exposing getTransitiveProtoPathFlags to skylark?

I don't see anything wrong with exposing this field to Skylark, especially
since all the other fields in ProtoSourcesProvider are exposed.

Additionally WDYT about adding a test that protects this from regression
and makes sure this continues to work? preferably one that shows this work
transitively like the updated example in the repo I pasted above.

I did add tests internally that prevent regressions. There was no OS test
setup for proto_library and I wanted to get the fix in quickly. I'm
working now on open-sourcing the tests.

preferably one that shows this work transitively like the updated example
in the repo I pasted above.

Have you tried the fix with the updated repo? I tested it on a similar
setup and it worked.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-366911240,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF8jIwLInc26x3gVJEI2lTscTOg3Rks5tWonHgaJpZM4Rycpg
.

[iirina]

@ittaiz The strict deps issue should be fixed now as well.

What remains to be done now is to make proto_source_root work within multiple workspaces. I'm not sure if this is of interest to you. If yes, you can follow #4665.

Exposing getTransitiveProtoPathFlags to Skylark is on its way.

[ittaiz]

Thanks!

4665 does interest me, thanks. It's not blocking us now but will block us in a few weeks (when we'll start using source dependencies between bazel repositories).

[ittaiz]

@iirina any updates on getTransitiveProtoPathFlags for skylark?

[iirina]

The strict deps fix was rolled back, I'll try to resubmit it tomorrow.

[ittaiz]

Any update about the strict deps reapply?
On Tue, 27 Feb 2018 at 18:48 Irina Iancu [e-mail] wrote:

The strict deps fix was rolled back, I'll try to resubmit it tomorrow.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-368944786,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIFzUNcfdMD7x1Ng24Qo4_ZPSvSSswks5tZDG7gaJpZM4Rycpg
.

[iirina]

I didn't find the culprit (yet) that made a test fail (why the change was rolled back). Since this is not critical, I'm not allocating a lot of time for the roll-forward.

[ittaiz]

I’d say this is very important though not a blocker. Having to turn off strict deps for proto doesn’t sound that wise to me

[jayconrod]

Is there any chance this can be fixed soon? It's a really high priority for rules_go. It's one of the biggest pain points for Kubernetes.

Kubernetes' proto import paths typically don't match their locations within repositories. For example, see k8s.io/apimachinery/pkg/apis/meta/v1/generated.proto. That imports other protos with paths like k8s.io/apimachinery/pkg/util/intstr/generated.proto. The k8s.io/apimachinery prefix refers to the repository itself. The actual path of the proto within the repository is pkg/util/intstr/generated.proto.

If we had something like cc_library include_prefix and strip_include_prefix or something at least as powerful, that would be extremely helpful in resolving a lot of issues.

[jmillikin-stripe]

@jayconrod that sounds like https://github.com/bazelbuild/bazel/issues/3867, I believe this issue is for a slightly different behavior (something about proto import aliases?).

[jayconrod]

@jmillikin-stripe You're right, #3867 is what I really want.

[ittaiz]

@iirina any news about the strict deps fix?

[helenalt]

@iirina @lberki do you have an update on this issue and what are the appropriate next steps?

[iirina]

e8956648d1c94a3a51e1aba5d229d1f27bdf8e35 should fix the strict deps issue.

[lberki]

strip_import_prefix = is the same thing as proto_source_root
= .

Does this answer your question?

On Tue, Jan 22, 2019 at 10:10 AM Shuai Zhang [e-mail]
wrote:

After a quick glance on this thread, I think both 1 & 2 are fixed.
However, may I ask your help figuring out how to strip the import prefix
now? I'm on bazel 0.21.0.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/4544#issuecomment-456323973,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADXI0s7KJlBCXT1ZKKT6M2kiWj7RIFI8ks5vFtV3gaJpZM4Rycpg
.

--
Lukács T. Berki | Software Engineer | [email protected] |

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany |
Geschäftsführer:
Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg,
HRB 86891

[hcoona]

Thank you, it works.