improve cross build configure in build.rs #27
Conversation
build.rs
Outdated
| @@ -76,7 +76,7 @@ fn build(metadata: &Metadata) { | |||
|
|
|||
| // If we're cross-compiling, let configure know. | |||
| if metadata.host != metadata.target { | |||
| configure_args.push(format!("--host={}", metadata.target)); | |||
| configure_args.push(format!("--host={}", metadata.host)); | |||
There was a problem hiding this comment.
I'm afraid that this does not look correct to me. GNU autotools uses "host" and "target" differently than Cargo: https://github.com/lu-zero/autotools-rs/blob/27d0437e071ddbf68ff0bbacfa02e01a04cc6b42/src/lib.rs#L71-L94
There was a problem hiding this comment.
Thanks for the link, I wasn't aware of this difference before.
Also I digged bit more on that, noticed that the zigbuild actually set CC_<triplets> and AR_<triplets> environment variables in, that are not passed all the way down to configure and make, so it was actually using the host tool & target to compile it into a macos target. Updated the build.rs to pass these when set.
gen-xu
changed the title
There was a problem hiding this comment.
I think this looks about right, but historically we've pushed the responsibility for setting CC and AR correctly onto the person doing the compile—e.g.:
CC=my_cross_cc AR=my_cross_ar cargo build
This is a departure from that philosophy. Probably a worthwhile one, as you're not the first person to struggle getting this crate to cross compile, but a departure nonetheless.
I can get on board with the autodetection of CC and AR if there's a target triple version in the env, but blindly setting the configure tests to yes still feels sketchy to me. Before jumping all the way in to automatically forcing those settings to yes, I'd prefer to instead document in the README of the crate what configure tests will need to be overridden, and how to do so (krb5_cv_attr_constructor_destructor=yes ac_cv... cargo build ...)
build.rs
Outdated
| .env("krb5_cv_attr_constructor_destructor", "yes") | ||
| .env("ac_cv_func_regcomp", "yes") | ||
| .env("ac_cv_printf_positional", "yes") |
There was a problem hiding this comment.
We've historical held the position that it is inappropriate to set these flags in this crate, as we don't actually know that the target system supports these features that configure is checking—they're in theory something that whoever's doing the cross compile needs to set for themselves.
That said, it does seem like the answer is yes for basically all the cross-compiling targets that anyone cares about these days...
I think the reason why the cargo-zigbuild is setting imo I don't think this is necessarily a departure of that philosophy, because zigbuild is the tool to setting without the zigbuild, one still needs to set those variables to invoke I do agree that setting those flags are a bit inappropriate, maybe we could provide a variable flag for user to tweak those behaviors, or even better to check compiler support directly without compiling a test program whenever possible? |
|
@benesch based on the conversation I removed the default environment variable set to skip the test, but providing hints to user to skip these checks correspondingly. |
Automatically set CC/AR environment variables when invoking configure/make if `CC_<target>`/`AR_<target>` are available in the environment.
Sounds good. Sorry for the delay here. I pushed up a revision of your patch that refactors things to my taste. Thanks very much for putting this together! Will merge if/when CI goes green. |
this addresses #26