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

small compilation fixes for macOS #271

Merged
merged 6 commits into from
Sep 15, 2020
Merged

Conversation

mamoll
Copy link
Contributor

@mamoll mamoll commented Sep 1, 2020

Description

Below are misc. small fixes to get MoveIt2 to compile on macOS 10.15.6. Everything ROS2-related is compiled from source.

I mostly followed the official ROS2 and MoveIt2 installation directions. A couple small hiccups:

  • I had to do this:
# for catkin:
ln -s /usr/local/bin/python3 /usr/local/bin/python
# for moveit
brew install ompl
  • I had to remove urdfdom*, which was pulled in by MoveIt2 installation instructions, but also existed inside the ros2 directory.
  • I had to clone a bunch of extra repos:
git clone -b ros2 git@github.com:wg-perception/object_recognition_msgs.git
git clone -b ros2 git@github.com:OctoMap/octomap_msgs.git
git clone -b foxy-devel git@github.com:ros-controls/control_msgs.git
git clone -b ros2 git@github.com:ros/eigen_stl_containers.git
git clone -b ros2 git@github.com:ros-planning/random_numbers.git
git clone -b ros2 git@github.com:ros/angles.git
git clone -b ros2_devel git@github.com:ros-controls/realtime_tools.git
git clone git@github.com:ros/xacro.git
git clone git@github.com:ros/catkin.git
git clone git@github.com:ros/roslint.git

The --symlink-install flag for colcon still causes issues. I had to run colcon like so:

colcon build --catkin-cmake-args -DCMAKE_BUILD_TYPE=Release -DCMAKE_OSX_SYSROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk

(See also https://www.robotandchisel.com/2020/08/10/rviz2-on-mac/.) After many hours this errors out, but after source install/setup.bash running colcon again succeeds.

The Ogre include path fix seems ugly. I don't know what the proper fix is. If you look at install/rviz_ogre_vendor/opt/rviz_ogre_vendor/CMake/OGREConfig.cmake you see:

get_filename_component(OGRE_INCLUDE_DIRS "${OGRE_PREFIX_DIR}/include/OGRE" ABSOLUTE)

Note that /OGRE part. If that wasn't there, the include path fixes wouldn't be needed.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@mikeferguson
Copy link
Contributor

ln -s /usr/local/bin/python3 /usr/local/bin/python

What is the error message you get if you don't do this? Is it really catkin having a problem, or is it some scripts inside MoveIt2? I originally did this until I update the shebangs on my various scripts (which, for ROS2, we should really be targeting Python3 anyways).

The --symlink-install flag for colcon still causes issues

I've seen this even on Ubuntu 20.04 with latest debs.

Copy link
Contributor

@mikeferguson mikeferguson left a comment

Choose a reason for hiding this comment

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

This is really exciting, I'm gonna have to try this out.

@@ -149,7 +149,7 @@ bool MoveGroupCartesianPathService::computeService(
constraint_fn = boost::bind(
&isStateValid,
req->avoid_collisions ? static_cast<const planning_scene::PlanningSceneConstPtr&>(*ls).get() : nullptr,
kset->empty() ? nullptr : kset.get(), _1, _2, _3);
kset->empty() ? nullptr : kset.get(), boost::placeholders::_1, boost::placeholders::_2, boost::placeholders::_3);
Copy link
Contributor

Choose a reason for hiding this comment

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

broader question: why are we using boost::bind instead of std::bind? Is there a lower-level library requiring it? If not, seems like we should update MoveIt2 to reduce our boost usage (core ROS2 has done so).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... or use lambda's.

Copy link
Contributor

@mikeferguson mikeferguson Sep 1, 2020

Choose a reason for hiding this comment

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

The only downside to lambdas is that I'm not sure they are as broadly understood - they seem a little more "advanced". In various discussions I've recently had with ROS/ROS2 package maintainers, I've noticed there is pretty decent evidence that the more advanced the codebase, the harder it is to get community help.

Copy link
Member

@henningkayser henningkayser Sep 4, 2020

Choose a reason for hiding this comment

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

I agree that we should switch to lambdas in cases where the function is trivial (i.e. log message and return of a result) or where another member function should be called but we need to preprocess the arguments. Otherwise,std::bind is often much shorter and more readable.

@mamoll
Copy link
Contributor Author

mamoll commented Sep 1, 2020

ln -s /usr/local/bin/python3 /usr/local/bin/python

What is the error message you get if you don't do this? Is it really catkin having a problem, or is it some scripts inside MoveIt2? I originally did this until I update the shebangs on my various scripts (which, for ROS2, we should really be targeting Python3 anyways).

I am pretty this is due to my PYTHON_PATH setting:

export PYTHONPATH=/Users/mmoll/Library/Python/3.8/lib/python/site-packages

I hate that Home Brew and pip both install in /usr/local by default. Two package managers for potentially the same files (numpy, PyQt, etc.) seems like a terrible idea, so I configured pip to install in my user directory. Some python3 packages use
syntax that catkin (which uses #!/usr/bin/env python) cannot handle when using python2.

@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #271 into main will decrease coverage by 0.00%.
The diff coverage is 41.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #271      +/-   ##
==========================================
- Coverage   47.87%   47.87%   -0.01%     
==========================================
  Files         154      154              
  Lines       15748    15749       +1     
==========================================
  Hits         7540     7540              
- Misses       8208     8209       +1     
Impacted Files Coverage Δ
..._ros/robot_interaction/src/interaction_handler.cpp 0.00% <0.00%> (ø)
...it_ros/robot_interaction/src/robot_interaction.cpp 0.00% <0.00%> (ø)
...ics_plugin_loader/src/kinematics_plugin_loader.cpp 24.65% <50.00%> (ø)
...nning_scene_monitor/src/planning_scene_monitor.cpp 39.88% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d95e4ae...2d689ba. Read the comment docs.

@henningkayser
Copy link
Member

@mamoll changes look good to me. Can you confirm that the latest MGI port works on macOS as well?

@henningkayser henningkayser changed the base branch from master to main September 4, 2020 12:38
@mamoll
Copy link
Contributor Author

mamoll commented Sep 7, 2020

I merged in the latest changes from the main branch (specifically the merged MGI port) last Friday and everything still compiles.

@davetcoleman
Copy link
Member

davetcoleman commented Sep 15, 2020

Can we merge this now @mamoll ? Feel free to do the honors, you have 2 approvals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants