dropbox / pb-jelly Goto Github PK

Currently, codegen supports three different forms of ZC codegen

rust.grpc_slices=true
rust.zero_copy=true
rust.blob=true

Analyze/understand why these are here and what they are, and potentially simplify.

Currently there's some non-OSS service generation code at DBX which relies on this - which can come out as well (pb_service)

This will allow us to be able to assert that the generated files match the ones as expected.

It will also mean that PRs when put up will more obviously show the changes to generated output.(as the .expected files will have differences)

In the benchmark tests under pb-test we create a proto message VecData that has a single field data that is a Vec<u8>. The performance of our deserialization compared to that of PROST! is much worse. In my benchmarking it's about 4,000,000 ns vs 350,000 ns. We should figure out where the slow down is and improve this

It is very confusing to me.

• Command::default() allows creating an object with everything missing
• Command { ts: 1 } complains that I will have to pass all values.
• I have to always pass a Some (regardless required or optional) for each field (this is actually the real issue)

Is there no way I could initialize Command where ts is not Option?

Code (proto 2)

message Command {
    required uint64 ts = 1;
    required CommandType command = 2;
    optional uint64 trace_id = 3;
    optional Service service = 4;
}

I have a proto file called enums.proto

Is there a way not to use it like:

use proto_enums::proto_enums::Error;

but instead generate it with one of the following (or another suggestion)

use proto::enums::Error;
use proto_enums::enums::Error;

This line and similar:
dd5336e#diff-fbffb38ecd902027ff1466187043d62f5e37c82f7595b2559db1344b05c11c6fR1441
are generating a lot of clippy lints about the return not being necessary.

I don't understand the field reflection work well enough to make the change. Do those always go at the end of a method? If not, is there a way to detect if they are the last statement?

As a last resort we could silence that lint here.

After playing around with adding an example of how to use pb-rs in #7, I think making pb-rs a workspace with three crates, pb-rs, pb-gen, pb-tests would make it more usable/portable.

1. pb-rs would contain pretty much everything currently under /src aka only the traits and structs necessary to support the generated code. This would cargo build with stable Rust and would require no other dependencies, this is what users would include in a Cargo.toml as a dependency.

2. pb-gen would contain /codegen and a Rust wrapper around/replacement of gen_protos.sh. This is what users would include in a Cargo.toml as a build-dependency. To start this would required Rust, Python, Go, and protoc because of the way our gen_protos.sh works. We could remove the Go dependency relatively easily (I think), but building a more batteries included codegen is a different task.

3. pb-tests would contain the more integration like tests between Rust and Go, and would also contain benchmarks. We could keep this using nightly Rust with the #![feature(test)] or compile it with stable by using criterion instead, but since this is only required for development, and the dev instructions are well documented, slimming the dependencies here is a low priority IMO.

Let me know what you think, would love some comments!

This task is to create an example in the examples crate that uses the (rust.preserve_unrecognized) extension. Then update the main README to reflect the new example.

This is pretty simple, in the codegen script we need to update the template for the rust_library bazel rule include edition = "2018", e.g. the rule should be:

rust_library(
    name = "{crate}",
    crate_type = "lib",
    edition = "2018",
)

Currently when you try to mark a field as [(rust.zero_copy)=true] in a proto3 file, the code gen fails. We should figure out why and fix this

This will ensure that pb-jelly serializes identically to the go autogenerated code.

This test exists in the DBX codebase - but needs to be ported to run here (or rewritten from scratch) using the pbtest test proto.

This task is to create an example in the examples crate that uses the (gogoproto.nullable) extension. Then update the main README to reflect the new example.

Python2 is dead, long live python2!

codegen.py in the pb-jelly-gen crate is a protoc plugin that powers the Rust code generation. We should migrate this to python3

Our codegen script is Python2, for developers to be able to run it, they'll need to install a few packages. To make that easier we need to make a requirements.txt

Per https://github.com/github/renaming - we'd like to rename master to main - waiting for github to provide the tools later this year.

protos/myproto.proto fails with a confusing error message (see below), but moving it into protos/myproto/myproto.proto will codegen successfully. The comment here right above the error sort of suggests its not expected.

[/home/jordi/.cargo/registry/src/github.com-1ecc6299db9ec823/pb-jelly-gen-0.0.4/src/lib.rs:169] output.status.code() = Some(
    1,
)
stdout=
stderr=Traceback (most recent call last):
  File "/tmp/codegenMIlOse/codegen.py", line 1783, in <module>
    generate_code(request, response)
  File "/tmp/codegenMIlOse/codegen.py", line 1768, in generate_code
    generate_single_crate(ctx, "", to_generate, response)
  File "/tmp/codegenMIlOse/codegen.py", line 1683, in generate_single_crate
    mod_name = mod_parts[-1]
IndexError: list index out of range
--rust_out: protoc-gen-rust: Plugin failed with status code 1.

From discussion over in #77:

one option
provide a better error message - telling folks to move the proto file into a directory
support this case - perhaps picking a crate name to match the file name?
I haven't really thought it through to decide which case to prefer.
Feel free to fix it! It seems like it would be hard to add a unit test in our testing framework for error situations (first option) - > but we could certainly test the second option.

Something to automatically post on PRs to ensure CLA is signed (eg https://colineberhardt.github.io/cla-bot/#installing-cla-bot)

Generates a different rust type which implements pb::Message to represent the field.

Including

• running mypy on codegen.py (using https://github.com/dropbox/mypy-protobuf for mypy-ing imported proto files)
• running the test suite via cargo
• running the benchmarks
• ensuring files are rustfmt'd

Inline the mypy annotations
Remove reference to six library

We should wait until python2 end-of-lifes before completing this task!

pb-jelly generates different proto modules to different crates. Add example to exercise this.

Creating a -gen create using pb-jelly-gen does not work on Windows due to unix-specific imports.

Output of cargo run --bin proto-gen (proto-gen is the gen crate in a cargo workspace):

   Compiling pb-jelly-gen v0.0.4
error[E0433]: failed to resolve: could not find `unix` in `os`
  --> <home dir>\.cargo\registry\src\github.com-1ecc6299db9ec823\pb-jelly-gen-0.0.4\src\lib.rs:42:9
   |
42 |     os::unix::fs::PermissionsExt,
   |         ^^^^ could not find `unix` in `os`

error[E0599]: no method named `set_mode` found for struct `std::fs::Permissions` in the current scope
   --> <home dir>\.cargo\registry\src\github.com-1ecc6299db9ec823\pb-jelly-gen-0.0.4\src\lib.rs:273:29
    |
273 |                 permissions.set_mode(0o777);
    |                             ^^^^^^^^ method not found in `std::fs::Permissions`

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0433, E0599.
For more information about an error, try `rustc --explain E0433`.
error: could not compile `pb-jelly-gen`.

Given the current benchmark testing feature requires the nightly compiler, we should consider migrating to criterion.

Whoever handles this task should investigate if the unstable test feature is going to be stabilized any time soon (soon is mauybe < 6 months?) and if it isn't, then we should migrate to criterion.

The API for pb-gen was written kind of hastily, we should remove the logging that is part of the implementation, and expose a properly documented builder struct so the code generation can have more options.

Currently if protoc fails then gen_protos(...) from pb-gen silently succeeds. The only sign that code generation failed is your generated files are gone because we clean the directories before calling protoc. You can see the error protoc emits by running cargo build -vv and scrolling through the output, but this isn't very friendly.

The task is to update pb-gen such that when protoc fails then the build step should fail too with the error from protoc.

Make sure our tests pass on windows + ensure library continues to work on windows