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

Fixes for Windows #530

Merged
merged 63 commits into from
Jul 30, 2021
Merged

Fixes for Windows #530

merged 63 commits into from
Jul 30, 2021

Conversation

Ace314159
Copy link
Contributor

@Ace314159 Ace314159 commented Jul 1, 2021

Description

With this PR, MoveIt2 builds on Windows, and I'm able to use it for some basic planning. Here's a summary of all the changes:

  1. Since there is no pkg-config on Windows, I had to find FCL and Bullet using find_package. Using vcpkg, the name for FCL is FCL instead of LIBFCL, so I also had to modify it to get it to work with that.
  2. Because static variables are not exported by the global export I had to add manual visibility control to these libraries:
    • moveit_collision_detection
    • moveit_collision_detection_fcl
    • moveit_collision_detection_bullet
    • moveit_kinematics_base
    • moveit_planning_scene
    • moveit_mesh_filter
    • moveit_planning_scene_monitor
    • moveit_warehouse
    • moveit_planning_pipeline
    • moveit_trajectory_execution_manager
    • moveit_move_group_interface
    • moveit_ros_robot_interaction
    • moveit_planning_scene_rviz_plugin
  3. The zlib component for boost is no longer required for iostreams
  4. RDF loader uses popen and pclose, but the Windows equivalents are _popen and _pclose
  5. MoveIt ROS warehouse connector uses fork and kill, but there's no equivalent fork and kill on Windows (as far as I'm aware), so I just disabled it on Windows and log an error when it's used
  6. Windows adds some extra defines like near, far, and max that are used as variable names. Using undef fixes these errors. Also, uint doesn't work on MSVC, so I replaced that to unsigned int
  7. BenchmarkExecutor does include winsock2.h on Windows, but it also needs to link with ws2_32.lib to build
  8. In collision detection fcl and bullet, the tests are built with some compiler flags (-Wno-deprecated-declarations) that don't work on Windows, so I didn't add those flags on Windows.
  9. I had to add a couple of includes (deque and boost/bind.hpp) in some places to get some things to compile
  10. I changed the include from pluginlib/class_list_macros.h to pluginlib/class_list_macros.hpp in a couple of places to fix the deprecation warning. The warning would actually cause it to not compile because #warning isn't supported on MSVC and that causes a compile error

There are also some of MoveIt2's dependencies that have PRs that need to be merged in order for MoveIt2 to build and completely work on Windows:

I know #238 already exists with some fixes for windows, but it is out of date and doesn't have all of the fixes this PR has.

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

Ace314159 and others added 30 commits May 31, 2021 13:28
fork and kill aren't available on windows, so just warn when they are trying to be used for now
uint is not defined on MSVC
* Fixed location of GLUT,GLEW framework on macOS
* Use explicit boost namespace for bind placeholders
Co-authored-by: Tyler Weaver <[email protected]>
Tobias-Fischer added a commit to RoboStack/ros-galactic that referenced this pull request Jul 22, 2021
@mamoll
Copy link
Contributor

mamoll commented Jul 23, 2021

@lilustga I am trying to build this on my Windows VM. I got pretty far, but there are two issues:

  • [Minor] I installed MongoDB via choco, but it doesn't include header files, so I have to ignore the warehouse_ros_mongo package:
colcon build --packages-skip warehouse_ros_mongo --event-handlers desktop_notification- status- --cmake-args -DCMAKE_BUILD_TYPE=Release
  • [Fatal] It's looking for non-existing debug versions of some Boost libraries:
c:\dev_moveit2\build\moveit_planners_ompl>cmake --build .
Microsoft (R) Build Engine version 16.7.0+b89cb5fde for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

  Auto build dll exports
LINK : warning LNK4044: unrecognized option '/openmp'; ignored [C:\dev_moveit2\build\moveit_planners_ompl\om
pl_interface\moveit_ompl_interface.vcxproj]
LINK : fatal error LNK1104: cannot open file 'C:\opt\ros\foxy\x64\debug\lib\boost_serialization-vc140-mt-gd.
lib' [C:\dev_moveit2\build\moveit_planners_ompl\ompl_interface\moveit_ompl_interface.vcxproj]
  gtest.vcxproj -> C:\dev_moveit2\build\moveit_planners_ompl\gtest\Debug\gtest.lib
  gtest_main.vcxproj -> C:\dev_moveit2\build\moveit_planners_ompl\gtest\Debug\gtest_main.lib

The non-debug versions exist C:\opt\ros\foxy\x64\lib. There's probably a simple fix for this, but I am not too familiar with Windows development.

@lilustga
Copy link
Contributor

@mamoll The mongo DB isn't working on Windows or Mac right now, you can see some conversation above in the thread about warehouse_ros_mongo.

For the second issue, are you getting the same error when you call colcon build (instead of cmake directly)?

Which version of Visual Studio do you have installed? Do you have the "Desktop development with C++" workload included?

@mamoll
Copy link
Contributor

mamoll commented Jul 24, 2021

I realized that I should have typed cmake --build . --target Release. Then the code almost compiles. I just needed to make this two-line change:

--- a/moveit_planners/ompl/ompl_interface/scripts/generate_state_database.cpp
+++ b/moveit_planners/ompl/ompl_interface/scripts/generate_state_database.cpp
@@ -56,14 +56,14 @@ static const std::string ROBOT_DESCRIPTION = "robot_description";
 static const std::string CONSTRAINT_PARAMETER = "constraints";

 static bool get_uint_parameter_or(const rclcpp::Node::SharedPtr& node, const std::string& param_name,
-                                  size_t&& result_value, const size_t default_value)
+                                  unsigned int& result_value, unsigned int default_value)
 {
   int param_value;
   if (node->get_parameter(param_name, param_value))
   {
     if (param_value >= 0)
     {
-      result_value = static_cast<size_t>(param_value);
+      result_value = static_cast<unsigned int>(param_value);
       return true;
     }

It makes no sense to make POD types universal refs., so this seems like a harmless change. (The const change isn't strictly necessary, but also doesn't make sense in the original code.)

Copy link
Contributor

@mamoll mamoll left a comment

Choose a reason for hiding this comment

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

This looks good to me. I had to change one function to get it to compile. Not sure what is different about my setup. Let's get this merged and let's add some Windows configs to the CI build matrix! Nice work, @Ace314159 and thanks for the assist, @lilustga.

@Ace314159
Copy link
Contributor Author

Awesome! I'm glad you were able to compile! I fixed the merge conflicts, so it should be good to merge!

@Ace314159
Copy link
Contributor Author

It seems to fail when trying to install the upstream dependencies. I don't think that's related to the changes.

@tylerjw
Copy link
Member

tylerjw commented Jul 26, 2021

It seems to fail when trying to install the upstream dependencies. I don't think that's related to the changes.

This is affecting all of our PRs right now. The issue seems to be with the official build farm. We are tracking the issue here: osrf/infrastructure#5

I'll retrigger the CI for this job (and the others) when that is resolved.

@wyattrees
Copy link
Contributor

Tested this on my Windows system and I'm running into this error from moveit_ros_perception when building:

semantic_world.obj : error LNK2019: unresolved external symbol "void __cdecl cv::findContours(class cv::_InputArray const &,class cv::_OutputArray const &,class cv::_OutputArray const &,int,int,class cv::Point_<int>)" (?findContours@cv@@YAXAEBV_InputArray@1@AEBV_OutputArray@1@1HHV?$Point_@H@1@@Z) referenced in function "public: class std::vector<struct geometry_msgs::msg::PoseStamped_<class std::allocator<void> >,class std::allocator<struct geometry_msgs::msg::PoseStamped_<class std::allocator<void> > > > __cdecl moveit::semantic_world::SemanticWorld::generatePlacePoses(struct object_recognition_msgs::msg::Table_<class std::allocator<void> > const &,double,double,double,unsigned int,double)const " (?generatePlacePoses@SemanticWorld@semantic_world@moveit@@QEBA?AV?$vector@U?$PoseStamped_@V?$allocator@X@std@@@msg@geometry_msgs@@V?$allocator@U?$PoseStamped_@V?$allocator@X@std@@@msg@geometry_msgs@@@std@@@std@@AEBU?$Table_@V?$allocator@X@std@@@msg@object_recognition_msgs@@NNNIN@Z) [C:\dev_ws\build\moveit_ros_perception\semantic_world\moveit_semantic_world.vcxproj]
C:\dev_ws\build\moveit_ros_perception\semantic_world\Release\moveit_semantic_world.dll : fatal error LNK1120: 1 unresolved externals [C:\dev_ws\build\moveit_ros_perception\semantic_world\moveit_semantic_world.vcxproj]

Note: the package silently failed until I included --event-handlers console_direct+ along with colcon build

I have OpenCV installed, and it is found during the build process:

-- Found OpenCV 3.4.6 in C:/opencv/x64/vc16/lib

any suggestions on how to fix this?

@lilustga
Copy link
Contributor

@wyattrees I haven't been able to repro this, I'm building this branch successfully with colcon build --event-handlers console_direct+.

Do you see linker errors for all the OpenCV symbols in semantic_world (cv::Point, cv::Mat, etc.) or just for cv::findContours?

@Ace314159
Copy link
Contributor Author

@wyattrees I'd also recommend using vcpkg for dependency management, which is much simpler to use and what I have tested with. I also have a GitHub Action that successfully builds MoveIt2 and its dependencies using vcpkg here if you'd like to see how to use it.

@lilustga
Copy link
Contributor

@Ace314159 with this change #576 tests should pass now. If you rebase or merge with main again I think the PR will be ready to merge.

@lilustga
Copy link
Contributor

@tylerjw @henningkayser Now that this is green, how do you feel about merging?

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

I read through these changes and don't see anything wrong. I think we should merge this so the windows team can use moveit on foxy with our latest fixes. This will also the be the last thing we are waiting on before making a foxy branch.

@tylerjw tylerjw merged commit c4a4539 into moveit:main Jul 30, 2021
@wyattrees
Copy link
Contributor

@lilustga @Ace314159 I'm only seeing the linker error for findContours. I tried using vcpkg to install the dependencies but still getting the same. I think this is probably an issue with something else I'm doing wrong in setup. Anyway, thanks for your work on this!

christianlandgraf pushed a commit to christianlandgraf/moveit2 that referenced this pull request Aug 12, 2021
Co-authored-by: JafarAbdi <[email protected]>
Co-authored-by: Nisala Kalupahana <[email protected]>
Co-authored-by: Jorge Nicho <[email protected]>
Co-authored-by: Henning Kayser <[email protected]>
Co-authored-by: Vatan Aksoy Tezer <[email protected]>
Co-authored-by: Tyler Weaver <[email protected]>
Co-authored-by: Lior Lustgarten <[email protected]>
@lilustga lilustga mentioned this pull request Aug 16, 2021
6 tasks
tylerjw added a commit to tylerjw/moveit2 that referenced this pull request Aug 27, 2021
Co-authored-by: JafarAbdi <[email protected]>
Co-authored-by: Nisala Kalupahana <[email protected]>
Co-authored-by: Jorge Nicho <[email protected]>
Co-authored-by: Henning Kayser <[email protected]>
Co-authored-by: Vatan Aksoy Tezer <[email protected]>
Co-authored-by: Tyler Weaver <[email protected]>
Co-authored-by: Lior Lustgarten <[email protected]>
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
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.