Allow file descriptor be generated without --include_source_info #786
Conversation
dfreese
force-pushed
the
comments
branch
2 times, most recently
from
85fd9aa
to
bffe7c4
Compare
The file descriptor sets generated by rules_proto in bazel don't have this by default, so this allows some flexibility for users to reuse results that are already available to them. It's fairly trivial adjustment so it seemed reasonable to me to allow the flexibility.
prost-build/src/ast.rs
Outdated
| @@ -134,7 +134,7 @@ pub struct Service { | |||
| /// The package name as it appears in the .proto file. | |||
| pub package: String, | |||
| /// The service comments. | |||
| pub comments: Comments, | |||
| pub comments: Option<Comments>, | |||
There was a problem hiding this comment.
instead of making this breaking change can we instead make it so comments has a Comments::default()?
There was a problem hiding this comment.
Looking at prost-build/src/lib.rs:154, the ast module isn't public, so I assumed this was an implementation detail of prost-build.
mod astDid I misunderstand that somehow?
There was a problem hiding this comment.
oh, nevermind. Missed line 147:
pub use crate::ast::{Comments, Method, Service};There was a problem hiding this comment.
Added a derived Default to Comment. I went that direction instead of directly defining one since all of the fields of Comment are public, so unlikely to change, and the derived default is what I'd expect here.
|
Awesome, I am gonna need some time to review this but I am keen to merge this. One cool thing that this could allow us is to use the |
…io-rs#786) * Allow file descriptor be generated without --include_source_info The file descriptor sets generated by rules_proto in bazel don't have this by default, so this allows some flexibility for users to reuse results that are already available to them. It's fairly trivial adjustment so it seemed reasonable to me to allow the flexibility. * revert breaking changes and add default
I've been working on another pure-rust replacement for protoc, protox, which does have full support for SourceCodeInfo. Its not very mature yet, but it should be sufficient for use with prost-build. My main concern with using it directly in prost-build would be that it uses prost-types internally, which might make publishing new versions difficult |
Oh wow, this is really really awesome! If you're interested, would love to add this to prost or something like that. I think this could be a huge valuable change to the protobuf ecosystem in rust.
I don't think this matters since prost-build depends on prost-types? |
I was thinking the process would have to be publish prost-types -> publish protox -> publish prost-build. Its certainly doable, and I'd be happy to share ownership of protox or merge it into the prost repo to make releases easier |
|
@andrewhickman yeah, I am open to what ever you are interested in. I don't have much time to take on the additional maintenance but if you're interested in contributing it, I can add you as a maintainer. I would love some help on prost anyways if you're interested. |
* Allow file descriptor be generated without --include_source_info (tokio-rs#786) * Allow file descriptor be generated without --include_source_info The file descriptor sets generated by rules_proto in bazel don't have this by default, so this allows some flexibility for users to reuse results that are already available to them. It's fairly trivial adjustment so it seemed reasonable to me to allow the flexibility. * revert breaking changes and add default * release 0.11.5 (tokio-rs#788) * Add message and enum attributes to prost-build (tokio-rs#784) closes tokio-rs#783 * chore: Prepare 0.11.6 release (tokio-rs#794) * chore: Added Kani to CI. (#1) (tokio-rs#798) * Added Kani documentation. (tokio-rs#799) * Fix issue with negative nanos in Duration::try_from(), and add tests (tokio-rs#803) * Fix issue with negative nanos in Duration::try_from(), and add a unit test * add proptest for negative nanos * PR comment: remove spurious dbg * Prevent spurious overflow in check_duration_roundtrip test (tokio-rs#804) Co-authored-by: Lucio Franco <luciofranco14@gmail.com> * Clarify `default_package_filename` documentation. (tokio-rs#809) * Bump msrv to 1.60 (tokio-rs#814) * chore(types): Remove including generated code (tokio-rs#801) * chore: Update github action (tokio-rs#815) Co-authored-by: Lucio Franco <luciofranco14@gmail.com> * chore: Add cargo-machete to detect unused dependencies (tokio-rs#817) * chore: Update msrv to 1.60 (tokio-rs#818) * feat: Added try_normalize to Timestamp (tokio-rs#796) Co-authored-by: Lucio Franco <luciofranco14@gmail.com> * Update PropProof docs to note the need to submodule init (tokio-rs#805) Co-authored-by: Lucio Franco <luciofranco14@gmail.com> * feat(build): Add direct fds compile support (tokio-rs#819) This commit adds two new compile functions that allow passing a `FileDescriptorSet` and it will generate the Rust code. This allows users to use libraries like `protox` directly and in an ergonomic way. * release 0.11.7 (tokio-rs#821) * fix: correct change in visibility of compiler module (tokio-rs#824) Introduced in tokio-rs#801 Closes tokio-rs#823 * release 0.11.8 (tokio-rs#825) * Add existing roundtrip test to Kani CI and avoid recursive submoduling (tokio-rs#828) * Add existing roundtrip test to Kani CI * Bump up Kani version * Remove `v` from GA version * Update to `syn@2` & `prettyplease@0.2` (tokio-rs#833) * Fix corrupted tests and missing CI testing (tokio-rs#832) Co-authored-by: Lucio Franco <luciofranco14@gmail.com> * chore: Update to criterion 0.4 (tokio-rs#835) * Fix build in directory not named `prost` (tokio-rs#839) This library will fail to build with the following error when checked out into a directory not named exactly `prost`: error: failed to load manifest for workspace member `/home/alex/src/prost-rs/tests/single-include` Caused by: failed to load manifest for dependency `prost` Caused by: failed to read `/home/alex/src/prost/Cargo.toml` Caused by: No such file or directory (os error 2) This is because the `single-include` test depends on `prost` with the path `../../../prost`. This patch fixes the issue by correcting the path to `../..`. * prost-build: support boxing fields (tokio-rs#802) This allows the user to request boxing of specific fields. This is useful for enum types that might have variants of very different size. * chore: Update to baptiste0928/cargo-install@v2 (tokio-rs#840) Co-authored-by: Lucio Franco <luciofranco14@gmail.com> * Fix typo in bail message (tokio-rs#848) --------- Co-authored-by: David Freese <freese@google.com> Co-authored-by: Lucio Franco <luciofranco14@gmail.com> Co-authored-by: damel_lp <dlambertpowell@gmail.com> Co-authored-by: Yoshiki Takashima <ytakashi@andrew.cmu.edu> Co-authored-by: Daniel Schwartz-Narbonne <danielsn@users.noreply.github.com> Co-authored-by: Gilad Naaman <gilad@naaman.io> Co-authored-by: tottoto <tottotodev@gmail.com> Co-authored-by: Oliver Browne <109075559+oliverbrowneprima@users.noreply.github.com> Co-authored-by: Marcus Griep <marcus@griep.us> Co-authored-by: Adrian Palacios <73246657+adpaco-aws@users.noreply.github.com> Co-authored-by: Donough Liu <liudingming@bytedance.com> Co-authored-by: Alex O'Brien <3541@3541.website> Co-authored-by: Thomas Orozco <thomas@orozco.fr> Co-authored-by: Brendon Daugherty <brendon1097@gmail.com>
The file descriptor sets generated by rules_proto in bazel don't have this by default, so this allows some flexibility for users to reuse results that are already available to them. It's fairly trivial adjustment so it seemed reasonable to me to allow the flexibility.