-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
Include/cpython/pystate.h contains non-relative of initconfig.h include causing macOS Framework include failure #83207
Comments
The cpython/pystate.h includes cpython/initconfig.h using the relative path "cpython/initconfig.h", which probably works fine if your include path explicitly contains the top of the python directory, however when developing with a framework in macOS, the framework's root path cannot be referred to relatively. Since cpython/pystate.h and cpython/initconfig.h live in the same directory, any C compiler should include them correctly, regardless of include path if the cpython/pystate.h includes "initconfig.h" instead of "cpython/initconfig.h", since I believe the very first path is always relative to the file including the next file. In this case, cpython is the parent of pystate.h and thus including initconfig.h directly should work fine. Previous 3.x versions worked fine, but the cpython directory wasn't in Headers for macOS. Although I wasn't able to exhaustively test this on all platforms and with all compilers, changing the include in cpython/pystate.h to "initconfig.h" solved the compilation/include problem. |
In what way does it not work when using a framework build? Is this when you add the python framework to an (Xcode) project and include the main python header using "#include <Python/Python.h>"? |
I should have been more specific. Sorry about that. Yes, when including <Python/Python.h> in Xcode, incorporating the framework. In particular, I'm embedding 3.8 in an application (previously used the system Framework, which was 2.7, which worked but Apple has warned that it may disappear....). The Xcode error is: In file included from /Users/gaige/MyApp/Code/PythonImporter.m:10:
In file included from ../python-framework/Python.framework/Headers/Python.h:121:
In file included from ../python-framework/Python.framework/Headers/genobject.h:11:
In file included from ../python-framework/Python.framework/Headers/pystate.h:129:
../python-framework/Python.framework/Headers/cpython/pystate.h:10:10: fatal error: 'cpython/initconfig.h' file not found
#include "cpython/initconfig.h"
^~~~~~~~~~~~~~~~~~~~~~
1 error generated. python-framework is in the framework search path. Let me know if there's something further I can clarify. |
IMO a better approach would be to add "cpython_" prefix to all header files living in Include/cpython/. It would prevent bad surprises. I tried to avoid adding a prefix when I added Include/internal/ but I got many practical issues. Depending where the #include is done, I got different errors. Adding a prefix prevents this issue. |
Ok, I think I see. Is there a concern that removing the cpython/ prefix might lead to the wrong initconfig.h being included? So your proposal is all includes in the root will do #include "cpython/cpython_foo.h" And any includes done in the cpython dir will be #include "cpython_foo.h" Currently there's only one file in the cpython dir that includes another, which would explain why this has never come up before. |
I propose to rename "cpython/initconfig.h" to "cpython/cpython_initconfig.h". #include "cpython/initconfig.h" would become: #include "cpython/cpython_initconfig.h" So it becomes possible to include a cpython_xxx.h header from another cpython_xxx.h header (as done py cpython/pystate.h which includes cpython/initconfig.h). -- Maybe a simpler change is simply to remove #include "cpython/initconfig.h" from cpython/pystate.h. Currently, cpython/pystate.h is included indirectly by Python.h at line 137, whereas cpython/initconfig.h is included by Python.h at line 135 (two lines before). C extensions must include "Python.h" first and when "Python.h" is used, pystate.h gets the cpython/initconfig.h definitions as expected. |
Ok, so there are three potential solutions
1 intoduces verbosity and touches more than the other solutions. 2 I'd appreciate your guidance on which path is preferable. On Mon, 18 May 2020, 23:08 STINNER Victor, <report@bugs.python.org> wrote:
|
Let's start with that and see if it's enough to fix the issue. |
I marked bpo-40642 as a duplicate of this issue. |
I'm not sure why Include/cpython/ is part of the compiler include directories. I am not sure why Include/ is *not* part of the compiler include directory. Anyway, it's hard to control how people build Python and there are always legit use cases where you fall into such issue. That's why I suggest to fix any risk of confusion by renaming Include/cpython/ header files to add a "cpython_" prefix. For example, rename Include/cpython/pystate.h to Include/cpython/cpython_pystate.h. In the Include/internal/ directory, header files already have a prefix for the exact same reason. For example, there is a pycore_pystate.h file there. |
I propose to only rename header files in Python 3.11. For Python 3.9 and 3.10, I suggest to modify Include/cpython/pystate.h by replacing:
#include "cpython/initconfig.h"
with:
#include "initconfig.h" Like the PR 20181. |
I wrote a different fix: PR 29488. Can someone please check if it fix the issue with Xcode? |
I'll try to check that in the next day or two. |
Gaige Paulsen: "I'll try to check that in the next day or two." Thanks. Did you have the opportunity to give it a try? |
Short version is not yet. I spent time on it but ran into an issue getting the full build on the machine I was building on. I expect to try it again before the weekend. Is there a binary copy of the framework from the CI somewhere? If so, I can certainly do a much faster check against building with the framework, saving me the step I'm having problem with right now. |
Sorry about the delay. Result: I can confirm that the compilation problem is resolved with this change (and was failing with the main branch prior to the change). |
Gaige Paulsen: "Result: I can confirm that the compilation problem is resolved with this change (and was failing with the main branch prior to the change)." Thank you for the confirmation. I merged my change and I'm backporting it to Python 3.9 and 3.10. |
I can test the backports if you like. In that case, let me know when the backports are done, I should be able to test those rapidly, now that I have my build environment working for building from source. Thanks again! |
I think #72798 broke Windows builds: python_uwp.cpp https://dev.azure.com/Python/cpython/_build/results?buildId=92032&view=results |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: