sdformat icon indicating copy to clipboard operation
sdformat copied to clipboard

Rename python bindings import library for Windows

Open j-rivero opened this issue 1 year ago • 11 comments

🦟 Bug fix

Fixes https://github.com/gazebosim/sdformat/issues/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

  • [x] Signed all commits for DCO
  • [x] 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.

j-rivero avatar Sep 20 '22 16:09 j-rivero

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

peci1 avatar Sep 20 '22 16:09 peci1

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

peci1 avatar Sep 20 '22 16:09 peci1

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.

traversaro avatar Sep 20 '22 16:09 traversaro

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.

codecov[bot] avatar Sep 20 '22 16:09 codecov[bot]

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.

peci1 avatar Sep 20 '22 20:09 peci1

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).

traversaro avatar Sep 20 '22 21:09 traversaro

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).

peci1 avatar Sep 20 '22 22:09 peci1

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.

traversaro avatar Sep 21 '22 09:09 traversaro

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

peci1 avatar Sep 21 '22 09:09 peci1

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?

traversaro avatar Sep 21 '22 09:09 traversaro

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.

j-rivero avatar Sep 21 '22 17:09 j-rivero