moveit-core: Fixes for Windows, and CI for macOS and Windows

[wolfv]

Description

Hi Moveit team!

This PR has the fixes we had to apply in the RoboStack project for noetic (http://github.com/robostack/ros-noetic). In the RoboStack project we build conda packages for ROS on all major OS's: macOS, Windows and Linux.

This PR mostly applies patches by @seanyen from Microsoft.

Using the conda packages from RoboStack also makes it easy to set up a cross-platform CI which we've done in this PR as well.

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

[welcome]

Thanks for helping in improving MoveIt and open source robotics!

[wolfv]

pinging @simonschmeisser and @Tobias-Fischer

ref: #2548

[simonschmeisser]

Please give some more info why you need to change what things. If you separate out trivial commits like the missing dependencies those could maybe also be merged earlier separately

[wolfv]

@simonschmeisser thanks for having a look! I think there are not so many really "deep" changes in this PR. I will fix the CMake so that it works with both the Conda-provided packages and Ubuntu / ROS packages.

Besides the near stuff, the biggest change is adding some "export" flags to properly export some symbols / variables in Windows DLLs. But that's also a fairly mechanical change.

[wolfv]

By the way, the working build / test for moveit core is executed here: https://github.com/RoboStack/moveit/actions/runs/751121040

[tylerjw]

The pre-commit format test is failing on this. Fortunately that is really easy to fix. You just need to install pre-commit (using python3 pip) and clang-format-10 (using apt or however you'd like to install it) locally and run pre-commit run -a in the repo and it should fix all formatting errors. For errors it can't fix it'll just output the result of the various linters it runs (most are auto formatting).

[tylerjw]

Some minor comments. Once this is ready we should run the pre-release test on it also to try to make sure none of these changes will affect a future release.

[tylerjw]

It looks like there is now a catkin_lint error:

moveit_core: CMakeLists.txt: error: unconfigured build_depend on 'pluginlib'
       * You declare a build dependency on another package but neither
       * call find_package() nor have it listed as catkin component in
       * the find_package(catkin) call.

[Tobias-Fischer]

Hmm, I'm not sure what this error is about:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites tests="3" failures="1" disabled="0" errors="0" timestamp="2021-04-15T21:19:46" time="22.616" name="AllTests">
  <testsuite name="MoveGroupTestFixture" tests="3" failures="1" disabled="0" errors="0" time="22.616">
    <testcase name="PathConstraintCollisionTest" status="run" time="6.016" classname="MoveGroupTestFixture">
      <failure message="/home/runner/work/moveit/moveit/.work/target_ws/src/moveit/moveit_ros/planning_interface/test/move_group_interface_cpp_test.cpp:255&#x0A;      Expected: planning_scene_interface_.getObjects().size()&#x0A;      Which is: 1&#x0A;To be equal to: std::size_t(0)&#x0A;      Which is: 0&#x0A;Google Test trace:&#x0A;/home/runner/work/moveit/moveit/.work/target_ws/src/moveit/moveit_ros/planning_interface/test/move_group_interface_cpp_test.cpp:155: PathConstraintCollisionTest" type=""><![CDATA[/home/runner/work/moveit/moveit/.work/target_ws/src/moveit/moveit_ros/planning_interface/test/move_group_interface_cpp_test.cpp:255
      Expected: planning_scene_interface_.getObjects().size()
      Which is: 1
To be equal to: std::size_t(0)
      Which is: 0
Google Test trace:
/home/runner/work/moveit/moveit/.work/target_ws/src/moveit/moveit_ros/planning_interface/test/move_group_interface_cpp_test.cpp:155: PathConstraintCollisionTest]]></failure>
    </testcase>
    <testcase name="CartPathTest" status="run" time="12.1" classname="MoveGroupTestFixture" />
    <testcase name="JointSpaceGoalTest" status="run" time="4.5" classname="MoveGroupTestFixture" />
  </testsuite>
</testsuites>

[codecov]

Codecov Report

Merging #2604 (7907d5f) into master (097583d) will decrease coverage by 0.01%.
The diff coverage is 29.32%.

❗ Current head 7907d5f differs from pull request most recent head 7d621a3. Consider uploading reports for the commit 7d621a3 to get more accurate results

@@            Coverage Diff             @@
##           master    #2604      +/-   ##
==========================================
- Coverage   60.47%   60.46%   -0.00%     
==========================================
  Files         402      404       +2     
  Lines       29659    29669      +10     
==========================================
+ Hits        17932    17937       +5     
- Misses      11727    11732       +5     

Impacted Files	Coverage Δ
...n/allvalid/collision_detector_allocator_allvalid.h	100.00% <ø> (ø)
...collision_detection/collision_detector_allocator.h	100.00% <ø> (ø)
...ction_bullet/collision_detector_allocator_bullet.h	100.00% <ø> (ø)
...n_detection_fcl/collision_detector_allocator_fcl.h	100.00% <ø> (ø)
...ield/collision_detector_allocator_distance_field.h	0.00% <ø> (ø)
...stance_field/collision_detector_allocator_hybrid.h	100.00% <ø> (ø)
...e/include/moveit/kinematics_base/kinematics_base.h	74.08% <ø> (ø)
...ene/include/moveit/planning_scene/planning_scene.h	53.13% <ø> (ø)
...del/include/moveit/robot_model/fixed_joint_model.h	100.00% <ø> (ø)
.../include/moveit/robot_model/floating_joint_model.h	100.00% <ø> (ø)
... and 24 more

---

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 097583d...7d621a3. Read the comment docs.

[Tobias-Fischer]

Cool, seems like all checks are passing now :)

[tylerjw]

Cool, seems like all checks are passing now :)

Now I just need to follow up with that small PR I promised. I'll ping you when that is done.

[tylerjw]

@v4hn Here is my PR to change the interface to a simple getter: RoboStack#1 Please review that PR to see if you'd like me to change any part of it before it becomes part of this PR?

[wolfv]

I tried my best to re-order and squash the commits into a meaningful history. Please let me know what you think!

I am happy to change it further.

[v4hn]

If we actually add this new cross-platform workflow, it should probably remain an optional feature for the foreseeable future.

CI setups tend to break often due to bit rot. Does anyone of you plan to maintain that workflow and provide fixes if it breaks or do you expect the moveit maintainer team to take over? I believe there is currently no maintainer with an intrinsic motivation to fix Windows builds.

Were these questions discussed at a maintainer meeting in the past?

[wolfv]

Hi @v4hn, I am happy to rename the environment file.
I am also happy to volunteer to keep this CI pipeline running for the foreseeable future, and I think @Tobias-Fischer is also happy to help out.

It's a similar situation to the one we're in where we need to keep patches up-to-date in the robostack repositories, which is quite a bit of work and could be drastically reduced if we could get more people to adopt the packages in the CI infrastructure.

I think supporting (at least CI-wise) macOS and Windows is a great thing, and as you can see the issues related to windows compatibility were not so big after all. Having the CI would mean that it wouldn't accidentally be broken again in the future.

I am happy to join a maintainer meeting if there is one if you want me to chime in.

Also, I am also happy to invite anyone from the MoveIt team to join our little RoboStack get-together (maybe one of those dates work out for you?): https://doodle.com/poll/fi4298qgedknicg3

[Tobias-Fischer]

I can confirm that I'd be happy to maintain the RoboStack CI. It'd be great to see you guys at our get together!

[traversaro]

Ok, in that case I support Robert's request for autogenerated files. If possible, please implement this so we can finally merge this request.

Perfect, thanks! @wolfv let me know if you want to proceed with your effort that mentioned earlier in the PR or you like me to add generate_export_header support.

[wolfv]

@traversaro if you could help me out here it would be much appreciated :) My current efforts look like this: https://github.com/RoboStack/moveit/compare/tests_with_conda_pkgs...RoboStack:gen_headers?expand=1

But didn't have the time to test it properly yet (and looks like CI fails).

[traversaro]

@traversaro if you could help me out here it would be much appreciated :) My current efforts look like this: https://github.com/RoboStack/moveit/compare/tests_with_conda_pkgs...RoboStack:gen_headers?expand=1

But didn't have the time to test it properly yet (and looks like CI fails).

The fixes for that branch to work correctly are provided in RoboStack#3 .

addressed

[v4hn]

Assuming CI passes (I'll not wait for the last three jobs anymore today), this looks good to merge.
Please rebase the commits to remove the fixups and the auxiliary branch name in the CI config.

I would prefer to have this merged via merge commit as a number of semi-independent changes are also fixed here.

[wolfv]

I've just rebased, removed the very last commit (that re-activated building on our branch) and attributed the CMake changes correctly to @traversaro.

Would be awesome to see this merged. Thanks for all the helpful reviews!

[welcome]

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

[v4hn]

Thank you for the contributions and your promise to take care of the new CI job in the future (and/or expand it to more than moveit_core).
This has been lying around for long enough, so I'll merge it now to avoid other PRs breaking it again.

[wolfv]

thanks for merging!