-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
get_python_inc returns wrong include directory for bundled Pythons #178
Comments
What is the value of Do you have any samples you can share? |
It is 'None' both before and after. |
@rnhmjoj Would you be able to analyze or help zjp analyze what's going on to characterize the missed expectation? |
Sorry, but I'm just a very stubborn guy that wanted to cross compile python packages, bisected all the way to that commit and fixed an obvious logical inconsistency: I know nothing more about this project or how the python build system works. |
@zjp, would you be able to give instructions on how to reproduce this error? Is there a way for me to get a copy of your bundled python that I could run? |
@isuruf You can download the daily build of ChimeraX here: https://www.cgl.ucsf.edu/chimerax/download.html We give users access to a Python shell to script the application as well as a limited interface to interact with pip. I dragged a copy of the 1.5 daily build to my desktop on an M1 Mac laptop (this just cuts down the time to open the app dramatically). I then opened an ipython shell by going to the top menu bar: Tools -> General -> Shell.
Then in ChimeraX's own command line I typed Afterwards:
|
I just found that the same 'Fix, again, finding headers during cross compiling' broke Yocto builds - which is a fairly important cross compile case :-) You can see the newly occuring fails here: I'm investigating. |
Seems like there are bogus paths in our configs that this change exposes. So the problem seems to be on our side, you don't need to do anything. |
In that issue, wqh17101 reports that the issue can be attributed to precedence defined at: distutils/distutils/sysconfig.py Lines 136 to 140 in 22b9bcf
In particular, they propose giving precedence to This proposed approach is untenable, because I believe kanavin's finding sheds some light on the issue. I suspect that users encountering this issue may have a local configuration that's triggering this code path. I suggest to look to find the source of It seems to me that the issue encountered by wqh17101 may be different than that encountered by zjp, because zjp is reporting that the custom include path is being overridden by a system one. Anyone still experiencing the issue, can you report the output of:
(or execute the Python in a Python interpreter). |
Output from the distutils bundled with setuptools both before and after the commit referenced in this ticket made it into setuptools' vendored distutils.
|
|
Thanks both for that information. Good news is I can see from that information that You can find where that value comes from by doing the following:
That file is where INCLUDEPY should be defined. That value was generated when that copy of Python is built. Perhaps the issue is that when cross-compiling, the target Python version isn't taking precedence. Maybe what needs to happen is the cross-compiled Python needs to supply the Can someone try replicating the issue in a Dockerfile? |
For what it's worth, we (yocto) have the same cross compile problem (whether to use build host vs target specific sysconfigdata), and solve it by patching python to look at the host version of it, unless there is a magic unix environment variable. It's not super elegant, but I do not think upstream python currently offers a better option. |
Actually, even patching is not necessary; I just looked into it properly, and pointing python to a completely non-default sysconfigdata can be done thusly prior to starting python ($STAGING_LIBDIR is a yocto specific variable, adjust as needed):
|
This change in behaviour caused breakage for us too (when upgrading from setuptools 65.1.1 to 65.2.0, which includes #173). In our case, the Python install has been relocated from its original install Python itself handles this fine, since it dynamically adjusts As such, distutil's The issue is that after #173, the order of precedence here favours the path returned by distutils/distutils/sysconfig.py Lines 137 to 139 in 3e9d47e
...even though the path returned by It seems the options to fix this might be:
|
Or how about not having a bogus value in the config? You can fix the one you have, or you can supply a correct custom one as shown above. |
So the "bogus value" in the config, is presumably coming from the As a workaround, we can try and rewrite the config (or say set
|
I'm not so sure that doing automatic guessing is the correct approach. I like being able to specify things explicitly in config files and have those specifications take precedence, because we can and do need to change those things when cross-compiling python and various python-based items for example, and in that case guessing by native python that is running on the build host only gets in the way. |
As for your situation, how about simply deleting the config file that has bogus stuff in it before you package up python? If something breaks then, then it should be fixed up to fall back to reasonable built-in (or guessed from executable binary location) defaults. |
I will explore that option too, thank you. Other alternatives I am considering:
Checking so far, it seems |
See also discussion on the breaking change in setuptools: |
I would lean toward changing the order of precedence to prefer |
This also impacts the version of Python included in Xcode if installed under a different name than |
Several people have suggested to change the order of prececedence, but if that's done, it effectively disables the includes from config (as
What about removing the values of sysconfigdata that are leading to the incorrect config?
These proposals may be worth exploring. I'm still not convinced that it should be distutils' responsibility to accommodate misconfigured environments. I'd prefer instead for the environments to be properly configured so that distutils can simply honor the configuration. Before we explore these options, let's show definitively that misconfigured environments are to be expected (i.e. that there isn't a reliably way for a portable Python environment to be configured). |
Also probably related: pypa/setuptools#3786 |
This could be considered a python bug in |
I'm finally able to replicate this issue after discovering that python-build-standalone is affected:
|
In #200, I've added a test and implemented the proposed workaround to only return the configured dir if it exists. |
Thanks for looking into this. I have one follow up question: now that a fix is in place, should the order of precedence be changed to prefer the nearest Python? I patched my copy of distutils to include your change, then built a wheel with ChimeraX (which uses a portable Python). I also have a system Python on my mac at /Library/Frameworks. We don't want users to use subtly different versions of Python to build wheels that may have binary extensions, just ours. After applying the fix, I still see that Python.h in /Library is preferred over Python.h inside the ChimeraX distribution (though, after I think it makes sense to try and get the header associated with the Python that's been invoked instead of looking for system headers first. |
Bumps [setuptools](https://github.com/pypa/setuptools) from 67.1.0 to 67.2.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pypa/setuptools/blob/main/CHANGES.rst">setuptools's changelog</a>.</em></p> <blockquote> <h2>v67.2.0</h2> <p>Changes ^^^^^^^</p> <ul> <li><a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/3809">#3809</a>: Merge with distutils@8c3c3d29, including fix for <code>sysconfig.get_python_inc()</code><code>pypa/distutils#178</code><code>has_function</code><code>pypa/distutils#195</code></li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pypa/setuptools/commit/f75c6289666ae845b80f1666ea68ec0f28afcc73"><code>f75c628</code></a> Bump version: 67.1.0 → 67.2.0</li> <li><a href="https://github.com/pypa/setuptools/commit/bb17d4f9b23b3b8d670271f36d8894239295efe3"><code>bb17d4f</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/3809">#3809</a> from pypa/distutils-8c3c3d29</li> <li><a href="https://github.com/pypa/setuptools/commit/47c7cfd67862e65eb19dca8b60094db0fe203049"><code>47c7cfd</code></a> Add changelog (draft)</li> <li><a href="https://github.com/pypa/setuptools/commit/43c6ee4b2b2ce4637bb1a36400fbaa548150ec8b"><code>43c6ee4</code></a> Merge <a href="https://github.com/pypa/distutils">https://github.com/pypa/distutils</a> into distutils-8c3c3d29</li> <li><a href="https://github.com/pypa/setuptools/commit/8c3c3d29bda85afb7f86c23a8ba66f7519f79e88"><code>8c3c3d2</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/199">#199</a> from mrbean-bremen/issue3591</li> <li><a href="https://github.com/pypa/setuptools/commit/1efd3d54c39aab7f960f7934298fbab4cff205be"><code>1efd3d5</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/201">#201</a> from mattip/pypy2</li> <li><a href="https://github.com/pypa/setuptools/commit/854862f228aa727b8ad88e1c432027767aff9db1"><code>854862f</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/203">#203</a> from fweimer-rh/ccompiler-windows</li> <li><a href="https://github.com/pypa/setuptools/commit/8fe7b5f2eee8a1eea501be3635e6e7af9553db6d"><code>8fe7b5f</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/202">#202</a> from GalaxySnail/fix-mingw-w64-2</li> <li><a href="https://github.com/pypa/setuptools/commit/7e751d33d42de2ce9bdb08dcd0a1bd5d5b85eed9"><code>7e751d3</code></a> distutils.ccompiler: Remove correct executable file on Windows</li> <li><a href="https://github.com/pypa/setuptools/commit/2f16327060394b21fadb6342ea83eec324512aaa"><code>2f16327</code></a> Mark test as xfail on Windows. Ref <a href="https://github-redirect.dependabot.com/pypa/distutils/issues/195">pypa/distutils#195</a>.</li> <li>Additional commits viewable in <a href="https://github.com/pypa/setuptools/compare/v67.1.0...v67.2.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=setuptools&package-manager=pip&previous-version=67.1.0&new-version=67.2.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>
…version 67.2.0 Dimitri Papadopoulos (8): Add `tests/test*.py` to source distributions Apply refurb suggestions Apply refurb suggestions Apply refurb suggestions Apply refurb suggestions Apply refurb suggestions Update outdated GitHub Actions Fix typos found by codespell Florian Weimer (2): distutils.ccompiler: Make has_function work with more C99 compilers distutils.ccompiler: Remove correct executable file on Windows GalaxySnail (2): Fix MinGW-w64 segmentation fault Fix MinGW-w64 segmentation fault Hugo van Kemenade (1): Link directly to PEPs Jason R. Coombs (7): Add xfail test capturing missed expectation. Ref pypa/distutils#178. In _get_python_inc_posix, only return an extant path from the sysconfig. Fixes pypa/distutils#178. ⚫ Fade to black. Revert "Merge pull request #197 from GalaxySnail/fix-mingw-w64" Mark test as xfail on Windows. Ref pypa/distutils#195. Add changelog (draft) Bump version: 67.1.0 → 67.2.0 mattip (1): add a pypy CI run mrbean-bremen (1): Fixed accumulating include dirs after compile
I opened a new issue. |
My group maintains a project which bundles Python and so we compile packages with our bundled Python and not the system Python on our machines to ensure a stable environment for users. After #173,
sysconfig.get_python_inc()
always returns the Python headers in/Library/Frameworks
instead of the header directory relative to our bundled Python.The text was updated successfully, but these errors were encountered: