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

Rename python bindings import library for Windows #1165

Merged
merged 2 commits into from
Oct 20, 2022

Conversation

j-rivero
Copy link
Contributor

🦟 Bug fix

Fixes #1150

Summary

There is a naming collision is caused by the both the import library of sdformat library and the import library of the pybind11 bindings library are called sdformat13.lib.

Thanks to @peci1 and @traversaro.

Checklist

  • Signed all commits for DCO
  • Consider updating Python bindings (if the library has them)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

The naming collision is caused by the both the import library of
sdformat library and the import library of the pybind11 bindings library
are called sdformat13.lib

#1150

Thanks to Martin Pecka and Silvio Traversaro.

Signed-off-by: Jose Luis Rivero <[email protected]>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Sep 20, 2022
@peci1
Copy link
Contributor

peci1 commented Sep 20, 2022

Thanks, I'll give it a try later today!

@peci1
Copy link
Contributor

peci1 commented Sep 20, 2022

I'm just curious why the buildfarm did not reveal this earlier...

@traversaro
Copy link
Contributor

I'm just curious why the buildfarm did not reveal this earlier...

The Windows jobs do not build Python bindings at the moment:

[25.859s]    -- pybind11 is missing: Python interfaces are disabled.

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #1165 (393f78a) into sdf13 (c703dee) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            sdf13    #1165   +/-   ##
=======================================
  Coverage   87.26%   87.26%           
=======================================
  Files         125      125           
  Lines       16111    16111           
=======================================
  Hits        14059    14059           
  Misses       2052     2052           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@j-rivero j-rivero changed the title jrivero/fix windows bindings Rename python bindings import library for Windows Sep 20, 2022
@peci1
Copy link
Contributor

peci1 commented Sep 20, 2022

Build succeeds with this fix, and the result is dynamic file named sdformat13.cp310-win_amd64.pyd.

I still can't find a way to actually load the python module (some DLL missing), but at least build is okay now.

@traversaro
Copy link
Contributor

I still can't find a way to actually load the python module (some DLL missing), but at least build is okay now.

What is the exact error? Otherwise you can also use https://lucasg.github.io/Dependencies/ to debug the missing dependencies (just remembre to launch it from inside the conda environment).

@peci1
Copy link
Contributor

peci1 commented Sep 20, 2022

Okay, after manually copying gz-math7.dll, gz-utils2.dll and sdformat13.dll from install/bin to install/lib/python, using the Python bindings works (I did just a little smoke test).

Now the question is how to convince Python to look in the right directory. Since Python 3.8, there is a change in behavior such that loading pyd modules does not look into PATH. All directories to be searched have to be manually added by calling os.add_dll_directory() (https://docs.python.org/3/library/os.html#os.add_dll_directory). What I don't understand is how/where to do that. Inside the pyd file is late, because it would not even start executing (its dlls are not found...). Does that mean each library would need to have a wrapper module that would just call the function with (hardcoded? relative to the wrapper? setup.py? __init__.py?) path and the load the pyd module?

A temporary workaround is setting env var CONDA_DLL_SEARCH_MODIFICATION_ENABLE=1 (see https://docs.conda.io/projects/conda/en/latest/user-guide/troubleshooting.html#numpy-mkl-library-load-failed).

@traversaro
Copy link
Contributor

Okay, after manually copying gz-math7.dll, gz-utils2.dll and sdformat13.dll from install/bin to install/lib/python, using the Python bindings works (I did just a little smoke test).

Now the question is how to convince Python to look in the right directory. Since Python 3.8, there is a change in behavior such that loading pyd modules does not look into PATH. All directories to be searched have to be manually added by calling os.add_dll_directory() (https://docs.python.org/3/library/os.html#os.add_dll_directory). What I don't understand is how/where to do that. Inside the pyd file is late, because it would not even start executing (its dlls are not found...). Does that mean each library would need to have a wrapper module that would just call the function with (hardcoded? relative to the wrapper? setup.py? __init__.py?) path and the load the pyd module?

A temporary workaround is setting env var CONDA_DLL_SEARCH_MODIFICATION_ENABLE=1 (see https://docs.conda.io/projects/conda/en/latest/user-guide/troubleshooting.html#numpy-mkl-library-load-failed).

Good point! I never realized this as I tipically installed in the prefix of the conda environment whenever I develop on Windows, but this is indeed a tricky issue.

@peci1
Copy link
Contributor

peci1 commented Sep 21, 2022

I guess this issue affects all gz libraries with python bindings. Is there a better place to discuss it than this PR?

@traversaro
Copy link
Contributor

I guess this issue affects all gz libraries with python bindings. Is there a better place to discuss it than this PR?

https://github.com/gazebosim/gz-math is the library lowest in the stack with python bindings, perhaps it could make sense to discuss it there?

Copy link
Contributor

@peci1 peci1 left a comment

Choose a reason for hiding this comment

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

Okay, I've created gazebosim/gz-math#507. Apart from that issue, I approve this PR.

@j-rivero
Copy link
Contributor Author

Okay, I've created gazebosim/gz-math#507. Apart from that issue, I approve this PR.

Thanks Martin and Silvio for the efforts to resolve the issue and the information about this. It was totally new to me.

@scpeters scpeters merged commit b5fb1ea into sdf13 Oct 20, 2022
@scpeters scpeters deleted the jrivero/fix_windows_bindings branch October 20, 2022 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Windows conda build broken by using versioned python modules
4 participants