moveit / moveit_drake

Experimental repository for Moveit2 - Drake integration
BSD 3-Clause "New" or "Revised" License
11 stars 4 forks source link

Adds Dockerfile, docker-compose and dev instructions on readme #4

Closed kamiradi closed 4 weeks ago

kamiradi commented 1 month ago

This PR adds

sea-bass commented 1 month ago

Actually, several of those issues building the code were due to some other misconfigurations I found. You guys should try my PR https://github.com/kamiradi/moveit_drake/pull/1 and merge it into this branch if it works.

It fixes everything except these errors in that demo subfolder:

CMake Error at demo/CMakeLists.txt:1 (add_executable):
  Target "pipeline_testbench_example" links to target
  "moveit_core::moveit_butterworth_parameters" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?

CMake Error at demo/CMakeLists.txt:1 (add_executable):
  Target "pipeline_testbench_example" links to target
  "moveit_core::moveit_planning_request_adapter" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?
kamiradi commented 1 month ago

Thanks for the review @sea-bass. A few overall comments on the docker build and how I intend to use the image/container, let me know if I am missing something.

Looks good! Almost worked on my end, but I was missing some rosdep dependencies of moveit_drake:

  Could not find a package configuration file provided by
  "moveit_visual_tools" with any of the following names:

    moveit_visual_toolsConfig.cmake
    moveit_visual_tools-config.cmake

Might be useful to actually do a COPY of the source code during build and run rosdep install with that package.xml also included at build.

The Docker image just ensures a drake installation and built moveit packages. After that, you will need to start the container and perform the following steps that were part of the initial build instructions suggested by @sjahr

  1. Perform the moveit-drake vcs import.
  2. Do the necessary rosdep installation
  3. colcon build

I put these in the readme. Didn't face any issues with the build. I did not copy the repository using COPY . . because I usually mount the repository using the compose file, purely for dev reasons. Obviously this can be optimized by just ensuring moveit_drake also builds with the rest of moveit2. Just thought that since I may be anyway building moveit_drake as part of the dev process, I'll keep it separate.

Although @sjahr I notice that installing the moveit_visual_tools dependency ends up installing a bunch of moveit binaries, which is not ideal. Can we maybe remove some of the dependencies in that package.xml separately? Maybe this pipeline benchmark demo is adding a lot of dependencies we don't want?

Even after that, inside the Docker I get some build errors:

CMake Error at demo/CMakeLists.txt:1 (add_executable):
  Target "pipeline_testbench_example" links to target
  "moveit_core::moveit_butterworth_parameters" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?

CMake Error at demo/CMakeLists.txt:1 (add_executable):
  Target "pipeline_testbench_example" links to target
  "moveit_core::moveit_planning_request_adapter" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

Hmmm, I didn't come across this. Does this stop the build or pop up as a warning?

Besides the build failures from the existing code in this repo, I think there are some easy improvements to make -- see the inline comments for details. But overall:

  • First, I would try to consolidate some of the installs, as there seem to be a few places where the same commands are running multiple times, which shouldn't be necessary.
  • I also think if there is a way to not install MoveIt packages from apt in the first place so you don't have to uninstall them, that would be preferable.
  • Last blocker is that there seem to be some environment variables that don't seem to exist, which likely are an effect of borrowing this setup from somewhere else.

Strong agree on these. Indeed, the docker config was taken from another setup I use. I'll make the necessary env variable changes and take all your docker optimizations Thank you! :smiley:

As a comment for later, it seems like this Docker setup won't persist the output of colcon build on the workspace, which is something I am happy to help modify myself after this PR is merged. Definitely not needed for this PR.

This again is more for dev reasons than anything else.

kamiradi commented 1 month ago

Actually, several of those issues building the code were due to some other misconfigurations I found. You guys should try my PR kamiradi#1 and merge it into this branch if it works.

It fixes everything except these errors in that demo subfolder:

CMake Error at demo/CMakeLists.txt:1 (add_executable):
  Target "pipeline_testbench_example" links to target
  "moveit_core::moveit_butterworth_parameters" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?

CMake Error at demo/CMakeLists.txt:1 (add_executable):
  Target "pipeline_testbench_example" links to target
  "moveit_core::moveit_planning_request_adapter" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

LGTM, I'll wait for @sjahr to review before merging it.

sea-bass commented 1 month ago

Good question, for me running sudo apt purge ros-humble-moveit* before building the workspace fixed the build error above. I know it is not the prettiest solution but maybe that is the fastest way to fix this for now. We'll need moveit_visual_tools for the demo unfortunately 🙈 @kamiradi Do you mind adding that to this PR and then we'll merge it

Gah sorry, @kamiradi had that in originally and I removed it thinking it was harmless.

sjahr commented 1 month ago

:sweat_smile: Oh damn

kamiradi commented 4 weeks ago

I have made the changes, but haven't built the image yet as I am facing some Xauthority issues when trying the new way. @sea-bass, can you point me in the direction of how I can create the $HOME/.Xauthority file? Seems like i am missing it on my system.

sjahr commented 4 weeks ago

:thinking: Adding the command to the docker file does not seem to fix the issue. When I re-build the container and then run colcon build inside it, is still see the issue. Running the remove command again, fixes it. Also for some reason, not all repositories from the .repos file are cloned into the src folder. My folder in the docker just contains generate_parameter_library geometric_shapes moveit2 moveit_drake moveit_msgs moveit_resources

sjahr commented 4 weeks ago

@kamiradi I am intrigued to merge this PR, and open issues to fix this. Let's talk about the problems in our meeting later today and move forward. None of the issues are blocking I think

kamiradi commented 4 weeks ago

In the current

🤔 Adding the command to the docker file does not seem to fix the issue. When I re-build the container and then run colcon build inside it, is still see the issue. Running the remove command again, fixes it. Also for some reason, not all repositories from the .repos file are cloned into the src folder. My folder in the docker just contains generate_parameter_library geometric_shapes moveit2 moveit_drake moveit_msgs moveit_resources

I suspected this would be an issue. Yes indeed, in my original version of the PR, I intended the moveit_drake build instructions on the readme to stay. After building the image, you will still need to do an additional moveit_drake vcs import before updating and installing the rosdeps, and then build the repository. This is still reflected in the readme.

Alternatively, in the subsequent PRs I can add these steps as well to the Dockerfile.