[drake_cmake_external] Add test case to disable solver #370

tyler-yankee · 2025-02-13T15:08:50Z

Addresses #22547. See #22626 for the change on Drake. This change adds a unit test to drake_cmake_external to test by disabling the CSDP solver.

Tested locally by changing the URL pointed to by ExternalProject_Add(drake) to the feature branch on Drake.

This change is

tyler-yankee

+@jwnimmer-tri for feature review, please!

Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri)

tyler-yankee

Note that the CI failure in Jenkins is expected, since it's using Drake master which doesn't yet have the CMake solver opt-out flags.

Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri)

jwnimmer-tri · 2025-02-13T22:04:43Z

Note that the CI failure in Jenkins is expected, since it's using Drake master which doesn't yet have the CMake solver opt-out flags.

A nice tool we sometimes use in cases like this is to temporarily add a commit here that re-points Drake to the pull request instead of master, so CI here will shine light. Then after the Drake PR lands, we revert that edit.

jwnimmer-tri

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing

drake_cmake_external/drake_external_examples/solver_disabled_test.py line 4 at r2 (raw file):

"""
Provides an example of disabling the open-source CSDP solver.

nit This doesn't quite smell right to me.

It seems more clear would be something along the lines of "Our CMakeLists.txt file disabled the CSDP solver as part of the Drake build. Here, we'll check that the opt-out succeeded."

Code quote:

Provides an example of

drake_cmake_external/CMakeLists.txt line 115 at r2 (raw file):

    -DWITH_USER_FMT:BOOLEAN=ON
    -DWITH_USER_SPDLOG:BOOLEAN=ON
    -DWITH_CSDP:BOOLEAN=OFF

nit A first-time Drake user will not be able to understand why this line is here. We should have an expository comment here or nearby that is sufficient for them to not be confused.

Perhaps something like "The Drake build has options to turn features on/off. See https://drake.mit.edu/from_source.html for the full list. Here, we demonstrate how to set an arbitrary option."?

tyler-yankee

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri)

drake_cmake_external/CMakeLists.txt line 115 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit A first-time Drake user will not be able to understand why this line is here. We should have an expository comment here or nearby that is sufficient for them to not be confused.

Perhaps something like "The Drake build has options to turn features on/off. See https://drake.mit.edu/from_source.html for the full list. Here, we demonstrate how to set an arbitrary option."?

Yes, agreed. I added that comment and included a note about it corresponding to one of the open-source dependencies.

drake_cmake_external/drake_external_examples/solver_disabled_test.py line 4 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit This doesn't quite smell right to me.

It seems more clear would be something along the lines of "Our CMakeLists.txt file disabled the CSDP solver as part of the Drake build. Here, we'll check that the opt-out succeeded."

Yes, that makes sense. I replaced that comment.

jwnimmer-tri

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [jwnimmer-tri] (waiting on @tyler-yankee)

drake_cmake_external/CMakeLists.txt line 98 at r4 (raw file):

ExternalProject_Add(drake
  DEPENDS eigen fmt spdlog
  URL https://github.com/RobotLocomotion/drake/archive/master.tar.gz

Working

Reminder to myself to make sure we revert this before landing the PR.

jwnimmer-tri · 2025-02-20T17:58:40Z

BTW the new test case is actually failing...

tyler-yankee · 2025-02-20T18:00:14Z

Yeah, I'm looking into it now. Something weird is going on with the types on the options defined by the function.

tyler-yankee · 2025-02-20T18:48:33Z

drake_cmake_external/CMakeLists.txt line 119 at r4 (raw file):

    # Here, we demonstrate how to set an arbitrary option
    # for one of the open-source dependencies.
    -DWITH_CSDP:BOOLEAN=OFF

By the way, these options should technically be defined as BOOL, not BOOLEAN. It's a non-breaking syntax error that should only be affecting how it would show up in the CMake GUI. Should this be changed, and if so, in this PR?

jwnimmer-tri

Reviewable status: 2 unresolved discussions, platform LGTM from [jwnimmer-tri] (waiting on @tyler-yankee)

drake_cmake_external/CMakeLists.txt line 119 at r4 (raw file):

Should this be changed, ....

That's a question for the Kitware side. I don't know enough about CMake to have an opinion here.

... and if so, in this PR?

I'm OK with whatever PR split or non-split you decide.

tyler-yankee · 2025-02-20T20:24:50Z

drake_cmake_external/CMakeLists.txt line 119 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Should this be changed, ....

That's a question for the Kitware side. I don't know enough about CMake to have an opinion here.

... and if so, in this PR?

I'm OK with whatever PR split or non-split you decide.

@BetsyMcPhail: could you take a look at this?

tyler-yankee · 2025-02-20T21:34:39Z

Provisioning script flaked on one of the Bazel examples in Jenkins, rebuilding ...

tyler-yankee · 2025-02-21T12:31:11Z

Okay, CI is all happy again. I had a couple of bugs in the new function to work out.

BetsyMcPhail

Reviewable status: 2 unresolved discussions, platform LGTM from [jwnimmer-tri] (waiting on @jwnimmer-tri and @tyler-yankee)

drake_cmake_external/CMakeLists.txt line 119 at r4 (raw file):

Previously, tyler-yankee wrote…

@BetsyMcPhail: could you take a look at this?

I think we should update all of these to use BOOL.

BetsyMcPhail

(modulo the test-only code being removed)

Reviewable status: 1 unresolved discussion, platform LGTM from [jwnimmer-tri, betsymcphail] (waiting on @jwnimmer-tri)

jwnimmer-tri

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [jwnimmer-tri, betsymcphail] (waiting on @tyler-yankee)

jwnimmer-tri

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [jwnimmer-tri, betsymcphail] (waiting on @jwnimmer-tri)

drake_cmake_external/CMakeLists.txt line 98 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Reminder to myself to make sure we revert this before landing the PR.

Tomorrow morning, hopefully the new nightly build packages will be usable here.

jwnimmer-tri

Reviewable status: 1 unresolved discussion, platform LGTM from [jwnimmer-tri, betsymcphail] (waiting on @tyler-yankee)

jwnimmer-tri

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all discussions resolved, platform LGTM from [jwnimmer-tri, betsymcphail] (waiting on @tyler-yankee)

add solver_disabled_test

846f2be

tyler-yankee mentioned this pull request Feb 13, 2025

[build] CMake options for solver opt-out RobotLocomotion/drake#22626

Merged

tyler-yankee assigned jwnimmer-tri Feb 13, 2025

tyler-yankee commented Feb 13, 2025

View reviewed changes

use development branch of Drake (temp)

7e717a9

jwnimmer-tri reviewed Feb 19, 2025

View reviewed changes

add comments to CMakeLists and the unit test for solver option

0d468e6

tyler-yankee commented Feb 20, 2025

View reviewed changes

jwnimmer-tri approved these changes Feb 20, 2025

View reviewed changes

jwnimmer-tri reviewed Feb 20, 2025

View reviewed changes

update temporary commit of Drake to test new option

0bb71ce

tyler-yankee force-pushed the cmake-solvers branch from 6032927 to 0bb71ce Compare February 20, 2025 21:23

BetsyMcPhail reviewed Feb 21, 2025

View reviewed changes

fix BOOL/BOOLEAN syntax in CMakeLists

294b836

BetsyMcPhail approved these changes Feb 21, 2025

View reviewed changes

jwnimmer-tri reviewed Feb 21, 2025

View reviewed changes

update temporary commit of Drake (CI test)

e077e65

jwnimmer-tri requested changes Feb 24, 2025

View reviewed changes

jwnimmer-tri added 3 commits February 25, 2025 07:05

Merge remote-tracking branch 'upstream' into cmake-solvers

16e8c6b

revert back to drake master

29aa298

rewrap comment to 80

aa7ea68

jwnimmer-tri approved these changes Feb 25, 2025

View reviewed changes

jwnimmer-tri merged commit 7cbadd2 into RobotLocomotion:main Feb 25, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[drake_cmake_external] Add test case to disable solver #370

[drake_cmake_external] Add test case to disable solver #370

tyler-yankee commented Feb 13, 2025 •

edited by jwnimmer-tri

Loading

tyler-yankee left a comment

tyler-yankee left a comment

jwnimmer-tri commented Feb 13, 2025

jwnimmer-tri left a comment

tyler-yankee left a comment

jwnimmer-tri left a comment

jwnimmer-tri commented Feb 20, 2025

tyler-yankee commented Feb 20, 2025

tyler-yankee commented Feb 20, 2025

jwnimmer-tri left a comment

tyler-yankee commented Feb 20, 2025

tyler-yankee commented Feb 20, 2025

tyler-yankee commented Feb 21, 2025

BetsyMcPhail left a comment

BetsyMcPhail left a comment

jwnimmer-tri left a comment

jwnimmer-tri left a comment

jwnimmer-tri left a comment

jwnimmer-tri left a comment

[drake_cmake_external] Add test case to disable solver #370

[drake_cmake_external] Add test case to disable solver #370

Conversation

tyler-yankee commented Feb 13, 2025 • edited by jwnimmer-tri Loading

tyler-yankee left a comment

Choose a reason for hiding this comment

tyler-yankee left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Feb 13, 2025

jwnimmer-tri left a comment

Choose a reason for hiding this comment

tyler-yankee left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Feb 20, 2025

tyler-yankee commented Feb 20, 2025

tyler-yankee commented Feb 20, 2025

jwnimmer-tri left a comment

Choose a reason for hiding this comment

tyler-yankee commented Feb 20, 2025

tyler-yankee commented Feb 20, 2025

tyler-yankee commented Feb 21, 2025

BetsyMcPhail left a comment

Choose a reason for hiding this comment

BetsyMcPhail left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

tyler-yankee commented Feb 13, 2025 •

edited by jwnimmer-tri

Loading