Repository navigation
proc-macro crates should build for the host instead of the target #205
Description
While experimenting with cross compilation using rules_rust I noticed something odd: it seems like bazel builds crates marked with the proc-macro type for the target platform instead of the host platform. This breaks procedural macros when cross compiling, especially when the target platform does not have a standard library.
I guess that we would want rules_rust to convince bazel to compile procedural macro crates and their dependencies for the host platform instead of the target platform, but I do not know enough about Bazel to determine if and how this would be possible.
For now I came up with an ugly but working solution: I added an extra bool field to rust_library named proc_macro_dep. If this field is set to true, the --target argument is omitted from rustc invocations. I can provide the patch in case anyone is interested.
Activity
mfarrugi commentedon Mar 13, 2019
A pull request with your workaround could be useful to others.
@damienmg do you know if we would be able to dynamically set
cfg = "host"inrust_library? The other solution I can come up with is a macro that callsrust_librarywhen the crate isn't a proc_macro, and a newproc_macrorule otherwise.wildarch commentedon Mar 14, 2019
One thing we should keep in mind when fixing this is that some crates in the dependency graph might need to be compiled for the target as well as the host if they are both a direct dependency as well as a transitive dependency from a procedural macro. While this would be easy to solve with a duplicate
rust_librarydefinition, I think it would be more ergonomic if this was automatically handled byrules_rust.I will be happy to whip up a PR for this issue once consensus on the right approach has been reached.
damienmg commentedon Mar 14, 2019
@mfarrugi pulling in @katre and @gregestren who understand how multi-platform configuration works in Bazel.
The main thing is that downstream
rust_libraryshould request the proc macro as host, not the target be compiled for host.@wildarch: how do that works when it needs to be compiled for both? Why would transitive dependencies needs the procedural macro crate?
mfarrugi commentedon Mar 14, 2019
@damienmg the proc-macro could have dependencies, which then need to be compiled for host, not the other way around.
wildarch commentedon Mar 14, 2019
What I meant is a case like the following:
We have crates A, B and C.
A and B are normal crates, C is a procedural macro.
If the dependency graph looks like this:
Then we need to build crate B for both the host and the target.
damienmg commentedon Mar 14, 2019
This is fine, if C is compiled for the execution platform, then their dependencies too. The only thing is that C should be compiled as a host deps.
I don't think this is feasible from the starlark rule context thought. We would have to either declare a different dependencies (which would be weird), or simply do some macro magic although I cannot think of a nice one to do at the moment except if we add a crate_name attribute to rust_library. In that case we would have a
rust_host_librarywith the name of the rust_library and depending on a rust_library_intl with the name "{name}-proc-macro" with a "host" transition. It can then forward it with a provider.Let's wait until @gregestren chime in and tell us all the magic about dynamic configuration and if there is one that allow us to do that without that last ugly hack.
gregestren commentedon Mar 14, 2019
@mfarrugi I'm not familiar with Rust - can you clarify more precisely exactly the condition you want to trigger?
i.e. are you asking if you can make a
rust_librarydynamically set itself tocfg = "host"depending on, say, the value of one of it's attributes? Or if the library's implementation function can dynamically setcfg = "host"for one of its deps?And what's the specific criteria you want to trigger this on?
mfarrugi commentedon Mar 14, 2019
gregestren commentedon Mar 14, 2019
+@juliexxia, @katre
We have the concept of "rule class transitions", which basically lets a rule declare itself as:
But we don't , I believe, have support for
cfg = "host"for that. @katre is doing work that could potentially add that. It's also possible to define_different_configurationto individually set all the flags that host transition would do like--host_cpu,--host_crosstool_top, etc.Or, if Rust rules fully support platforms I think it'd be possible today to define a transition that sets
--platforms=to the host platform? @katre is that realistic or am I missing something?Alternatively, is it really necessary for both target and host deps to be defined on the same attribute? Can you instead have a binary define
depsandhost_depsand have the binary's implementation function check that allhost_depshave a provider exposing "proc-macro"?Finally, having a dep set to host, then have one of its dep set back to target might be difficult.
katre commentedon Mar 14, 2019
It should be possible to use a transition to set the target platform (via the
--platformsflag). Saving the previous value and setting it back might or might not work well depending on what other flags and transitions are happening.gregestren commentedon Mar 14, 2019
Do rust rules already rely on
--platforms?wildarch commentedon Mar 15, 2019
They do! Sounds like the way to go 👍
m3rcuriel commentedon Mar 19, 2019
So I made a quick proof-of-concept that implements the ability to explicitly build crates for host using a 1-1 transition. The problem is that support for attribute-based transitions on rules doesn't exist yet according to the tracking document, but @gregestren can correct me if it's been finished already? With current Bazel features I think we'd have to do it with a separate rule that we can later phase out into just the attribute, or wait for the rust feature.
If we did it with a separate rule we'd have to expose that in cargo-raze as well ¯\(ツ)/¯
wildarch commentedon Mar 19, 2019
@m3rcuriel Cool! Is your POC available somewhere?
m3rcuriel commentedon Mar 19, 2019
Implementation found here. Ideally we would check attrs for crate type and transition based on that but that's the unfinished feature, I suppose.
https://gist.github.com/b44df5e57897c1dfdba658108cf35f71
m3rcuriel commentedon Mar 19, 2019
Almost certain more needs to be done to have rustc handle it properly. WIP.
gregestren commentedon Mar 19, 2019
@juliexxia has been incrementally de-experimentalizing transitions. I can't remember exactly which pieces don't fall under experimental now or what if anything is blocking attribute-based transitions.. She can clarify.
That said, that's a nice example of mixing platforms and transitions - this is a good example of the direction we want all rules to go.
juliexxia commentedon Mar 19, 2019
What's de-experimentalized (i.e. you don't have to pass the
--experimental_starlark_config_transitionsflag to use it) is rule class transitions on native options that don't read configured attributes (i.e. attributes that are set by a select()).The reason for the last restriction, not reading configured attributes, is an overkill way to make sure that if
crate_type, for example, is read byplatform_to_host_transitionto set--platform, then--platformisn't selected on to setcrate_type, since that would create a dependency cycle. In the future we're trying to relax the restriction so it just applies to selecting on the flags we care about (the ones set by rule class transitions) but for now, it's all selecting on the read attributeBut long story short, you should be able to check the attrs for crate_type in the transition. But if any instance of rust_proc_macro (or any rule that uses this rule transition) tries to use a select to set
crate_type, then their build is going to error out. I guess the question is how often you think users will be doing that?m3rcuriel commentedon Mar 19, 2019
https://gist.github.com/6cd8ec00a395dd6c63f63844c810fb66
Working example with
--experimental_starlark_config_transitions.Thanks @juliexxia for clearing up the status of this (and also for all your work on configuration in general! ^_^)
juliexxia commentedon Mar 20, 2019
Sweet! Looking good. Happy to review any PRs writing transitions if you need going forward!
Just to clarify, you should be able to build rules that use the transition without
--experimental_starlark_config_transitionswith the next bazel releasebazelbuild/bazel#6968. Our bazel release sheriff just told me that should go out either friday or early next week.
Up until then, you'll need to pass the flag to build/use the targets.
m3rcuriel commentedon Mar 20, 2019
@wildarch @damienmg is there anything other than building for host that rustc needs to make proc macros work? I'm more experienced with Bazel than Rust so I'm not sure that this actually works or just fixes that compilation.
wildarch commentedon Mar 21, 2019
If this makes procedural macros and all their dependencies build for the host we should be all good!
What would be the path to integrating this, as this currently depends on an experimental flag? Even if bazel implements support for this without the feature flag, distributions may be slow to catch up. Perhaps a PR implementing this could also
fail(..)when it detects that the running Bazel version does not support transitions?damienmg commentedon Mar 21, 2019
I had an idea but it does not work (to define a custom starlark repository that test bazel version and create a bzl file with the
_platform_to_host_transitiondefinition. If the bazel version is too old "platform_to_host_transtion" will be set to "target" simply, else it is set to the function you provided). This idea would need to be able to read the flag value from starlark repository.@juliexxia: is there's any plan to help rule author write rules that offer conditional feature? I know about starlark flag but those would not work in that case where the rule is executed at the loading of the starlark file. Also since when is the
cfgattribute present in therule? This would be great if when browsing the doc one could figure that out.Add proc-macro support from bazelbuild#205
m3rcuriel commentedon Nov 20, 2019
@wildarch / @Collaborators I believe this was fixed in #240 / #272.
Should we get this issue closed?
wildarch commentedon Nov 20, 2019
It looks like this would fix the issue, but I don't have anything to verify with. Maybe it would be good to add a test for this to the CI, and then close this issue once we know for sure it works?