Skip to content
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

Remove pyconfig.h header parsing #1521

Merged
merged 2 commits into from
Mar 28, 2021
Merged

Conversation

ravenexp
Copy link
Contributor

The config header parsing code was supposed to be only invoked when
cross-compiling for Windows, but in reality it fails to correctly parse
the config header files shipped with the upstream Python for Windows.

Given that there are now better options for reliable cross-compiling
for Windows such as PYO3_CROSS_PYTHON_VERSION or the abi3-py3* features,
it should be OK to remove this config for v0.14.

Update the cross-compilation instructions section of the user guide.

Fixes #1337

The config header parsing code was supposed to be only invoked when
cross-compiling for Windows, but in reality it fails to correctly parse
the config header files shipped with the upstream Python for Windows.

Given that there are now better options for reliable cross-compiling
for Windows such as `PYO3_CROSS_PYTHON_VERSION` or the `abi3-py3*` features,
it should be OK to remove this config for v0.14.

Update the cross-compilation instructions section of the user guide.

Fixes PyO3#1337
@davidhewitt
Copy link
Member

Thanks, yes this is probably fine to go, however it does lead to a possible situation where a user trying to cross-compile for a very unusual windows build might not be able to configure pyo3 as they need.

I've been conisidering a PR which enables users to provide exact configuration externally (maybe as JSON), but didn't finish it yet. We might want to consider adding that before merging this, just incase anyone needs the flexibility.

@ravenexp
Copy link
Contributor Author

I agree that a configuration tool which is independent of the target Python installation would be nice to have,
but unfortunately pyconfig.h parsing as it presently exists is of little help here.

Mainly because it is only capable of parsing autoconf/autoheader generated headers, but the Windows Python port does not use these tools. It instead supplies a hand-written config header containing multiple #ifdef clauses and whatnot, which the current PyO3 code can't possibly make sense of.

One such example is the Py_ENABLE_SHARED definition.
#1359 makes the problem even worse by assuming Py_ENABLE_SHARED is not set when the parser fails, which is simply wrong when targeting Windows.

In the case someone customizes their Python build on Windows, they are supposed to define some magic preprocessor macros like Py_NO_ENABLE_SHARED, which in turn trigger the #ifdef clauses inside pyconfig.h. But then this decision is not represented in the config header file contents at all. As such, Py_NO_ENABLE_SHARED is only mentioned in the comments.

@kngwyu
Copy link
Member

kngwyu commented Mar 26, 2021

I think it's OK to remove this if we can prepare a more flexible solution until the next release.

I've been conisidering a PR which enables users to provide exact configuration externally (maybe as JSON),

Maybe [package.metadata] is better?

@ravenexp
Copy link
Contributor Author

Maybe [package.metadata] is better?

This must be a per-target setting, and the target triple is supplied externally via --target or CARGO_BUILD_TARGET.
Environment variables are currently being used for this kind of configuration.

Can we tolerate another 3-4 PYO3_CROSS_* variables?
Using JSON will likely introduce a build dependency on serde and serde_json, which take quite a long time to compile.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if it were an JSON file it would be an environment variable specifying where to find the JSON.

Using JSON will likely introduce a build dependency on serde and serde_json, which take quite a long time to compile.

I was thinking I would make this "advanced" build configuration opt-in via a feature :)


Regarding this PR specifically, I'm now convinced that leaving the header parsing in causes more headaches than it solves. Users who really need advanced configuration can always patch their local PyO3 temporarily!

@kngwyu
Copy link
Member

kngwyu commented Mar 27, 2021

I merged main to fix CI.
BTW, we can use nanoserde as a lightweight (but limited) alternative of serde.

@ravenexp
Copy link
Contributor Author

I've looked through the Windows pyconfig.h contents once more, and I'm now fairly convinced that there are only two switches the downstream builders can flip without modifying the vanilla CPython sources:

  • define Py_NO_ENABLE_SHARED which maps to !Py_SHARED in PyO3
  • define _DEBUG which maps to Py_DEBUG in PyO3

Defining _DEBUG does not enable Py_TRACE_REFS though, so the debug Python builds should remain ABI-compatible, at least for version 3.7+.

Disabling WITH_THREAD is not supported without modifying the headers manually, this fact is also mentioned here

pyo3/build.rs

Line 296 in 3bc5caa

// WITH_THREAD is always on for Python 3.7, and for PyPy.

As a follow-up, I propose adding PYO3_CROSS_NO_ENABLE_SHARED environment variable, which can enable users to statically link the Python interpreter library into their Rust programs for Windows. It doesn't make sense for the extension modules though, and should be ignored with a warning when "extension-module" feature is enabled.
I'll try to implement it myself and submit a new PR in the nearest future.

IMO, as far as cross-compiling for Windows is concerned, the JSON configuration feature may be an overkill.

@davidhewitt
Copy link
Member

As a follow-up, I propose adding PYO3_CROSS_NO_ENABLE_SHARED environment variable, which can enable users to statically link the Python interpreter library into their Rust programs for Windows. It doesn't make sense for the extension modules though, and should be ignored with a warning when "extension-module" feature is enabled.
I'll try to implement it myself and submit a new PR in the nearest future.

Note that the last time I experimented with this, I concluded linking the Python interpreter statically doesn't work very well (probably not until we have some upstream support). When linking binaries and tests statically Rust seems to trim out the symbols that aren't used directly by the program, so when other Python extension modules are imported by the statically-linked binary, often there are missing symbol errors. E.g. #742

So it may not be worth your effort implementing that variable. Thanks for looking though!

@ravenexp
Copy link
Contributor Author

I was thinking about programs embedding a Python interpreter for application-level scripting, just like Lua and Tcl interpreters are commonly used. Loading Python extension modules is not a requirement in this case, because it would allow to escape the user script sandboxing...

@davidhewitt
Copy link
Member

Mmm. This would include parts of the Python standard library which are implemented as C extension modules!

@kngwyu kngwyu merged commit 24b0000 into PyO3:main Mar 28, 2021
@ravenexp
Copy link
Contributor Author

As a side note, apparently it is possible to link in the standard library modules statically, as described here:

https://wiki.python.org/moin/BuildStatically

@ravenexp ravenexp deleted the remove-header-parsing branch March 29, 2021 05:13
ravenexp added a commit to ravenexp/pyo3 that referenced this pull request Mar 29, 2021
PYO3_CROSS_VERSION was renamed to PYO3_CROSS_PYTHON_VERSION.

PYO3_CROSS_INCLUDE_DIR was removed in
PyO3#1521
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build.rs fails to parse hand-written pyconfig.h headers installed by python for windows (MSVC-compiled)
3 participants