Fix "two different versions of crate" compiliation error

[colin353]

This PR doesn't actually fix anything yet, just introduces a failing example so I can debug it. I've been trying to update my personal rust + protobuf + grpc repository to the latest rules_rust, but it won't compile for the same reason as this example.

The bug is: when I have a rust binary which depends on a protobuf library A and another library B (which depends on A), I end up with A linked in twice, and rustc gives this error:

 --> proto/main.rs:5:30
  |
5 |     common_lib::do_something(&common_proto_rust::Config::new());
  |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `common_proto_rust::common::Config`, found struct `common_proto_rust::Config`
  |
  = note: expected type `&common_proto_rust::common::Config`
             found type `&common_proto_rust::Config`
note: Perhaps two different versions of crate `common_proto_rust` are being used?

I've binary searched for the commit which introduced the bug, and I can confirm that the example compiles before 7cde1e4 but is broken afterward, i.e. the bug was introduced by #240.

I don't have a great understanding of what could cause this, but I dug into the rustc invocations before and after this PR, and noticed an extra rustc arg -Ldependency=bazel-out/k8-fastbuild/bin/myproto_proto.proto.rust in the broken invocation when compared to a working one.

I'm going to try to do some debugging on this this weekend, but if anyone has any pointers or ideas, that'd be greatly appreciated.

[colin353]

Update: basing off of 7cde1e4, I can get my example to compile correctly by removing:

In rust/private/rust.bzl:L323

"_whitelist_function_transition": attr.label(
        default = "//tools/whitelists/function_transition_whitelist",
    ),

and rust/private/rust.bzl:L323

rust_library = rule(
    _rust_library_impl,
    attrs = dict(_rust_common_attrs.items() +
                 _rust_library_attrs.items()),
    fragments = ["cpp"],
    host_fragments = ["cpp"],
    #cfg = wasm_transition, <--- remove this line
    toolchains = [
        "@io_bazel_rules_rust//rust:toolchain",
        "@bazel_tools//tools/cpp:toolchain_type"
    ],
    doc = """
    ...

No clue what any of that means, but I'll keep digging

[colin353]

Obviously just deleting these lines isn't the solution, but it does get the example to compile.

[ghost]

Any news on this one? I just got it as well updating the rules_rust in my repo. Can I help somehow?

[colin353]

@Sythanis - I didn't make much progress on this, still trying to understand what these two lines actually do. My strategy was to use bazel build -s to get the rustc commands and try to diff them to get a better understanding of the problem.

If you want to help, I think the solution should be something simple - if you check out this branch and run

bazel build -s //proto:common_proto_rust

you can get the rustc invocations bazel is generating and try to figure out what's going wrong.

@johnedmonds maybe you have a better idea since you wrote #240 ?

[dfreese]

@colin353 Thanks for putting this together. This is a much cleaner example of that I had run into in #300 , though finding this sooner would have saved me a lot of debugging. The same problem and strategy continues to work on master.

[colin353]

I tried to reach out to the author of the wasm PR, with no luck.

I think if we can somehow figure out the test failures in this branch/why that code is necessary for wasm, we can fix this.

I'll spend some more time looking at it this weekend and see if I can make any progress - want to do the same and compare notes?

[dfreese]

Sounds good, though I might not be able to dedicate a ton of time to this over the weekend, but was planning to on Monday.

…

On Sat, Apr 11, 2020 at 08:36 Colin Merkel ***@***.***> wrote: I tried to reach out to the author of the wasm PR, with no luck. I think if we can somehow figure out the test failures in this branch/why that code is necessary for wasm, we can fix this. I'll spend some more time looking at it this weekend and see if I can make any progress - want to do the same and compare notes? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#294 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2K7M7LBHLY5CEBULP2LRDRMCE7VANCNFSM4KZNTCRQ> .

[johnedmonds]

Sorry I missed this. I can try to dig into it a bit this week. If I'm understanding correctly is it that we're just not properly de-duping the dependencies?

[colin353]

@johnedmonds no problem! Yeah I think that just be the root of it. I don't have a strong grasp on what was done in the wasm PR so any insight would be super valuable.

[johnedmonds]

@colin353

I can try to explain the part you've identified in this PR:

WASM is basically a separate compiler target but gets represented by a different toolchain (i.e. basically we swap out rustc (x86) for another rustc (wasm32-unknown-unknown)). Transitions are applied to a rule and basically let you swap out the build config (i.e. the toolchain for us) for all dependencies of the rule.

But some dependencies like proc-macro crates shouldn't be built in wasm (wasm32-unknown-unknown) because they need to be executed on the bazel host machine (e.g. x86_64-unknown-linux-gnu) as part of the build. So we add this transition to rust_library so that if we detect a proc-macro crate, we transition to building using the host config for that and all its dependencies (the proc_macro_host_transition).

I'm not sure why this would cause the library to be linked twice. It's possible bazel somehow thinks the host transition is considered a different architecture than the one you're currently building for. I wonder if a short-term solution is to only introduce the transition if you're transitioning from a wasm toolchain. That doesn't explain why it's happening but it might fix it for most use case (and if it works that gives us more information about the true fix).

I can try to experiment if that fixes this problem.

[colin353]

Hmm, maybe that could explain it. Worth a shot to try it! Let me know if you get anywhere with it

[dfreese]

Looks like it might be because rust_binary and rust_library have two different cfg types. rust_library has one, while rust_binary is not specified. Adding the same thing to rust_binary appears to solve the issue.

[dfreese]

The "same thing" being:

    cfg = proc_macro_host_transition,

to rust_binary's rule definition, and

      "_whitelist_function_transition": attr.label(
        default = "//tools/whitelists/function_transition_whitelist",
    ),

to __rust_binary_attrs

[dfreese]

spoke too soon. I had made _proc_macro_host_transition a pass through by just returning settings. That didn't work, however, making it a pass through, and then adding the same configuration to both rust_library and rust_binary let at least the example compile, so perhaps that's a clue to what's going wrong.

[johnedmonds]

@dfreese That helps. Bazel transitions can cause a target to be built multiple times (once per build config). So I'm guessing A is getting built twice but because it's being built with a different config, the depset can't figure out how to dedup it. If that's what's happening then maybe this is because the starting build config is somehow different from the "host" config so bazel thinks it actually is doing a transition when it shouldn't be (it should be transitioning from host -> host which should not be considered a transition).

[dfreese]

So looking at my error messages a little more clearly, it looks like when I just added the cfg = proc_macro_host_transition, and whitelist to rust_binary, that was causing proc_macro_host_transition to fail on not having the "crate_type" attribute for some targets. Changing the check in _proc_macro_host_transition to

if hasattr(attr, "crate_type") and attr.crate_type == "proc-macro":

seems to work.

Would the more appropriate solution be to move the whitelist into _rust_common_attrs from _rust_library_attrs, and add cfg = proc_macro_host_transition to all of the rules?

[johnedmonds]

I think that makes sense

[dfreese]

Even easier, #270 fixes this issue. Looks like it was never merged, and needs to be updated for rustfmt.

Didn't realize examples was a separate bazel workspace. Uploaded PR #301

[colin353]

Solved by #301. Thanks @dfreese!