-
Notifications
You must be signed in to change notification settings - Fork 156
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 the build of the Python bindings. #387
Fix the build of the Python bindings. #387
Conversation
8651c4d
to
420e630
Compare
Thank you for the contribution @robquill - this could make sense, but could you expand what this flag would do in context of the broader codebase? Asking as if it's more of a corner case for a particular use-case, it may make more sense adding this on the respective project cmake file as opposed to by default to the repo to all targets |
My understanding is that building a shared object requires position-independent code (https://stackoverflow.com/questions/57649452/why-must-shared-libraries-be-position-independent-while-static-libraries-dont), so I could try only setting this option if -DKOMPUTE_OPT_BUILD_PYTHON=ON is set. I don't think you can only set it e.g. for the Pybind11 module itself as that shared library is still built using the static libraries which would still contain position dependent code. |
Signed-off-by: Robert Quill <robert.quill@imgtec.com>
Doing so is a partial fix for building with -DKOMPUTE_OPT_BUILD_TESTS=ON -DKOMPUTE_OPT_BUILD_PYTHON=ON but there are still undefined symbols at link time. Building either the tests or the Python bindings works correctly. Building both at the same time still does not work. Signed-off-by: Robert Quill <robert.quill@imgtec.com>
Position-independent code is only required when building a shared object. Signed-off-by: Robert Quill <robert.quill@imgtec.com>
56ee2b1
to
4bdfdb4
Compare
From my understanding position independent code is glowingly the default in more modern distros, and it may indeed be the case that restricting it to only -DKOMPUTE_OPT_BUILD_PYTHON=ON may be the way to go, as I can't think of a corner case at this stage (unless someone wants to build static vs dynamic version of python although haven't tried if possible) |
OK then, that is already done, so I think this should be good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
When attempting the build the Python bindings like this:
I get this error: