[doc] Streamline Drake build instructions #22461
Conversation
jwnimmer-tri
added
priority: medium
release notes: none
|
@drake-jenkins-bot linux-jammy-unprovisioned-clang-bazel-experimental-debug please |
jwnimmer-tri
changed the title
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers
doc/_pages/from_source.md line 106 at r1 (raw file):
version is hard-coded in Drake and cannot be configured. * WITH_SNOPT (default OFF). When ON, enables the `SnoptSolver` in the build. * This option is mutally exclusive with `WITH_ROBOTLOMOTION_SNOPT`.
typo
Suggestion:
ROBOTLOCOMOTIONdoc/_pages/from_source.md line 112 at r1 (raw file):
* Drake does not support using a SNOPT binary release (i.e., shared library); it requires a source archive (i.e., the Fortran code). * WITH_ROBOTLOMOTION_SNOPT (default OFF). When ON, enables the `SnoptSolver`
typo
Suggestion:
ROBOTLOCOMOTIONdoc/_pages/from_source.md line 178 at r1 (raw file):
If you enable any of proprietary solvers flags, then you must first install the solver and set environment variables per the [Proprietary Solvers](http://127.0.0.1:8000/bazel.html#proprietary-solvers)
BTW: I'm not sure that this is going to be sane when viewed on the drake website? Consider a relative URL?
Suggestion:
bazel.html#proprietary-solvers
jwnimmer-tri
force-pushed
the
doc-install-repave
branch
from
65ec436 to
b881cac
Compare
There was a problem hiding this comment.
Let me propose this:
+@sherm1 for feature review of the changes in setup/_pages/bazel.md (i.e., the developer-focused docs), please.
+@mwoehlke-kitware for feature review of the changes in setup/** plus the changes to docs/_pages/from_source.md, please.
Then for a final pass, we'll have 1 platform reviewer look over the whole thing including cross-checking all of the small 1-line edits.
WDYT?
Reviewable status: LGTM missing from assignees ggould-tri(platform),sherm1(platform),mwoehlke-kitware, labeled "do not merge" (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Feature 
doc/_pages/* files
Reviewed 5 of 11 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees ggould-tri(platform),mwoehlke-kitware, labeled "do not merge" (waiting on @jwnimmer-tri)
doc/_pages/bazel.md line 9 at r2 (raw file):
For first-time users, we strongly suggest using one of the pre-compiled binaries described on our [installation](/installation.html) page. If you need to install Drake from source please refer to the [CMake instructions](from_source.html). If
BTW currently goes to the generic "from source" doc; could go right to CMake like the Bazel one does.
Suggestion:
from_source.html#building-with-cmakedoc/_pages/bazel.md line 25 at r2 (raw file):
as a subprocess and maps CMake build options into Bazel build options, so that users can install Drake via standard tools without knowing anything about Bazel. See the [CMake instructions](from_source.html) for details.
BTW here too
Suggestion:
from_source.html#building-with-cmakedoc/_pages/from_source.md line 9 at r2 (raw file):
For first-time users, we strongly suggest using one of the pre-compiled binaries described on our [installation](/installation.html) page. This page explains how to build Drake form source, which is somewhat more challenging.
typo: form -> from
doc/_pages/from_source.md line 172 at r2 (raw file):
(to download a precompiled release). When building Drake from source as Bazel external, we offer flags for
typo: "as Bazel" -> "as a Bazel"
jwnimmer-tri
force-pushed
the
doc-install-repave
branch
from
b881cac to
2c0459a
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees ggould-tri(platform),mwoehlke-kitware, labeled "do not merge" (waiting on @jwnimmer-tri)
doc/_pages/bazel.md line 9 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW currently goes to the generic "from source" doc; could go right to CMake like the Bazel one does.
I'll split the difference and link to "Supported Configurations". I really want people to read that section before diving head-first into the build commands.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: LGTM missing from assignees ggould-tri(platform),mwoehlke-kitware, labeled "do not merge" (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Reviewed 1 of 11 files at r1.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees ggould-tri(platform),mwoehlke-kitware, labeled "do not merge" (waiting on @jwnimmer-tri)
setup/install_prereqs line 9 at r2 (raw file):
# This script should be run as the normal user (i.e., not under sudo). if [[ "${EUID}" -eq 0 && -n "${SUDO_USER:+D}" ]]; then echo 'This script must NOT be run through sudo as root' >&2
Both the comment and error message seem wrong/confusing. This seems to be mirroring this existing check:
# Check for existence of `SUDO_USER` so that this may be used in Docker
# environments.
if [[ -n "${SUDO_USER:+D}" && $(id -u ${SUDO_USER}) -eq 0 ]]; then
cat <<eof >&2
It appears that this script is running under sudo, but it was the root user
who ran sudo. That use is not supported; when already running as root, do not
use sudo when calling this script.
eof
...which seems to have much better wording.
setup/install_prereqs line 14 at r2 (raw file):
# We'll default to installing only the prerequisites necessary to build Drake. # To run tests, developers should pass the "--developer" flag. For details, see
Handling --developer in the lower-level scripts seems like a lot of changes, plus it introduces the potential for those scripts to be invoked in potentially surprising ways, e.g. --without-test-only --developer. I wonder if maybe this script should look for --developer and pass any other arguments to the lower-level scripts, along with the various --without-X if --developer is not present?
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees ggould-tri(platform),mwoehlke-kitware, labeled "do not merge" (waiting on @jwnimmer-tri)
setup/install_prereqs line 9 at r2 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Both the comment and error message seem wrong/confusing. This seems to be mirroring this existing check:
# Check for existence of `SUDO_USER` so that this may be used in Docker # environments. if [[ -n "${SUDO_USER:+D}" && $(id -u ${SUDO_USER}) -eq 0 ]]; then cat <<eof >&2 It appears that this script is running under sudo, but it was the root user who ran sudo. That use is not supported; when already running as root, do not use sudo when calling this script. eof...which seems to have much better wording.
It's actually a different check than your quoted text.
Previously (i.e., on master), our Ubuntu docs said to run sudo setup/ubuntu_install_prereqs.sh.
Now (in this PR), our docs say to run setup/install_prereqs (no sudo). This check is meant to fail-fast in case the user accidentally types in sudo ahead of the command.
I suppose the error message could be more clear? Saying "as root" probably distracts from the core problem. It could just directly say "ERROR: Run this program without using sudo", or something like that? Does that come across better?
The bash comment prior to the check seems clear enough to me, but if there's something specific confusing about it, let me know.
setup/install_prereqs line 14 at r2 (raw file):
I'm rewriting the entirety of all of these bash shenanigans from scratch in Python during #22055, so my primary goal here is to support what's necessary for our new, more sensible documentation.
I decided that it was worth putting 22055 on pause to fix our documentation first, and then circle back in a second pass to finish replacing the scripts. Specifically, in doc/_pages/from_source.md we now teach users who are installing Drake from source to run drake/setup/install_prereqs, with no arguments. (And so, the default source prereqs flavor is now just build-prereqs, instead of bazel-test-prereqs.) That means developers need a new flag to install the test-only (etc) things. You're right that it's many of boilerplate changes, but that's just because this pile of bash goo is terrible, so I think we just need to live with it for another month or two?
potentially surprising ways, e.g.
--without-test-only --developer
A fair observation, but you'll note that none of our documentation mentions --without-test-only, and therefore it is not supported, and so if users do silly things they get to keep the pieces when it breaks. It's also not super hard to recover from or work around, so the impact should be low anyway.
I wonder if maybe this script should look for
--developer...
As of 22055 (and follow-up), there will be only one prereqs script for all platforms and all flavors. That feels like the right time to better align how we map from command line flags into the work we're doing.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees ggould-tri(platform),mwoehlke-kitware, labeled "do not merge" (waiting on @jwnimmer-tri)
setup/install_prereqs line 9 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
It's actually a different check than your quoted text.
Previously (i.e., on master), our Ubuntu docs said to run
sudo setup/ubuntu_install_prereqs.sh.Now (in this PR), our docs say to run
setup/install_prereqs(no sudo). This check is meant to fail-fast in case the user accidentally types insudoahead of the command.I suppose the error message could be more clear? Saying "as root" probably distracts from the core problem. It could just directly say "ERROR: Run this program without using sudo", or something like that? Does that come across better?
The bash comment prior to the check seems clear enough to me, but if there's something specific confusing about it, let me know.
Yes, maybe it's "as root" causing the confusion. The way it's worded now, I read it "if you're root, don't use sudo, but otherwise sudo is okay". Which is what the other script is checking, IIUC.
If what we're trying to test is simply that you aren't running under sudo, do we need the $EUID check at all?
For that matter, I understand that (at least for docker, but possibly even in general), we want/need to support root installing Drake, but what about the user trying to run the script via a root login or some other mechanism that $SUDO_USER isn't set? (In #22477, rather than relying on $SUDO_USER, I use the user that owns the Drake installation.)
setup/install_prereqs line 14 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I'm rewriting the entirety of all of these bash shenanigans from scratch in Python during #22055, so my primary goal here is to support what's necessary for our new, more sensible documentation.
I decided that it was worth putting 22055 on pause to fix our documentation first, and then circle back in a second pass to finish replacing the scripts. Specifically, in
doc/_pages/from_source.mdwe now teach users who are installing Drake from source to rundrake/setup/install_prereqs, with no arguments. (And so, the default source prereqs flavor is now just build-prereqs, instead of bazel-test-prereqs.) That means developers need a new flag to install the test-only (etc) things. You're right that it's many of boilerplate changes, but that's just because this pile of bash goo is terrible, so I think we just need to live with it for another month or two?potentially surprising ways, e.g.
--without-test-only --developerA fair observation, but you'll note that none of our documentation mentions
--without-test-only, and therefore it is not supported, and so if users do silly things they get to keep the pieces when it breaks. It's also not super hard to recover from or work around, so the impact should be low anyway.I wonder if maybe this script should look for
--developer...As of 22055 (and follow-up), there will be only one prereqs script for all platforms and all flavors. That feels like the right time to better align how we map from command line flags into the work we're doing.
JFYI, # #22477 makes changes to those scripts.
doc/_pages/from_source.md line 55 at r3 (raw file):
(either by building Drake from source, or by downloading a pre-compiled Drake release) please see our gallery of [drake-external-examples](https://github.com/RobotLocomotion/drake-external-examples).
"gallery of drake-external-examples" is grammatically awkward. Consider either "drake-external-examples gallery" or "gallery of examples [consuming Drake as an external project]" (or some other grammatical "gallery of X").
Also, this whole paragraph feels like it would work better under "New Users", with the sentence "otherwise ..." removed.
doc/_pages/from_source.md line 99 at r3 (raw file):
running Drake's CMake configure script; Drake does not automatically download Gurobi. If Gurobi is not installed to its standard location, you must also specify `export GUROBI_HOME=${...GUROBI_UNZIP_PATH...}/linux64`
"Specify export" is a little awkward. "Run export" would be okay, but I think we can just imply the action.
Suggestion:
must also `export GUROBI_HOME=${...GUROBI_UNZIP_PATH...}/linux64`
jwnimmer-tri
force-pushed
the
doc-install-repave
branch
from
2c0459a to
0689ae6
Compare
There is now a single install_prereqs script for all platforms, which defaults to installing only the build prereqs (not developer prereqs). (Future work will further consolidate the prereqs into simpler files.) Retool our from_source (user) and bazel (developer) pages to use the new script and generally rewrite large chunks for improved clarity.
jwnimmer-tri
force-pushed
the
doc-install-repave
branch
from
0689ae6 to
52876ee
Compare
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees ggould-tri(platform),mwoehlke-kitware, labeled "do not merge" (waiting on @jwnimmer-tri)
doc/_pages/from_source.md line 55 at r3 (raw file):
grammatically awkward
Done.
this whole paragraph
I tweaked installation.html to highlight the gallery a little bit more.
In terms of this document, I think it's important to have "Building with CMake" immediately redirect people to the gallery, since in the modal case what they should be doing is forking one of those examples, not typing in these commands by hand.
doc/_pages/from_source.md line 99 at r3 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
"Specify export" is a little awkward. "Run export" would be okay, but I think we can just imply the action.
Done.
setup/install_prereqs line 9 at r2 (raw file):
... do we need the
$EUIDcheck at all?
Clarified in comment. (We should still allow sudo -u.)
user trying to run the script via a root login
This is allowed (though possibly ill-advised). Only only goal here is to catch a common mistake made out of habit. If users do something bizarre, it's okay for the script to continue and then fail partway through on whatever the real trouble was. This guard doesn't need to be perfect.
There was a problem hiding this comment.
setup/** and docs/_pages/from_source.md
Reviewed 4 of 11 files at r1, 2 of 3 files at r4.
Reviewable status: LGTM missing from assignee ggould-tri(platform), labeled "do not merge" (waiting on @jwnimmer-tri)
|
We'll leave it to @ggould-tri for platform review on Tuesday (who Sherm already assigned). |
There was a problem hiding this comment.
Reviewed 8 of 11 files at r1, 1 of 2 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions
a discussion (no related file):
I don't think "release notes: none" is right here, given that the install_prereqs change does break users' expected workflow. That change should be release notable, either in the release headline notes or in the breaking changes section (although it is not formally a breaking change).
doc/_pages/bazel.md line 43 at r4 (raw file):
We suggest you keep the default clone directory name (``drake``) and not rename it (e.g., ``drake2``). The CLion integration will suffer if the checkout is not
BTW, author's discretion, this seems to be a broader feature IDEs; vscode also has this property, at least by default.
Suggestion:
IDE
jwnimmer-tri
added
release notes: fix
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees ggould-tri(platform),sherm1(platform),mwoehlke-kitware
a discussion (no related file):
Previously, ggould-tri wrote…
I don't think "release notes: none" is right here, given that the install_prereqs change does break users' expected workflow. That change should be release notable, either in the release headline notes or in the breaking changes section (although it is not formally a breaking change).
Right. Upgraded to "fix".
However, note that nothing is broken. There is new advice for how the prereqs command is spelled, but the old spelling remains intact. (We'll deprecate the old spelling eventually, but not here.)
doc/_pages/bazel.md line 43 at r4 (raw file):
Previously, ggould-tri wrote…
BTW, author's discretion, this seems to be a broader feature IDEs; vscode also has this property, at least by default.
Enh. I don't have any basis to judge. I'll let a VSCode user open a PR with any docs fixes that are specific to their IDE.
For example in our clion.md we say "To use Drake with CLion, your Drake source directory (or git clone) must be named drake." and thus we have a reminder of that here. Once vscode.md decides to say something similar, then we can amend this section at the same time
There is now a single install_prereqs script for all platforms, which defaults to installing only the build prereqs (not developer prereqs). (Future work will further consolidate the implementation of the prereqs into simpler files.)
Retool our from_source (user) and bazel (developer) pages to use the new script and generally rewrite large chunks for improved clarity.
There are basically four main flows we focus on now:
(1) Typical users who install from binary (
installation.html).(2) Advanced users who need/decide to install from source (
from_source.html).(3) Drake developers/contributors (
bazel.html).(4) Downstream projects that already use Bazel (the small tail end of
from_source.html).The goal here is to make each of those four flows have a particular home in our docs, to have each of their flows be simple / easy / robust, and to redirect people who start from the wrong page (wrong flow) back into the flow they should be using.
This change is