Fix include directory when cross compiling

[jameshilliard]

Fixes #9119.

[alex]

I'm concerned this may have the practical effect of reverting aa8dbd8, particularly since we have no ability to test any of these cross-compilation patterns in CI.

Is there any reasonable way to validate this works correctly?

[jameshilliard]

I'm concerned this may have the practical effect of reverting aa8dbd8, particularly since we have no ability to test any of these cross-compilation patterns in CI.

I think it's probably not going to cause issues as headers is from my understanding only set for a subset of python cross compilation configurations(it doesn't look like yocto sets it).

Is there any reasonable way to validate this works correctly?

I'm not all that familiar with how yocto handles python cross compilation here.

[alex]

Perhaps a dumb question, but I don't see headers mentioned in https://docs.python.org/3/library/sysconfig.html, is this an official thing?

[jameshilliard]

is this an official thing

It's set here I think.

[alex]

Ah, so it's merely under-documented.

…

On Fri, Jun 23, 2023 at 6:44 PM James Hilliard ***@***.***> wrote: is this an official thing It's set here I think. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[alex]

@kanavin can you confirm that this PR doesn't have a negative impact on your cross-compilation?

[kanavin]

On our setup 'headers' is not present at all:

>>> import sysconfig; print(sysconfig.get_paths())              
{'stdlib': '/usr/lib/python3.11', 'platstdlib': '/usr/lib/python3.11', 'purelib': '/usr/lib/python3.11/site-packages', 'platlib': '/usr/lib/python3.11/site-packages', 'include': '/usr/include/python3.11', 'platinclude': '/usr/include/python3.11', 'scripts': '/usr/bin', 'data': '/usr'}

Reading the code for sysconfig, 'headers' entry is only present if python is called from its own source/build directory, and so shouldn't be relied on for building other items, such as this one :) It's not documented for the same reason.

[kanavin]

I'd also like to ask @jameshilliard , how do you get the sysroot prefix from the build host into these paths reported by sysconfig? Yocto doesn't do it, assuming sysconfig should be reporting target paths. Rather, we patch the sysroot prefix into specific variables in _sysconfigdata.py. Python cross-compile is a mess.

[kanavin]

I found it:
https://github.com/buildroot/buildroot/blob/master/package/python3/0004-Adjust-library-header-paths-for-cross-compilation.patch
and
https://github.com/buildroot/buildroot/blob/master/package/pkg-python.mk#L45

I don't have a clear idea yet how we can meet in the middle, need to research further.

[kanavin]

Okay, sorry for the flood.

@alex The more I look at it, the more I'm convinced that "sysconfig.get_path ('include')" was correct all along. I'd suggest at this point that my change is reverted as well - both yocto and buildroot need to figure out how to make that work without having to patch components such as this one.

[jameshilliard]

Reading the code for sysconfig, 'headers' entry is only present if python is called from its own source/build directory, and so shouldn't be relied on for building other items, such as this one :) It's not documented for the same reason.

Yeah...I think to some degree we're treating our 3rd party python package builds somewhat like python stdlib module builds. I recall that approach was the least broken and that this patch was related.

Python cross-compile is a mess.

Very true, and it's largely undocumented as well with everyone handling things slightly differently, rust is pretty bad at cross compilation as well considering setting env variables with names like __CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS are effectively mandatory to work around cargo linker bugs when cross compiling with stable rust toolchains.

I don't have a clear idea yet how we can meet in the middle, need to research further.

Well if headers isn't normally defined...then this shouldn't cause side effects right?

@alex The more I look at it, the more I'm convinced that "sysconfig.get_path ('include')" was correct all along. I'd suggest at this point that my change is reverted as well - both yocto and buildroot need to figure out how to make that work without having to patch components such as this one.

Yeah, I agree sysconfig.get_config_var('INCLUDEPY') does not really look right to me, I think using the headers variable when present is probably harmless as it's not normally defined at all. I'm still somewhat confused exactly why this issue only started popping up with 6c39999 is that the same commit that introduced the build issue for yocto as well?

[reaperhulk]

It popped up in that commit because we fundamentally changed how we build and link openssl with that work. We needed to do this so we could link rust-openssl against the same copy of openssl that we use for the cffi layer.

[kanavin]

I submitted a revert PR to my change. Yocto will carry the fix locally and mark it as unsuitable for upstream submission. (and we need to seriously look into cross-building python in a better way).

I cannot agree that using undocumented API is harmless, 'headers' or otherwise. It's problematic for two reasons:

• anyone reading the code will have a hard time understanding what the undocumented api call does and why is it there. You'd need to research commit history, PR comments, and python code which implements undocumented stuff, and hope you can grasp all of it and make things clear for yourself.

• python upstream can change the behavior behind undocumented items at any point, and then there's suddenly a bug in components using it with no clear resolution, as adapting to new behavior will break installations of python with old behavior.

I'd suggest that this change stays local to buildroot as well, as it is very much specific to that environment (running python out of its build tree) and is unlikely to be useful elsewhere.

[jameshilliard]

It popped up in that commit because we fundamentally changed how we build and link openssl with that work.

Was this include directory not being used anywhere in the build previously? I'm wondering if there's a different way of pulling this directory that wouldn't trigger this issue that is being used in other places.

anyone reading the code will have a hard time understanding what the undocumented api call does and why is it there.

Well anything related to python cross compilation tends to be poorly or entirely undocumented in general, I updated the PR to add a comment to indicate how headers is used.

python upstream can change the behavior behind undocumented items at any point, and then there's suddenly a bug in components using it with no clear resolution, as adapting to new behavior will break installations of python with old behavior.

That's not all that clear, it seems to be used by distutils/setuptools based on what I'm seeing here in some situations which would indicate that the behavior should be semi-stable as long as it's used similar to distutils/setuptools.

I'd suggest that this change stays local to buildroot as well, as it is very much specific to that environment (running python out of its build tree) and is unlikely to be useful elsewhere.

To clarify we're not actually running python out of its build tree for these 3rd party module builds, we're setting the sysconfig environment up similar to how module builds from within the python build tree work from my understanding but still running the host python binary itself from its out of tree location.

[alex]

Was this include directory not being used anywhere in the build previously? I'm wondering if there's a different way of pulling this directory that wouldn't trigger this issue that is being used in other places.

Previously the include directory was being found somewhere inside of setuptools logic. My preference would be to just reproduce whatever it's doing (that's what we thought we were doing with the sysconfig usage, but perhaps there's a subtly we're missing).

[jameshilliard]

Previously the include directory was being found somewhere inside of setuptools logic.

It looks like it might be here, not entirely sure I've traced this properly however.

[alex]

Note that the sysconfig there is _not_ the stdlib sysconfig module, its distutils.sysconfig. This looks plausible though, I think we'd be happy to take a patch that emulated this.

…

On Sat, Jun 24, 2023 at 10:10 PM James Hilliard ***@***.***> wrote: Previously the include directory was being found somewhere inside of setuptools logic. It looks like it might be here, not entirely sure I've traced this properly however. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[jameshilliard]

I think we'd be happy to take a patch that emulated this.

Seems easier and probably more reliable to just pass the include_dirs via an env variable from setuptools-rust like this.

[alex]

@jameshilliard @kanavin can you both confirm that this latest version works for your setups? Seems like it should, which will be great, and we can put this cursed issue to bed :-)

[jameshilliard]

can you both confirm that this latest version works for your setups?

Yeah, seems to work for me.

[kanavin]

Works for me as well, thanks!