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.

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

incompatible_generated_protos_in_virtual_imports: put generated `.proto` files into a virtual proto source root directory #9215

Closed
lberki opened this issue Aug 21, 2019 · 13 comments

Comments

@lberki
Copy link
Contributor

@lberki lberki commented Aug 21, 2019

If a proto_library rule has both generated and non-generated sources, it would give rise to multiple source roots (one in the source tree and one in the output tree). This is unintuitive and incompatible with the ProtoInfo provider that has place for only one ([ProtoInfo].proto_source_root).

To resolve this, we create a virtual source root under bazel-bin/<package>/_virtual_imports/<rule>.

No user-visible changes are expected except in cases where Starlark code relied on the exact layout of the source tree.

@lberki lberki self-assigned this Aug 21, 2019
bazel-io pushed a commit that referenced this issue Aug 21, 2019
…or generated .proto files.

It was meant to be a compatible change, but alas, people seemed to depend on
the exact layout of the output tree. Will flip in 1.0 . Tracking bug:

#9215

RELNOTES: None.
PiperOrigin-RevId: 264581654
@dslomov dslomov changed the title `incompatible_generated_protos_in_virtual_imports`: put generated `.proto` files into a virtual proto source root directory incompatible_generated_protos_in_virtual_imports: put generated `.proto` files into a virtual proto source root directory Aug 21, 2019
katre added a commit that referenced this issue Aug 21, 2019
…or generated .proto files.

It was meant to be a compatible change, but alas, people seemed to depend on
the exact layout of the output tree. Will flip in 1.0 . Tracking bug:

#9215

RELNOTES: None.
PiperOrigin-RevId: 264581654
@thomasvl

This comment has been minimized.

Copy link
Member

@thomasvl thomasvl commented Aug 22, 2019

Can you maybe give an example of what does/doesn't change because of this so one can better assess the impact on LANG_proto_library rules?

@lberki

This comment has been minimized.

Copy link
Contributor Author

@lberki lberki commented Aug 23, 2019

Consider the following code say, in x/BUILD:

genrule(name="gen", srcs=[], outs=["a.proto"])
proto_library(name="p", srcs=["a.proto", "b.proto"])

At HEAD, this results in the proto_library having a proto_source_root of x. However, a.proto and b.proto are not actually siblings (one is under x, the other, under bazel-bin/x, thus, logic is required to figure out what command line to pass to protoc.

When this flag is flipped, the two .proto files are both symlinked into a directory bazel-bin/x/virtual_imports/p) and that's what proto_source_root will be. Simpler and removes guesswork for LANG_proto_library rules and makes it so that each proto_library only has one source root.

@thomasvl

This comment has been minimized.

Copy link
Member

@thomasvl thomasvl commented Aug 23, 2019

If we expand that with something depending on it, how does it end up?

# x/BUILD:
genrule(name="gen", srcs=[], outs=["a.proto"])
proto_library(name="p", srcs=["a.proto", "b.proto"])

# in m/n/o/BUILD:
proto_library(name="foo", srcs=["foo.proto"])

# in x/y/BUILD:
proto_library(name="bar", srcs=["bar.proto"], deps=["//x:p", "//m/n/o:foo"])

Assuming the generated b.proto has import x/a.proto; and bar.proto will have import "x/b.proto" and import "m/n/o/foo.proto".

What will be in the providers and the file system so we can build the protoc command line correctly? Do we hard code the use of virtual_imports like https://github.com/bazelbuild/rules_swift/pull/288/files does (which says it has a leading underscore unlike what is said here)?

@lberki

This comment has been minimized.

Copy link
Contributor Author

@lberki lberki commented Aug 23, 2019

Sorry, I meant /_virtual_imports/ (including the leading underscore)

I wanted to provide enough information in providers so that you can construct the command line without assuming things about paths, or, even better, provide a Starlark method to do so, but I couldn't make it in time for Bazel 1.0. So I decided that I'll be satisfied with that being possible without backwards-incompatible changes.

Hard-coding _virtual_imports is fine -- that won't change after Bazel 1.0.

In the above case, it depends on whether foo.proto is generated or not. If it is not, the proto path of the proto compile action of :bar will be . and bazel-bin/x/_virtual_imports/p. If it is, in addition to that, bazel-bin/m/n/o/_virtual_imports/foo will also be there. IOW, each proto_library that has import_prefix, strip_import_prefix or generated sources creates its own _virtual_imports directory which contains the .proto files in its srcs.

These are available as [ProtoInfo].transitive_proto_path. What is not available, however, is the information that which file in [ProtoInfo].transitive_sources is under which entry in [ProtoInfo].transitive_proto_path. In addition to time constraints, it's kinda tricky to come up with a data structure that doesn't require significant changes to ProtoInfo, does not cause a significant increase in memory use in large builds with many proto_library rules and is also reasonably simple to use. FWIW, Bazel also uses guesswork:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java#L680

, but don't replicate that logic yet -- once 0.29 is out, I'll hardwire alwaysAddRepositoryPath to false and some part of the logic only applies to our Google-internal rule which does not use _virtual_imports.

@thomasvl

This comment has been minimized.

Copy link
Member

@thomasvl thomasvl commented Aug 23, 2019

Assuming b.proto is the only generated file, the three proto_library usages should then all vend in ProtoInfo.proto_source_root from the libraries would have bazel-bin/x/_virtual_imports/p, ., and . respectively? And we can get the relative path for each proto file by trimming off their respective roots (if they aren't .)?

@lberki

This comment has been minimized.

Copy link
Contributor Author

@lberki lberki commented Aug 26, 2019

Yep. For the time being, that's the best option.

@thomasvl

This comment has been minimized.

Copy link
Member

@thomasvl thomasvl commented Aug 27, 2019

Actually, how does this all work with a cross repo reference?

Say x/BUILD was actually a remote reference (http_archive importing dep). If we are trimming off the _virtual_imports to get the relative paths to pass to protoc (along with the -I directives using the proto_source_root values), doesn't that mean that we merge the namespaces of proto file names and you could have conflicts between the main workspaces and the others?

@lberki

This comment has been minimized.

Copy link
Contributor Author

@lberki lberki commented Aug 27, 2019

You mean that the import paths of @a//x:y.proto and @b//x:y.proto are the same?

Yes indeed, that is the case, but that's no different from Bazel 0.28 or any version before that (or the logic of C++ include scanning): repository roots have always been proto source roots.

@thomasvl

This comment has been minimized.

Copy link
Member

@thomasvl thomasvl commented Aug 27, 2019

Not that they are the same, but that the dep on //x:p would actually be @something//x:p.

@lberki

This comment has been minimized.

Copy link
Contributor Author

@lberki lberki commented Aug 29, 2019

Fix for the only failing project (rules_swift) is bazelbuild/rules_swift#298

@dslomov

This comment has been minimized.

Copy link
Contributor

@dslomov dslomov commented Sep 2, 2019

What is the ETA for this?

@bazel-io bazel-io closed this in 5bc2030 Sep 2, 2019
@lberki

This comment has been minimized.

Copy link
Contributor Author

@lberki lberki commented Sep 2, 2019

Done. I thought I had submitted the fix, but apparently it was just a green presubmit run.

@ignasl

This comment has been minimized.

Copy link

@ignasl ignasl commented Sep 17, 2019

It seems like this PR may have broken rules_scala in a subtle way. The proto files are getting copied to the repository directory structure that is being built. For example in rules_scala test protos are getting copied here test_expect_failure/proto_source_root/dependency/_virtual_imports/. See: bazelbuild/rules_scala#843.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.