Split ExternType out into a standalone crate

[sbrocket]

As mentioned in trait/README.md, this is useful for implementing ExternType for types in a library crate without requiring all users of the library to link in the full cxx along with its C++ symbols.

[dtolnay]

Thanks for the PR!

I think I would prefer not to make this change. In particular I think this is the wrong model:

For example, a widely used library crate can depend on this, enabling but not requiring its users to use the library's types with cxx.

This implies that you want the widely used library unconditionally depending on the cxx-traits crate, whether or not anything downstream needs the impls. A better way would be for cxx to be an optional dependency of the widely used library and then conditionally compile the impls. That avoids introducing the fairly heavy dependency on cxxbridge-macro when it's never needed.

[sbrocket]

Unfortunately the optional dependency approach does not work with our non-Cargo build system. Our build system (GN) has no equivalent. I could do something like it by splitting the library into 2 variants - once with the feature, once without - and requiring consumers to manually pick the variant they want, but I believe that won't work as soon as the dependency graph ends up including both versions (which is extremely likely in my use case, since I want to use this in a crate depended on by ~every Fuchsia Rust crate).

I believe Cargo works around this using name mangling, but I think even that approach has limitations in that objects from the different versions are incompatible? Or perhaps in this case Cargo would simply figure out whether the feature is needed for anything in the dependency graph. In any case, that approach is not available outside of Cargo.

[sbrocket]

Also, the approach you recommend is still available for environments where Cargo is in use, and perhaps we could recommend in the README that people prefer that approach unless it does not work for them.

[dtolnay]

I would want to understand a bit more about the problem this is solving in the context of your build system / use case. (Basically I can see this is a solution to something, but the something hasn't been pinned down enough to tell what better solutions are available.)

From the PR description:

this is useful for implementing ExternType for types in a library crate without requiring all users of the library to link in the full cxx along with its C++ symbols

• Would you be able to clarify whether this is primarily intended as just a build speed improvement or something else?

• Note that in general the cxx crate's C++ symbols from src/cxx.cc are not coupled to the rest of the Rust library except in Cargo, due to build.rs. Cargo requires that Rust is the center of the universe and any piece of non-Rust needs to be strung onto some responsible piece of Rust code. This is not the case in natively polyglot build systems.

  For example in Buck it would be easy to represent a dependency on the Rust code but not the C++ code. If this PR is primarily motivated by the presence of C++ symbols in code that otherwise wouldn't use C++, would it make more sense for you to split the cxx related build logic along C++ vs Rust, instead of along trait vs non-trait?

• What were the actual downside of having the C++ symbols in the dependency graph prior to this change? Link failures? Binary size?

• If binary size, do you have any insight into why unused symbols would not have been eliminated at link time? Is it possible to quantify any benefit from this PR?

[sbrocket]

No, this wasn't targeting build speed or binary size, but link failures. Specifically the case where the library providing the ExternType-implementing types is included in a Rust static library target that does not otherwise use cxx. With static library targets the unused symbols can't yet be pruned, leading to link failures if the C++ symbols are not included.

For example in Buck it would be easy to represent a dependency on the Rust code but not the C++ code.

Yes, I currently have that. The Rust crate and a C++ source_set with src/cxx.cc are separate targets that can be separately depended on. This in fact works fine for binary targets that depend on the library because the unused cxx symbols get removed and so no link errors result. The problem is specifically static libraries.

That leaves me with roughly the following for solutions, as far as I can tell, unless you have another proposal:

1. Include the C++ symbols in any static libraries that otherwise have no reason to link against them. Obviously non-ideal. I wouldn't go with this approach if only for the confusing DX, if not for other reasons.
2. This proposal, factoring out the trait. (Or alternatively you could think of it as factoring out everything that doesn't depend on C++ symbols, but I think those sets are currently identical.)
3. Or I could give up on having the types directly implement the trait and create a separate library with repr(transparent) newtype wrappers and some From impls to convert. I'd prefer the option above because it would lead to a more seamless DX - developers who are already familiar with our library types could use them directly in the bridge without the extra mental overhead of a newtype wrapper - but I could live with this.

[sbrocket]

Actually, I believe I mis-analyzed the cause of the link failures; let me take another look.

[sbrocket]

Ah yeah, I mixed up my hack around it and the cause when I typed the above response out. The link failures are from building a shared library (that includes a static library which in turn depends on the Rust library where I'd like to implement ExternType for types) and the shared library is built with -no-undefined.

Specifically, here's the link failures unless the C++ symbols are included somewhere: https://gist.github.com/sbrocket/4e5f47149e29d42f4a3358e440238ada

So yes, the unused Rust symbols are not getting pruned before attempting to build the shared library and then that fails because the C++ symbols are still undefined. Perhaps there's another option to make sure those unused Rust symbols get pruned earlier?

[dtolnay]

Ah okay, thanks for the update. That makes more sense given what I understand from Buck's handling of this.

In Buck builds, Buck gets informed of the dependency relationship between the cxx crate rust_library target and the related cpp_library target containing those C++ symbols. Buck uses that dependency graph when putting together the linker invocation (whether for a binary or shared library). So anywhere the Rust code would be linked, the required C++ code is also made available to the linker.

Do you see any possibility of representing that in the GN case?

My hesitation with "just accepting" this PR is that the failure mode you are describing doesn't seem at all specific to the cxx crate, but to any dependency relationship of Rust on C++. Eventually as you mix the languages more and more in your codebase at some point you are going to need to make GN able to handle the situation without link errors, at which point the workaround of splitting traits out of cxx is made unnecessary, yet we are left stuck supporting it on our end.

[sbrocket]

I have tried that - adding the C++ dependency alongside the Rust crate dependency to the target implementing ExternType - but it seems the dependencies don't propagate fully or correctly. The target graph looks something like this:

fuchsia-zircon rust_library target (I believe this produces an rlib) <- wlan-mlme-c rustc_staticlib <- wlan driver loadable_module

The direct dependency on cxx is in fuchsia-zircon, but adding a dependency on the C++ symbols there instead of directly to the staticlib in the middle still results in link failures in the driver target. I'm not sure if this is a defect in GN's native Rust support and it's rust_library target or expected behavior somehow.

I had thought this might be more generally useful originally, but I'll just go ahead with my 3rd option above and use a separate crate with repr(transparent) wrappers to implement the trait.

[dtolnay]

Which linker is producing wlan driver loadable_module? If gold, you might need to get GN to put --start-group/--end-group around the pair of cxx Rust and C++ inputs. (Not needed if lld.)

One way I would think to debug would be to reproduce a similar target graph in Cargo and then compare the rustc and ld invocations made by Cargo vs the ones made by GN.