Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

Initial ROS drake package and repository information #6

Merged
merged 23 commits into from
Mar 19, 2024

Conversation

j-rivero
Copy link
Collaborator

@j-rivero j-rivero commented Jan 16, 2024

New ROS drake package based on just calling ament_vendor to download, build and install the version 1.24 of Drake.

In this initial phase the GUROBI, MOSEK and SNOPT components are disabled in the build using the CMake parameters. The ament_vendor uses a couple of patches to deal with issues related to clang version and to disable the assumption of platform having a libjchart2d-java package. Patches uses the Debian DEP-3 format to include metadata on the same file. Dependencies are based on the official Drake documentation and comments are in package.xml.

Together with the code there is Dockerfile that will build the code in a plain jammy image, resolving the dependencies installation using rosdep (this allow us to test the completeness/correctness of the dependencies in package.xml ) and call the CMake build interface using the same parameters used by the ROS buildfarm.

In draft (@jwnimmer-tri feel free to comment) until I can test the code in a real ROS buildfarm build and check that a Drake code example builds fine.


This change is Reviewable

New ROS drake package based on just calling ament_vendor
to download, build and install the version 1.24 of Drake.

In this initial phase the GUROBI, MOSEK and SNOPT
components are disabled in the build using the CMake
parameters.

The ament_vendor uses a couple of patches to deal with
issues related to clang version and to disable the
assumption of platform having a libjchart2d-java package.
Patches uses the Debian DEP-3 format to include metadata
on the same file.

Dependencies are based on the official documentation and
comments are in package.xml.

Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
@jwnimmer-tri
Copy link
Contributor

FYI https://github.com/RobotLocomotion/drake/releases/tag/v1.25.0 was just tagged. We can drop the 0002 patch once we shift to that version.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

In this initial phase the GUROBI, MOSEK and SNOPT components are disabled in the build using the CMake parameters.

Sounds good.

The Dockerfile.cmake_dpkg_flags can be used to simulate the build that the ros_buildfarm will perform ...

This is great, thanks! Once this PR is sufficiently polished, I am happy to try it locally to cross-check that it works for me. Let me know when you think that is.


I posted some review discussions on the code itself. If it's premature to review, feel free to dismiss or defer any of the comments until they are helpful.

Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @j-rivero)


.entrypoint-cmake-make-with-dpkgbuildflags.sh line 9 at r1 (raw file):

cmake .. -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DCMAKE_PREFIX_PATH=/opt/ros/rolling -DAMENT_PREFIX_PATH=/opt/ros/rolling -DCMAKE_INSTALL_PREFIX=/opt/ros/rolling -DCMAKE_BUILD_TYPE=None
make -j1 || exec "$@"
exec "$@"

nit It took me a bit of study to glean what the final two lines were accomplishing, and why "$@" was expanded twice. I think the aim is just to give the user their bash prompt, whether or not the build passed?

If so, then I think there is a more straightforward spelling:

Suggestion:

make -j1 || true
exec "$@"

CMakeLists.txt line 11 at r1 (raw file):

  VCS_VERSION v1.24.0
  CMAKE_ARGS
    -DDRAKE_IS_BUILDING_DOCUMENTATION:BOOL=OFF

nit I'm fairly certain that -DDRAKE_IS_BUILDING_DOCUMENTATION:BOOL=OFF is a no-op and should be removed. Drake's CMakeLists doesn't use that flag.

(There is some Python code that checks something similar, but the CMake definitions don't propagate into Python.)


CMakeLists.txt line 17 at r1 (raw file):

    -DWITH_USER_EIGEN:BOOL=ON
    -DWITH_USER_FMT:BOOL=ON
    -DWITH_USER_SPDLOG:BOOL=ON

nit Would it be typical to comment any non-default configuration settings with their justification?

Suggestion:

    # At the moment, closed-source dependencies are disabled.
    -DWITH_GUROBI:BOOL=OFF
    -DWITH_MOSEK:BOOL=OFF
    -DWITH_SNOPT:BOOL=OFF
    # Drake should use the Ament version of these dependencies found by CMake,
    # not the Ubuntu version found in /usr/include.
    -DWITH_USER_EIGEN:BOOL=ON
    -DWITH_USER_FMT:BOOL=ON
    -DWITH_USER_SPDLOG:BOOL=ON

Dockerfile.cmake_dpkg_flags line 43 at r1 (raw file):

COPY . ros-drake-vendor/

# RUN date

nit Possibly stray line?

Code quote:

# RUN date

Copy link
Collaborator Author

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

I posted some review discussions on the code itself. If it's premature to review, feel free to dismiss or defer any of the comments until they are helpful.

Useful to advance in the work while we wait other PR and implementations to happen. Thanks.

This is great, thanks! Once this PR is sufficiently polished, I am happy to try it locally to cross-check that it works for me. Let me know when you think that is.

Not yet there but I'll change the Draft status as soon as we resolve some pending items like #3, #7 or #8. Ping you when ready but please feel free to jump in anytime if you have feedback.

Reviewed 2 of 7 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @j-rivero)


.entrypoint-cmake-make-with-dpkgbuildflags.sh line 9 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit It took me a bit of study to glean what the final two lines were accomplishing, and why "$@" was expanded twice. I think the aim is just to give the user their bash prompt, whether or not the build passed?

If so, then I think there is a more straightforward spelling:

done in 0ec7427


CMakeLists.txt line 11 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I'm fairly certain that -DDRAKE_IS_BUILDING_DOCUMENTATION:BOOL=OFF is a no-op and should be removed. Drake's CMakeLists doesn't use that flag.

(There is some Python code that checks something similar, but the CMake definitions don't propagate into Python.)

done in 4f0e964


CMakeLists.txt line 17 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Would it be typical to comment any non-default configuration settings with their justification?

done in a4cd6a9


Dockerfile.cmake_dpkg_flags line 43 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Possibly stray line?

done in 98052d3

Signed-off-by: Jose Luis Rivero <[email protected]>
This is waiting #8 resolution but coding the preferred method here

Signed-off-by: Jose Luis Rivero <[email protected]>
Name change is waiting to resolve #7 but this name change was
required in order to make the examples to work when using the
drake-extras.cmake.

Signed-off-by: Jose Luis Rivero <[email protected]>
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r2, 4 of 5 files at r3, all commit messages.
Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @j-rivero)

@j-rivero
Copy link
Collaborator Author

j-rivero commented Mar 7, 2024

I have updated the PR to use the drake name 0044fbd as discussed in #8 and do some other improvements:

With this I'm able to compile the drake_cmake_installed and drake_cmake_installed_aptand drake_ament_cmake_installed from RobotLocomotion/drake-external-examples without touching a single line (this is being done in the GitHub actions).

With this code I'm also able to create a testing ros-rolling-drake package in the ROS testing buildfarm Build Status that we can install and use to build the previous examples just fine. I'm starting to be moderately confident that it could be a good candidate for more wider testing.

@j-rivero j-rivero marked this pull request as ready for review March 7, 2024 18:04
@j-rivero j-rivero requested a review from jwnimmer-tri March 8, 2024 15:34
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: modulo open discussions.

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @j-rivero)


.entrypoint-cmake-make-with-dpkgbuildflags.sh line 1 at r4 (raw file):

#!/bin/bash -e

nit Now that the Dockerfile is gone, is this file dead code that should be removed?

If not, then it probably some comment at the start here to explain the context of how/where it will/should be used.


package.xml line 12 at r4 (raw file):

  </description>
  <maintainer email="[email protected]">Drake Users</maintainer>
  <license>BSD</license>

What is the expectation for the comprehensiveness of this <license> element? Certainly the majority of Drake's first-party code is SPDX: BSD-3-Clause, but Drake vendors a bunch of other code with various open-source licenses.

(If we still need some surgery here then that's fine, but please file a tracking issue to remind us.)


package.xml line 13 at r4 (raw file):

  <maintainer email="[email protected]">Drake Users</maintainer>
  <license>BSD</license>
  <!--

FYI #12 is related. For now, we can merge the manually-curated list of dependencies, but eventually we'll need computer assistance with computing this list.


.github/workflows/ci.yaml line 22 at r4 (raw file):

              "build": {
                "cmake-target": "install",
                "merge-install": true

BTW These lines seem like they might be a temporary work-around for #10? If that's so, then we should have a comment nearby explaining, so that we will remember to fix this stanza after its resolved.


.github/workflows/ci.yaml line 39 at r4 (raw file):

          . /opt/ros/rolling/setup.bash
          . install/setup.bash
          colcon build --merge-install --packages-select drake_cmake_installed drake_cmake_installed_apt drake_ament_cmake_installed --event-handlers console_direct+

It's not clear to me why drake_cmake_installed_apt is on this list. Building that project against the output of ros-drake-vendor is not a supported workflow (nor will it ever be). That project documents that https://drake.mit.edu/apt.html must happen before buliding it, which is not part of this CI job (nor should it be).

Copy link
Collaborator Author

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 9 files reviewed, 4 unresolved discussions (waiting on @jwnimmer-tri)


package.xml line 12 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

What is the expectation for the comprehensiveness of this <license> element? Certainly the majority of Drake's first-party code is SPDX: BSD-3-Clause, but Drake vendors a bunch of other code with various open-source licenses.

(If we still need some surgery here then that's fine, but please file a tracking issue to remind us.)

I've created #13 and made a comment on package.xml that refers to the issue. I leave the term "BSD" by now since the field accepts any string and although it may sound vague, it is widely used in the ROS community to refer to BSD-3-Clause.


package.xml line 13 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI #12 is related. For now, we can merge the manually-curated list of dependencies, but eventually we'll need computer assistance with computing this list.

+1 on this.


.github/workflows/ci.yaml line 22 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW These lines seem like they might be a temporary work-around for #10? If that's so, then we should have a comment nearby explaining, so that we will remember to fix this stanza after its resolved.

Good idea, done in b44b5be


.github/workflows/ci.yaml line 39 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It's not clear to me why drake_cmake_installed_apt is on this list. Building that project against the output of ros-drake-vendor is not a supported workflow (nor will it ever be). That project documents that https://drake.mit.edu/apt.html must happen before buliding it, which is not part of this CI job (nor should it be).

Makes sense, removed 94257a9


.entrypoint-cmake-make-with-dpkgbuildflags.sh line 1 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Now that the Dockerfile is gone, is this file dead code that should be removed?

If not, then it probably some comment at the start here to explain the context of how/where it will/should be used.

ouch, removed 607c226

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @j-rivero)

Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Copy link
Collaborator Author

@jwnimmer-tri excuse the change after the approval. I wanted to get the green CI before merging and changed the Rolling distro to Humble since Rolling seems to currently have some problems detecting the installation correctly. 6562fe9.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @j-rivero)

@jwnimmer-tri
Copy link
Contributor

I'll just merge this now to get something committed onto the main branch. Future PRs can still improve things.

@jwnimmer-tri jwnimmer-tri merged commit 9fb0016 into main Mar 19, 2024
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants