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 ***@***.***> 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 <#4544 (comment)>, 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 #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 ***@***.***> 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 <#4544 (comment)>, 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 ***@***.***> 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 <#4544 (comment)>, 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á ***@***.***> 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 <#4544 (comment)>, 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).

[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 ***@***.***> 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 <#4544 (comment)>, 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 ***@***.***> 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 <#4544 (comment)>, 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 #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]

e895664 should fix the strict deps issue.

[lberki]

strip_import_prefix = <package name> is the same thing as proto_source_root = <package name>. Does this answer your question?

…

On Tue, Jan 22, 2019 at 10:10 AM Shuai Zhang ***@***.***> 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 <#4544 (comment)>, 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.