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.

Sign up for GitHub

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

Jump to bottom

Multiple proto generation/linking causing compilation issue #300

Closed
dfreese opened this issue Apr 10, 2020 · 5 comments · Fixed by #301
Closed

Multiple proto generation/linking causing compilation issue #300

dfreese opened this issue Apr 10, 2020 · 5 comments · Fixed by #301

Comments

@dfreese
Copy link
Collaborator

dfreese commented Apr 10, 2020

I've tried to put a minimally reproducible example of the problem I was running into here

When going from commit 5c0e90a to commit 7cde1e46, via #240, I was getting compilation errors, of

the trait bound `server::EchoImpl: echo_rust_grpc_proto::Echo` is not satisfied

when importing a server implementation from another file that's trying to create a gRPC/protobuf server. This is using the rust_library, rust_executable, proto_library, and rust_grpc_library rules.

The compilation between those two different commits seems nearly identical, but in the failing case, the SUBCOMMAND: # //proto:echo_proto seems to be run twice, and then linked into the executable twice.

I'm new to rust. Is this something incorrect in that workspace setup, or is something else going wrong?
@dfreese
Copy link
Collaborator Author

dfreese commented Apr 10, 2020

And I should note that I see this same issue at master as well, the commits are the result of a bisect.

@charlesfoo
Copy link

+1, seeing the same issue when building in bin/ directory using commit 7cde1e4

@dfreese
Copy link
Collaborator Author

dfreese commented Apr 11, 2020

Following up with this, commenting out these two sections in //rust/private/rust.bzl removes the problem. A similar strategy of commenting out this section and this line works on master.

@colinmerkel-th
Copy link

Seems like the same problem described in #294

@dfreese
Copy link
Collaborator Author

dfreese commented Apr 11, 2020

@colinmerkel-th thanks for pointing that out. I didn't think to look into PRs. That's a much cleaner example.

@dfreese dfreese mentioned this issue Apr 11, 2020
Closed
dfreese added a commit to dfreese/rules_rust that referenced this issue Apr 11, 2020
@dfreese
Add cfg = proc_macro_host_transition to all rules
a59c939 
The rust_library rule specifies a cfg option.  This performs transitions
in order to support WASM.  Other rules, such as rust_binary, do not have
this configuration, which causes Bazel to build and link some libraries
into the final binary twice, resulting in compilation errors.  This is
detailed in pull request bazelbuild#294 and issue bazelbuild#300.  The issue was introduced
in commit 7cde1e4.

This adds the example from @colin353 in PR bazelbuild#294 as an example that would
otherwise fail.

Fixes bazelbuild#300
@dfreese dfreese mentioned this issue Apr 11, 2020
Merged
mfarrugi pushed a commit that referenced this issue Apr 11, 2020
@dfreese
Add cfg = proc_macro_host_transition to all rules (#301)
a1d8936 
The rust_library rule specifies a cfg option.  This performs transitions
in order to support WASM.  Other rules, such as rust_binary, do not have
this configuration, which causes Bazel to build and link some libraries
into the final binary twice, resulting in compilation errors.  This is
detailed in pull request #294 and issue #300.  The issue was introduced
in commit 7cde1e4.

This adds the example from @colin353 in PR #294 as an example that would
otherwise fail.

Fixes #300
dfreese added a commit to dfreese/rules_rust that referenced this issue Apr 20, 2020
@dfreese
Remove transition from binary/test/bench rules, add to grpc/proto
702a896 
This mostly reverts commit a1d8936, removing the
proc_macro_host_transition from most rules.  This fixes bazelbuild#305, by no
longer having a transition on rust_binary.  It still solves bazelbuild#300 by
adding this transition to rust_proto_library and rust_grpc_library.
Those rules are meant to mirror rust_library, so they should share the
same cfg values.  Sharing the same configuration means that
rust_proto_library or rust_grpc_library can be dependencies to
rust_library or rust_binary and not cause another configuration of the
library to be generated.

The example introduced by bazelbuild#301 is moved in this commit into a subfolder
to provide a more clear directory structure.
@dfreese dfreese mentioned this issue Apr 20, 2020
Merged
damienmg pushed a commit that referenced this issue Apr 21, 2020
@dfreese
Remove transition from binary/test/bench rules, add to grpc/proto (#309)
e285f2b 
This mostly reverts commit a1d8936, removing the
proc_macro_host_transition from most rules.  This fixes #305, by no
longer having a transition on rust_binary.  It still solves #300 by
adding this transition to rust_proto_library and rust_grpc_library.
Those rules are meant to mirror rust_library, so they should share the
same cfg values.  Sharing the same configuration means that
rust_proto_library or rust_grpc_library can be dependencies to
rust_library or rust_binary and not cause another configuration of the
library to be generated.

The example introduced by #301 is moved in this commit into a subfolder
to provide a more clear directory structure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add cfg = proc_macro_host_transition to all rules dfreese/rules_rust
3 participants
@dfreese @charlesfoo @colinmerkel-th