Skip to content

[drake_bazel_external] Add documentation for proprietary solvers #391

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tyler-yankee
Copy link
Collaborator

@tyler-yankee tyler-yankee commented May 1, 2025

Closes #108.


This change is Reviewable

Copy link
Collaborator Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

+@BetsyMcPhail for feature review, please.

Reviewable status: all discussions resolved, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)

@jwnimmer-tri
Copy link
Contributor

jwnimmer-tri commented May 2, 2025

This should do a better job of xref to https://drake.mit.edu/from_source.html#building-with-bazel. Also the flags used here are wrong, see the https://github.com/RobotLocomotion/drake/blob/master/tools/flags/BUILD.bazel linked from that doc for the right spelling. Also the environment variables should be illustrated here, per the docs.

@tyler-yankee
Copy link
Collaborator Author

  • Thanks for the spelling; I think I mixed it up with the CMake options.
  • What do you see as the benefit of xref to that link? It discusses the Bazel examples, which the user has already found as they are here, and links to the proprietary solvers page, which I've already linked directly.
  • The proprietary solvers page already explains the environment variables and provides example export commands, so I'd rather not duplicate information in a comment here. What did you have in mind? Some quick/generic sentence here (e.g. "Make sure you've set the environment variables to the location of the license files."), or maybe something separate in the README?

@jwnimmer-tri
Copy link
Contributor

Also the environment variables should be illustrated here, per the docs.

Oops, retracted. Somehow I missed that they already were here.

However, note that we want build --test_env not test --test_env, and we should have better comments like Drake has already, e.g.:

# Location of the MOSEK license file, typically named "mosek.lic".
# Setting this --test_env as a 'build' flag is deliberate, to keep the analysis
# cache intact when switching between build and test.
build --test_env=MOSEKLM_LICENSE_FILE

What do you see as the benefit of xref to that link? It discusses the Bazel examples, which the user has already found as they are here, and links to the proprietary solvers page, which I've already linked directly.

Ya. The piece that's on the website doc but not in the rcfile here is the pointer to flags/BUILD.bazel, but I guess once the flags are fixed to refer to labels inside Drake's flags directory, maybe users will figure that out on their own.

... I'd rather not duplicate information in a comment here ...

I am OK with the current PR's scope, now that I read it more carefully.


Also remember -- feature review here will need to uncomment these lines and run the bazel test //... to confirm that the commented-out code actually works.

@tyler-yankee
Copy link
Collaborator Author

uncomment these lines and run the bazel test //...

...and remember that this won't work in CI, since DEE CI is not set up to pull the license files and set up the environment variables. We'll have to verify locally.

I can definitely add those comments in as you suggested.

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.

[drake_bazel_external] Commercial solvers
3 participants