-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
CPython: Use shared by default on Windows #23648
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
@RubenRBS @uilianries Can an exception be made to the hook? (please see above for reasoning). If not (but you still agree), perhaps I could trick the hook by using |
@Ahajha thanks for the PR and the reasoning behind it (Along with the one in Slack!) |
Hi @Ahajha - thanks for this! I'm actually surprised by this! Not so much on Windows - but I thought that a statically-linked-python-runtime-into-the-interpreter was actually widely used in some distros (Debian based) and Conda? Historically:
I think other distros did not follow the path - apparently there's a flag you can pass at compile time to also get performance benefits for the shared one as well, see this thread: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/NWPVQSKVWDKA75PDEIJNJIFL5C5SJXB2/ Just a curiosity - if the static variant doesn't work on Windows, I would make it an invalid option in |
@jcar87 Thanks for the context, I was not aware (though also not surprised) that there's more to the story for non-Windows platforms. I'll do some reading and think about that case separately. I think I recall there being a lengthy discussion in nanobind about... something related (see here wjakob/nanobind#360 (comment), may or may not be related). As for Windows, static builds are already invalid, but I think it provides for a poor user experience for the default state to be invalid. Every single user would have to set it to shared anyways, we might as well set that as the default. So perhaps maybe it should only be default shared on Windows only? |
@jcar correct me if I'm wrong: So from my rough understanding, the difference between shared and static is that the static package ships the binary with a statically linked libpython. For our purposes, we also need to ship the static library, since it's a "generic" package that can also be used for embedding. The shared package ships an exe linked to libpython dynamically, again with both shipped. The part that I don't quite understand is where it would be necessary for a C extension to link to libpython, but I'm not sure if that's relevant here. I think I'm okay with leaving non-Windows platforms as static in this case, after reading about the performance increase that comes with static. |
We could possibly add def config_options(self):
if self.settings.os == "Windows":
self.options.shared = True to set the default to True for Windows only. |
@valgur I think that's the change I'll end up going with. I'll push that later today. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@jcar87 Can you take another look at this one? |
Conan v1 pipeline ✔️All green in build 4 (
Conan v2 pipeline ✔️
All green in build 4 (
|
Specify library name and version: cpython/all
(See python/cpython#110234 for more context, I will summarize below)
On Windows, static builds haven't worked since 3.10. Additionally, when using static builds (in the versions that support it), the extension modules aren't built, making the package less useful. Due to this, it makes sense to make shared the default on Windows, otherwise the default state would be either invalid or not very useful. (Generally, static Windows builds are for specific use cases.)
This will also fix/avoid issues like this one which arise from static being the default. I've also seen a few other PRs pop up (now that this recipe is usable) which set shared to true, so that provides some more evidence, plus some places to clean up just a little.