-
Notifications
You must be signed in to change notification settings - Fork 38
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
Enable Python bindings in conda-forge CI and fix Python bindings compilation and tests on MSVC #380
Conversation
MSVC is failing with this error:
|
The problem seems that the pybind11 bindings for LeggedOdometry references some |
Now that tests are compiling, they were not running correctly. The problem was that in Windows there is no RPATH. This is tipically workarounded by placing all the |
Due to #382, I switched the Windows Conda CI from VS generator to Ninja. |
After this, the CI is finally green. This is now ready for review, if necessary I can rework the commit to match the three main changes summarized in first post, otherwise we can just merge with squash. |
@prashanthr05 your review was mainly for the commit 9ddee6f . |
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.
I trust you and CI on Windows, I don't have better ideas about how to solve it nicely. I'm just wondering, would the tests succeed against the installed tree (inside the active site-packages folder) instead of the build tree stored in the build folder? Maybe in this case the PATH is already configured system-wide.
Yep, in that case everything would work fine also for Multi-Config generators, and the copy of dll will be not necessary, but also not problematic. |
Ok clear, thanks! In Python ⩾ 3.8 I guess an alternative workaround would be using |
I tried to use that, but it appeared not to work, not sure why (something related to conda builds?) or if I did something wrong. Tweaking the |
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.
Thank you @traversaro to deal with this
Could you please reorganise the commits has you wrote here #380 (comment) ? |
6fe2416
to
99f00c7
Compare
Done! |
Since the need of using this workaround for importing modules on Windows has been kind of reverted in conda, I would not be surprised that the usage of the function does not behave as it should. Thanks for the additional details! |
The macos failure shouldn't be related to this PR. Am I correct? |
Pcl version in brew updated 14 hours ago Homebrew/homebrew-core@b2e4d3b |
Exactly, the same failure is in the scheduled job: https://github.com/dic-iit/bipedal-locomotion-framework/actions/runs/1077070309 . |
Since this PR is not related to the failure we decided to merge the PR |
LeggedOdometry::resetEstimator
overloads that actually belonged to the parent classFloatingBaseEstimator
.