improve cross build configure in build.rs

[gen-xu]

this addresses #26

[benesch]

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

[gen-xu]

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.

[benesch]

Thanks for the PR! Unfortunately I don't think this is the right approach; left you a line comment with details.

[benesch]

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 ...)

[benesch]

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...

[gen-xu]

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 ...)

I think the reason why the cargo-zigbuild is setting CC_<target_with_underscores> instead of just CC is coming from the convention in rust's cc crate
https://github.com/rust-lang/cc-rs/blob/eb3390615747fde57f7098797b2acb1aec80f539/src/lib.rs#L97
that this seems to be used pretty in common for cross-build

imo I don't think this is necessarily a departure of that philosophy, because zigbuild is the tool to setting CC_<target_with_underscores> and AR_<target_with_underscores> before invoke cargo for the user

without the zigbuild, one still needs to set those variables to invoke CC_<target_with_underscores>=... AR_<target_with_underscores>=... cargo build, where this PR only adds an extension to take the convention used by rust's cc crate.

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?

[gen-xu]

@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.

[benesch]

@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.

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.