pyo3-build-config: fix windows "cross-compile" panic

[davidhewitt]

Possible fix for #2229

[messense]

Actually it's linux not windows now.

[adamreichold]

Actually it's linux not windows now.

I think this could still work, i.e. host and target are x86_64-unknown-linux-gnu in your test case which would have been rejected for cross compiling due to the host == target condition when writing but not when reading the configuration.

[messense]

Here we correctly force cross compilation if PYO3_CROSS_LIB_DIR is set

pyo3/pyo3-build-config/src/lib.rs

Line 73 in 01788b7

} else if impl_::any_cross_compiling_env_vars_set() {

But in here the logic is a little different

pyo3/pyo3-build-config/src/lib.rs

Line 180 in 01788b7

} else if let Some(interpreter_config) = impl_::make_cross_compile_config()? {

[adamreichold]

Yeah, we should probably bite the bullet and properly unify those code paths. @davidhewitt If you are busy I can cook up something here or elsewhere factoring out the logic for checking for cross compilation and creating that configuration?

[adamreichold]

But in here the logic is a little different

I suspect that is actually necessary because of cases where we do detect cross-compilation, but do not have the environment variables set. I think this fix here only takes care of the other way around?

[messense]

I suspect that is actually necessary because of cases where we do detect cross-compilation, but do not have the environment variables set.

Yeah it's necessary, I think it only missed that it should also do cross compilation when the environment variables are set.

[davidhewitt]

I think we should probably unify those code paths a bit, I have some ideas.

I'm enjoying having a go at this at the moment - might take me a few gaps through the day. Got a bit of a routine beginning to form. If things get busy later I might push, run, and ping you @adamreichold 😅

[messense]

BTW cross compilation when host and target architectures are the same should be supported, we have an issue in the past: #1513

#2230 should be included in CI when we fix this.

[adamreichold]

#2230 should be included in CI when we fix this.

I think it would make sense to cherry-pick that commit here so that we have only a single PR.

[davidhewitt]

I think it would make sense to cherry-pick that commit here so that we have only a single PR.

Agreed, I'll cherry-pick once windows job fixed.

[davidhewitt]

Think this is getting there now.

@adamreichold I'm going to be busy for a few hours or so now, if you're around and want to finish this off feel welcome. Otherwise I'll hopefully tidy up this evening.

[davidhewitt]

Looks great, let's merge? I'm hoping we can tidy the coverage later in #2191