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

Add PYO3_CROSS_NO_ENABLE_SHARED variable #1532

Closed
wants to merge 1 commit into from

Conversation

ravenexp
Copy link
Contributor

@ravenexp ravenexp commented Mar 29, 2021

It allows to override the default value of Py_ENABLE_SHARED
definition in pyconfig.h when cross-compiling for Windows.

Equivalent to defining Py_NO_ENABLE_SHARED when building
a C program statically linking the Python interpreter.

@ravenexp ravenexp force-pushed the add-cross-no-shared branch from f5463a5 to 1a63a7b Compare March 29, 2021 09:22
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.

Thanks for implementing this. I have a couple of comments regarding the copy.

Also, for this to be useful for users we would need to remove the cfg-gates I put in gil.rs:

#[cfg(all(Py_SHARED, not(PyPy)))]

pyo3/src/gil.rs

Line 140 in f97aac6

#[cfg(all(Py_SHARED, not(PyPy)))]

However I'm a bit worried about doing so. I added those because we got a lot of users having problems with statically-linked tests. 😄

build.rs Outdated Show resolved Hide resolved
guide/src/building_and_distribution.md Show resolved Hide resolved
@ravenexp
Copy link
Contributor Author

ravenexp commented Mar 30, 2021

Also, for this to be useful for users we would need to remove the cfg-gates I put in gil.rs:
...
However I'm a bit worried about doing so. I added those because we got a lot of users having problems with statically-linked tests.

Sorry, I don't quite understand why it is gated on Py_SHARED and not for example on py_sys_config = "WITH_THREAD".

I grepped the current CPython sources for Py_ENABLE_SHARED and Py_NO_ENABLE_SHARED uses, and it seems to be mostly dealing with __declspec(...) shenanigans in the Windows build. It also affect the default module search path on Windows: apparently it is relative to the Python DLL location. I couldn't find any uses that would affect the Unix-like builds.

EDIT: I've checked the Python 3.6 sources as well, and nothing seems to have changed since then.

@davidhewitt
Copy link
Member

Yes, those #[cfg] are not motivated by upstream cpython limitations. I put them there because embedding the Python interpreter statically accidentally has caused lots of problems for users in the past.

See original PR in #1347

@davidhewitt
Copy link
Member

.... I am wondering if perhaps the right thing to do is remove those #[cfg] attributes and fix this instead with better documentation. I think it's still correct to disable auto-initialize with static linking as that'll prevent most of the accidents, however we could alter the error message for this case to be better - especially now that the auto-initialize feature is opt-in.

I'll try to put together a PR for this case tonight.

@ravenexp
Copy link
Contributor Author

I still can't figure out why is it illegal to call Py_InitializeEx(0) when statically linking to the interpreter library.

But assuming it is, then Py_SHARED can only be disabled for the extension modules. But then it is completely useless on Windows because loading extensions requires the dynamically-linked Python. Then this PR does not make sense to begin with.

@davidhewitt
Copy link
Member

davidhewitt commented Mar 30, 2021

I still can't figure out why is it illegal to call Py_InitializeEx(0) when statically linking to the interpreter library.

Apologies, I'm communicating poorly - it is fine to call Py_InitializeEx(0) with a statically embedded Python. But users often then run code which imports other extensions, and encounter issues because the statically embedded Python does not contain all symbols.

Give me a few days to try out some experiments and write some documentation in a new PR. Then I will come back to re-review this PR.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is OK, but let's wait a bit for David to come up with a good document.

build.rs Show resolved Hide resolved
@davidhewitt
Copy link
Member

Ok, so as per #416 (comment) I was able to prove that linking a static python can work. It is however super awkard so not something we can support first-class in PyO3 at the moment.

Given that this PR affects windows, I'm going to try to repeat a similar experiment on Windows and see what makes sense to change / put into documentation.

@ravenexp
Copy link
Contributor Author

ravenexp commented Apr 7, 2021

Given that this PR affects windows, I'm going to try to repeat a similar experiment on Windows and see what makes sense to change / put into documentation.

AFAIK on Windows the extensions can't import symbols from the embedding executable because the Python DLL name gets hardcoded into the extension modules. One could theoretically build a fake python3y.dll that forwards all its symbols to the user executable (that's how abi3 extensions are implemented on Windows: python3.dll forwards to python3y.dll), but I can't imagine anyone actually doing it. When you are compiling the interpreter statically, you have to compile in the Python standard library extensions statically as well. And loading third-party extensions at the run time is out.

@ravenexp ravenexp force-pushed the add-cross-no-shared branch from 4001d4a to c0988f6 Compare April 8, 2021 06:29
@davidhewitt
Copy link
Member

👍 that sounds about right based on the reading I've been doing too.

Thanks for this; I'm satisfied let's merge it and I'll add a follow-up PR shortly to unlock more pyo3 functionality when linking statically.

build.rs Show resolved Hide resolved
It allows to override the default value of `Py_ENABLE_SHARED`
definition in `pyconfig.h` when cross-compiling for Windows.

Equivalent to defining `Py_NO_ENABLE_SHARED` when building
a C program statically linking the Python interpreter.
@ravenexp
Copy link
Contributor Author

Obsoleted by #1553

@ravenexp ravenexp closed this Apr 11, 2021
@ravenexp ravenexp deleted the add-cross-no-shared branch March 18, 2022 10:48
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.

4 participants