dropbox / pb-jelly

A protobuf code generation framework for the Rust language developed at Dropbox.
Apache License 2.0
610 stars 25 forks source link

pb-jelly-gen does not work on Native Windows #77

Closed JParisFerrer closed 3 years ago

JParisFerrer commented 3 years ago

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`.
nipunn1313 commented 3 years ago

awesome - thanks for finding this This seems related to #67 - adding CI support for windows.

For what it's worth - pb-jelly's codegen definitely works on Windows (and we use it on windows at DBX) - however, pb_jelly_gen seems like it does not.

If you directly invoke protoc with --rust_out referencing the codegen.bat - it'll work.

JParisFerrer commented 3 years ago

After messing around with file encoding, BOM, etc etc I worked around it via reinstalling all the deps in WSL and running the gen there. Not ideal, but easier (although probably not faster) than building/writing down some protoc command somewhere.

Note an issue I noticed: protos/myproto.proto fails with a confusing error message (see below), but moving it into protos/myproto/myproto.proto seemed to fix that. Should that be filed separately? 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.
nipunn1313 commented 3 years ago

Yep - codegen.py appears to expect that it is in a directory which implies the crate name. file something separately for that.

one option

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.

nipunn1313 commented 3 years ago

@JParisFerrer - just added windows support! Fixed the bugs in #74