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

protoc-gen-rust does not follow common file placement conventions #189

Closed
acmcarther opened this issue Dec 27, 2016 · 25 comments
Closed

protoc-gen-rust does not follow common file placement conventions #189

acmcarther opened this issue Dec 27, 2016 · 25 comments

Comments

@acmcarther
Copy link

acmcarther commented Dec 27, 2016

Instead of outputting *.rs files to the "conventional" (per cpp, go, and python) directories -- relative to the source proto files -- the generated files are placed relative to the command invocation site.

When I run the following example command:

protoc --rust_out . ./first/second/example.proto

Instead of

./first/second/example.rs

We see

./example.rs

A more complete example with several generators:

> protoc --rust_out . ./first/second/example.proto
> protoc --cpp_out . ./first/second/example.proto
> protoc --go_out . ./first/second/example.proto
> protoc --python_out . ./first/second/example.proto
> protoc --java_out . ./first/second/example.proto
> tree
├── example.rs                             # Rust
├── first
│   └── second
│       ├── example_pb2.py                 # Python
│       ├── example.pb.cc                  # Cpp
│       ├── example.pb.go                  # Go
│       ├── example.pb.h                   # Cpp
│       └── example.proto                  # Proto
└── org
    └── pubref
        └── rules_protobuf
            └── examples
                └── CommonProto.java       # Java

It appears that only we and the Java rules break from "convention".

I understand this change would likely break many existing users, so I'd understand if this change was rejected for that reason. Example short change diff

EDIT:

To clarify, the primary reason this is an issue for me (and hasn't been for anyone else yet presumably) is because I'm using this protoc extension with bazel instead of cargo+build.rs file, specifically the rules_protobuf "library". That library has an expectation built in that generated source files are placed in a directory structure adjacent to the underlying proto files. It seems that assumption holds for all protoc-gen-* except us and java.

@aajtodd
Copy link

aajtodd commented Jan 22, 2017

Maybe it could be added as an option. It would be nice to output files as name.pb.rs

@beamspease
Copy link

I'd second this request although I'm currently working around it.

@stepancheg
Copy link
Owner

Maybe it could be added as an option. It would be nice to output files as name.pb.rs

name.pb.rs is not valid rust module file name. File could be named name_pb.rs.

@acmcarther
Copy link
Author

acmcarther commented Jun 8, 2017

name_pb would have some interesting side effects. At the very least you'd have to change how you import other protos, since the file name is the same as the name of the module its contents live in.

As an "update" to this, I actually had to do a lot more shenanigans to the generated files to get them to play nice in bazel. I use my own little fork for now, and perform the following additional transformations after the fact using an external script:

  1. Generate a some_proto.rs file
  2. Start a new some_proto_lib.rs file (to wrap that proto) containing
    2a) extern crate protobuf
    2b) extern crate ${some_other_proto_dep}
    2c) pub mod some_proto {
    2d) dump the contents of the file, but drop the "super::" from everything (no idea why)
    2e) pub use ${some_other_proto_dep::*} (ex post facto use is TOTALLY LEGAL RUST)
    2f) }

This impl is against some pretty ancient rust-protobuf though.

EDIT: I remember why super is dropped. At the time (and maybe now too), rust-protobuf assumed that it was compiled using a build.rs, with generated protos living in the same "module". Therefore, all other proto deps were implicitly available in super. In my hacked out version, I'm using extern crate to get them, so i have to drop super::.

EDIT EDIT: The "ex post facto use" is mandatory, as the generated file uses crate attributes, which have to come before any use statements.

@stepancheg
Copy link
Owner

stepancheg commented Jun 8, 2017

I'm not sure you need to drop super:: because you can pub use modules from other crates. And no changes in rust-protobuf needed. E. g.

  • you have crate aaa which contains bbb.proto and generates mod bbb in bbb.rs
  • you have crate xxx containing yyy.proto which includes bbb.proto

Crate xxx (lib.rs) could simply declare (manually, by programmer):

pub use aaa::bbb;

and after that super::bbb will be available from generated yyy.rs.

So basially all imported nnn.proto files need to be available in root namespace of each crate.

Or, better, bazel could simply generate file likes protos.rs which should contain

mod yyy; // for each proto files from current crate
pub use aaa::bbb; // for each proto file declared in imported crates

So each proto file nnn.proto will be available as protos::nnn in each crate, and doesn't matter if nnn.proto declared in current crate or imported.

Am I missing something?

@stepancheg
Copy link
Owner

And another idea.

Probably rust-protobuf could have another generator next to protoc-gen-rust, e. g. protoc-gen-rust-hier.

Instead of importing files as super::bbb it could use a directory name as a crate or module name. So if bbb.proto is declared in zzz/aaa/bbb.rs, protoc-gen-rust-hier could reference imported modules as either ::aaa::bbb or ::zzz::aaa::bbb.

@acmcarther
Copy link
Author

Ah I think you are missing something (that I forgot until now).

The reason I cannot use "super::*" is because using that would pollute the extern-ed namespace of the root crate. That is to say, the following scenario is not possible if you pub use transitive dependencies in the outer-most scope:

  1. Proto_dep_1 depends on proto_dep_2 and proto_dep_3
  2. Proto_dep_2 depends on proto_dep_3

This is caused by the bulk import of proto dependencies (pub use my_dep_proto::*). You end up with a namespace collision on proto_dep_3.

In my example, i've nestled the generated code into a geneated module to hide the module's own proto deps.

That said, I need to completely review the rest of your post, and also the current state of the project. Thanks for your reply!

@stepancheg
Copy link
Owner

Proto_dep_1 depends on proto_dep_2 and proto_dep_3
Proto_dep_2 depends on proto_dep_3

Do you mean that you want to deal with two different proto_dep_3.proto files in the repository?

@acmcarther
Copy link
Author

I mean that I have three totally separate generated crates -- proto_dep_1, proto_dep_2 and proto_dep_3. proto_dep_1 is my actual target, but it transitively depends on proto_dep_3 twice: once for itself, and once through proto_dep_2.

This causes a namespace collision when the contents of proto_dep_3 are resolved within proto_dep_1: a given proto_dep_3::Proto could come in via proto_dep_2::* or from proto_dep_3::*. Let me furnish a real world example.

@acmcarther
Copy link
Author

Ok: please bare with me -- this is not exactly normal rust.

In my tiny game dev project, i have three proto libraries:
game_proto: here
network_proto: here, which depends on game_proto
state_proto: here which depends on game_proto and network_proto

@stepancheg
Copy link
Owner

Do I understand correctly that game.proto,rs is compiled three times in state crate, Once for state, network and game crates? And state crate compiles itself all of game.proto,rs, network.proto,rs and state.proto,rs?

That shouldn't be right. When compiling state.rs generated from state.proto, game.rs should be "used" from game crate, not compiled again. There shouldn't be a collision.

@acmcarther
Copy link
Author

Ah, so its not "compiled" three times. Rather, the reference to the inner module of //../game_proto ("game_proto" again i think), is ambiguous.

That is to say, in the default implementation at the time of writing, the outer module of network_proto would be re-exporting game_proto due to its pub use of it in the outermost scope.

Then when it comes time for state_proto's outer module to go and fetch the game_proto inner module, it doesn't know which to pick.

It boils down to each generated mod.rs pub useing deps.

@acmcarther
Copy link
Author

I think this actually may be an artifact of my own solution to avoiding super::*, not something intrinsic to rust-protobuf. Let me review this code and come back with either a more concrete set of facts, or a pull request. Thanks again for your time in responding.

@jbowens
Copy link

jbowens commented Nov 22, 2017

Do you mean that you want to deal with two different proto_dep_3.proto files in the repository?

@stepancheg do you have a solution for that scenario? It seems like right now rust-protobuf will overwrite the earlier generated .rs files if a filename appears twice within the code base. I've worked with proto repositories that have hundreds of proto packages, and a filename collision is inevitable. Flattening the output into a single directory eliminates all the utility of packages in protos.

@jbowens
Copy link

jbowens commented Nov 27, 2017

Would we be able to use an option to add support in a backwards-compatible way, analogous to these changes to the go_package option?

@stepancheg
Copy link
Owner

@jbowens yes, definitely we could have options to

  • explicitly specify package name per-file
  • global option to use placement in filesystem to generate rust files in corresponding directories (and if it proves to work fine, it could be made a default option later).

@SirVer
Copy link

SirVer commented Feb 13, 2018

I would appreciate this being merged. Working in a mono repo without this is very much harder with the current output placement of files.

@stepancheg
Copy link
Owner

@SirVer

I would appreciate this being merged. Working in a mono repo without this is very much harder with the current output placement of files.

Someone needs to implement it first :) I'd like to do it, but honestly it's unlikely I will have time to do that change soon.

As I said, if should be first implemented as option, and some time later probably made default.

@acmcarther
Copy link
Author

I can put up a PR some time soon with this change, but I need to learn how to pass configuration options through protoc into the plugin. If you happen to know how to do that off the top of your head @stepancheg, that would be handy.

@stepancheg
Copy link
Owner

@acmcarther

It's not easily possible to pass configurations directly through protoc plugin.

However, there are two ways:

First,

option can be specified in rustproto.proto (which has to be imported from user .proto file). This is how for example gogoproto customization works.

Another option

is to pass options to rust cogegen when codegen is invoked from build.rs using protoc-rust crate.

E. g. user could write in build.rs:

extern crate protoc_rust;

protoc_rust::run(protoc_rust::Args {
    out_dir: "src/protos",
    input: &["protos/a.proto", "b.proto"],
    includes: &["protos"],
    options: protoc_rust::Option {
        package_hierarchy: true, // or whatever option name is
        .. Default::default()
    },
}).expect("protoc");

This way of passing options is not implemented yet, but it is relatively easy to implement, and need to be implemented anyway.

This works because protoc-rust uses protoc only as .proto parser which outputs parsed proto files (in binary protobuf), but codegen is invoked directly, not as protoc plugin.

@acmcarther
Copy link
Author

This works because protoc-rust uses protoc only as .proto parser which outputs parsed proto files (in binary protobuf), but codegen is invoked directly, not as protoc plugin.

Ah this is news to me! I use the protoc_gen_rust binary directly with protoc to generate my rust and haven't noticed any issues. Are you planning to deprecate this usage?

Regarding passing data directly through protoc: Apparently there is a parameter convention used with the --rust_out flag, of the form --rust_flag=${parameters}:OUT_DIR. This is available in the code generator request here, and could be used to change the behavior of the code generator. Alternatively the behavior could be gated behind a crate feature.

@stepancheg
Copy link
Owner

@acmcarther

Ah this is news to me! I use the protoc_gen_rust binary directly with protoc to generate my rust and haven't noticed any issues. Are you planning to deprecate this usage?

Probably not, because this is the way Google recommends using protobuf.

However, readme of rust-protobuf project recommends using protoc-rust crate and build.rs (since several months ago).

Regarding passing data directly through protoc: Apparently there is a parameter convention used with the --rust_out flag, of the form --rust_flag=${parameters}:OUT_DIR

I don't recall if any protobuf implementation for other languages uses this mechanism, and this is probably inconvenient, but yes, this also works.

So, I think, there shoud be three ways of passing parameters to code generator:

  • rustproto.proto
  • --rust_out=OPTIONS:OUT_DIR
  • with protoc-rust crate and function parameters

And all three alternatives should support exactly the same set of options.

@mineralres
Copy link

is there any solution today?

@ruffsl
Copy link

ruffsl commented Oct 6, 2019

I've worked with proto repositories that have hundreds of proto packages, and a filename collision is inevitable. Flattening the output into a single directory eliminates all the utility of packages in protos.

I'm encountering this as well, as I'm trying to retain a similar module hierarchy to that of the directory tree all source .proto files are structured in my project. Why should flattening generated files into a single directory ever be the default behavior? How are folks working around this issue today?

@stepancheg
Copy link
Owner

API significantly changes in the last two years. Closing due to old age. Please reopen if you have suggestions before rust-protobuf=3 released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants