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

Fix linking to the wrong Python library for Debug built type on Windows #514

Merged
merged 6 commits into from
Nov 14, 2024

Conversation

duburcqa
Copy link
Contributor

@duburcqa duburcqa commented Nov 2, 2024

No description provided.

@jcarpent jcarpent requested a review from jorisv November 4, 2024 09:23
@jorisv
Copy link
Contributor

jorisv commented Nov 4, 2024

Hello @duburcqa,

Thank for the PR.
When I read wrap_python.hpp and your PR title I'm a little confuse. Did this PR allow to:

  • Build our Python modules in debug mode and be loaded by a Python release runtime on Windows ?
  • Build our Python module in debug mode and be loaded by a Python debug runtime on Windows ?
  • Both ?

@duburcqa
Copy link
Contributor Author

duburcqa commented Nov 4, 2024

The point is rather linking with the Python library that Boost::Python wants us to link with. The current implementation in Eigenpy is by-passing Boost::Python logics entirely. Even worst, only compilation units that includes "include/eigenpy/numpy.hpp" are affected, but not the others, which means that the whole project may end up linking against multiple Python libraries at the same time.

To answer for question, it allows both, depending on whether the pragma definition "BOOST_PYTHON_DEBUG" is set. However, in practice what we want is linking with the release Python library even in debug, which is the default with Boost::Python but not with barebone Python (and hence not with numpy either).

@jorisv
Copy link
Contributor

jorisv commented Nov 14, 2024

Hello @duburcqa,

I don't really get why an header will change our linking behavior.
As far as I know, when building a Python module with Boost.Python we don't link at all against Python.
Instead, we will link with Python library when the module is loaded by the Python interpreter.

If we want to embed the Python interpreter into our application, we need to link with libpython. Bug again, the libpython will be chosen by the build script (usually CMake).

Did you have some document that explains your point ?

I will try to test the Windows Debug behavior.

@duburcqa
Copy link
Contributor Author

duburcqa commented Nov 14, 2024

I don't really get why an header will change our linking behavior.

That is just how it is. This information can be found at several places in the documentation, for instance here or here.

By the way, I can ensure you that my patch is working on real-world deployment.

@jorisv
Copy link
Contributor

jorisv commented Nov 14, 2024

Thank for the link. The first one explain well the issue.
I also find this link that explain well what changed in the debug build and that since Python 3.8 there is no ABI incompatibility between debug and release Python build.
And the related source code.

So on Windows, pyconfig.h (included by Python.h) can call some MSVC specific pragma to link against a DLL.
It use the _DEBUG define to choose which library to link to. But this define is used commonly and we can then link against the wrong library.
To avoid this issue boost/python/detail/wrap_python.hpp undef _DEBUG and define it back after including Python.h.

I was able to reproduce the issue and the patch fixed it.

I will modify the comment to precise it's a Windows specific issue.

@jorisv jorisv enabled auto-merge November 14, 2024 13:30
@jorisv jorisv merged commit 56e3381 into stack-of-tasks:devel Nov 14, 2024
33 of 34 checks passed
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.

2 participants