gh-82909: Update PC/pyconfig.h to allow disabling pragma based auto-linking #19740
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@zooba Thanks for your patience, let me know if you need anything else to move forward with the integration 🙏 |
@jcfr Seems like a release note could be added in the meantime. |
|
Ping? Any update here? It'd be nice to have this in 3.9. |
|
Seems this will miss 3.10 as well. |
|
Ping. This would be really really nice to have sometime soon 👍 |
|
I thought we'd already added this? I guess if it's not there, then we should. Definitely needs a NEWS entry, probably deserves a What's New as well. Not sure if there's any place in the documentation that would suit, but if so, we should mention it for sure. |
I can definitely add an entry, What about the |
|
I added draft documentation and the NEWS entry for the macro. However, they need improvements. |
|
@arhadthedev Thanks for your help moving this forward 🙏 |
|
I haven't tested the commands added to the docs, but if they do work as described and produce the files, then this looks fine to me. |
|
It seems that if building Python by oneself, python's lib file will not be found in virtual environment. Hope this issue fixes as quickly as possible |
|
No, virtual environment is only for Python libraries. You'll need to look in |
|
@zooba My question is like this:rosinality/stylegan2-pytorch#325 (comment) So, is it more like to be a question about venv? |
|
It looks like Torch is deciding those settings, and it's deciding wrong about where to find the libs files. There's a revamp of |
|
@zooba I can give some additional informations. For exmaple, And that's maybe why dll file cannot be found? |
|
Yeah, many tools are not going to work well when running from a CPython build directory. It's an unsupported configuration. Try using the PC/layout tool (e.g. |
|
Any progress here? I see conflicts, so at least a rebase is in order. It'd be nice to get this into 3.13 at least… @vstinner Interested in shepherding this C API change at all? |
|
A rebase ought to be enough, and when someone confirms that the build commands shown in the new documentation are correct then this can be merged (as per my last message). |
|
This would potentially be extremly helpful, any chance that this will get merged at some point? |
|
It looks like the rebase isn't coming, so someone will need to submit a new PR. It also looks like my last hesitation was that the commands in the docs weren't tested. |
…linking Define PY_NO_LINK_LIB to build extension disabling pragma based auto-linking. This is relevant when using build-system generator (e.g CMake) where the linking is explicitly handled
There was a problem hiding this comment.
Nothing wrong with the PR as it is, but I think we can do a bit of tidying here if you want.
| #ifdef MS_COREDLL | ||
| # if !defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_BUILTIN) | ||
| /* not building the core - must be an ext */ | ||
| # if defined(_MSC_VER) | ||
| # if defined(_MSC_VER) && !defined(Py_NO_LINK_LIB) |
There was a problem hiding this comment.
This whole set of conditions is pretty ugly... any interest in simplifying?
Ideally in this order:
- check if the feature is explicitly disabled (new variable)
- check if we're in a context where it should be implicitly disabled (
Py_BUILD_CORE[_BUILTIN]) - check if the compiler supports it (
_MSC_VER)
The order doesn't need to be spelled out to anyone reading it, but it should make it easiest to read (the first check has the best name) and easiest to maintain (alternate compilers can be supported in the "middle", not the edges).
It also really doesn't need to be indented by 8 characters. Four would be plenty.
| /* Define Py_NO_LINK_LIB to build extension disabling pragma | ||
| based auto-linking. | ||
| This is relevant when using build-system generator (e.g CMake) where | ||
| the linking is explicitly handled */ |
There was a problem hiding this comment.
This comment isn't important for this file, though a short header above the entire block (something like "automatically reference python3x.lib") might make it easier to understand the purpose without reading the whole way through.
|
Guess we're not getting those improvements, so we'll leave them for later. Should be an easy first contribution for a sprint. |
|
Thanks everyone for the help moving this forward 🙏 |
| :file:`Python.h` triggers an implicit, configure-aware link with the | ||
| library. The header file chooses :file:`pythonXY_d.lib` for Debug, | ||
| :file:`pythonXY.lib` for Release, and :file:`pythonX.lib` for Release with | ||
| the `Limited API <stable-application-binary-interface>`_ enabled. |
Define
PY_NO_LINK_LIBto build extension disabling pragma basedauto-linking. This is relevant when using build-system generator
(e.g CMake) where the linking is explicitly handled
https://bugs.python.org/issue38728