-
Notifications
You must be signed in to change notification settings - Fork 460
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
Provide an option to opt-out of compile_well_known_types #3228
base: main
Are you sure you want to change the base?
Provide an option to opt-out of compile_well_known_types #3228
Conversation
9247136
to
0f2ba75
Compare
@UebelAndre Would love to get a review on this 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me! But can you add some usage of this in the tests directory somewhere so it's regression tested?
2d316cb
to
32602aa
Compare
32602aa
to
6a71c58
Compare
@UebelAndre I added tests, which are basically the WKT tests but adapted to use prost_types. In order to allow configuring the I had originally built the test as its own Bazel workspace with its own toolchain, but this made the test setup much more complicated, and also brought more code into the repo. However, for the transition to work properly, I had to expose the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I got to this so late, looks like there's a conflict. When I went to resolve it though I had one question which I would love your insight on!
for prost_runtime in [prost_toolchain.prost_runtime, prost_toolchain.tonic_runtime]: | ||
runtimes = [prost_toolchain.prost_runtime, prost_toolchain.tonic_runtime] | ||
if not prost_toolchain.compile_well_known_types: | ||
runtimes.append(prost_toolchain.prost_types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why explicitly add prost_types
in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code-generated library will now depend on prost-types
, so I'm adding the dependency here. Since the user will already have provided a prost-types
dependency as an argument to the toolchain, I felt it more consistent to automatically add it to the runtimes for them, instead of asking them to provide it again in the runtimes themselves.
This PR adds a
compile_well_known_types
flag to the Prost toolchain declaration.By default, for backwards compatibility reasons, this flag is set to
True
, as this is the current behavior. If this flag is switched toFalse
, thecompile_well_known_types
option is no longer passed to Prost and Tonic, and the protoc wrapper will ignore types from thegoogle.protobuf
package when computing external types, relying instead on prost's own externals handling. Furthermore, the configured Prost types crate will be added as a runtime dependency to allrust_prost_library
targets.We're currently migrating away from our own set of proto rules to
rust_prost_library
, but our existing code relies heavily on theprost-types
crate's implementations of well-known-types, which is the recommended way of using prost. However, the current behavior ofrules_rust_prost
makes it impossible to wire up properly, hence this PR.Fixes #2058