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 symlink-install compilation #32

Merged
merged 4 commits into from
Apr 16, 2021
Merged

Conversation

youliangtan
Copy link
Member

@youliangtan youliangtan commented Apr 14, 2021

Issue

This fix is partly to address the potential issue when build with --symlink-install. open-rmf/rmf#24.

Fix

Previously the rmf_fleet_adapter_python will invoke a cmake build from setup.py. This PR will remove the need of setup.py, and install the pkg via ament_python_install_package() in the CMakeList.

Also, the package ament_python is removed, and replaced with pybind11_vendor. This will be installed during rosdep.

Test

# Compile, with and without `--symlink-install`
ros2 run rmf_fleet_adapter_python test_adapter.py

-- in-progress --

  • Pending colcon test

Signed-off-by: youliang <tan_you_liang@hotmail.com>
@youliangtan youliangtan added this to In Review in Research & Development via automation Apr 15, 2021
@luca-della-vedova
Copy link
Member

This fixes the symlink issue for me as well, I guess we should get colcon test to work first? Does the rclpy matching code help?

@youliangtan youliangtan marked this pull request as ready for review April 15, 2021 08:41
@youliangtan
Copy link
Member Author

Added the test 86b4a7f

colcon test --packages-select rmf_fleet_adapter_python --event-handlers console_direct+

Last but not least, once this is merged, users will need to re-run rosdep update & install sequence to ensure pybind11-vendor is locally installed.

@luca-della-vedova
Copy link
Member

Added the test 86b4a7f

colcon test --packages-select rmf_fleet_adapter_python --event-handlers console_direct+

Last but not least, once this is merged, users will need to re-run rosdep update & install sequence to ensure pybind11-vendor is locally installed.

Awesome! I tried building / testing / sourcing and importing the python bindings and it's working.
I agree that needing rosdep update / install is not ideal but that shouldn't be an issue for binary builds, while for source builds we can point it out to users.

@youliangtan youliangtan merged commit 5cbef9b into main Apr 16, 2021
Research & Development automation moved this from In Review to Done Apr 16, 2021
@youliangtan youliangtan deleted the fix/adapter-py-compilation branch April 16, 2021 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants