search

MaterializeInc / rust-krb5-src

Rust build system integration for libkrb5, MIT's Kerberos implementation
Apache License 2.0
6 stars 9 forks source link

improve cross build configure in build.rs #27

Closed gen-xu closed 1 month ago

gen-xu commented 3 months ago

this addresses https://github.com/MaterializeInc/rust-krb5-src/issues/26

gen-xu commented 3 months ago

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 commented 3 months ago

@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 commented 1 month ago

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