Update documentation #357
Update documentation #357
Conversation
… remove deprecated reference to scripts/ directory in top-level README
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)
a discussion (no related file):
+@BetsyMcPhail for feature review, please!
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @tyler-yankee)
drake_bazel_external/README.md line 14 at r1 (raw file):
sudo setup/install_prereqs
See discussion in #359. @tyler-yankee what are your thoughts on how to proceed here?
drake_cmake_external/CMakeLists.txt line 101 at r1 (raw file):
# Or from a commit (download and use "shasum -a 256 'xxx.tar.gz'" on it to # get the URL_HASH. # URL https://github.com/RobotLocomotion/drake/archive/962483669d015ee2618585ace2c884598fbadbb5.tar.gz
It's not clear to me why these were updated?
drake_cmake_installed/README.md line 21 at r1 (raw file):
# 1) A specific version (date-stamped) # curl -O https://drake-packages.csail.mit.edu/drake/nightly/drake-20240214-jammy.tar.gz
BTW Some of the example lines begin with # and some don't. It seems like it would be more clear for the commands to not start with #?
README.md line 16 at r1 (raw file):
Scripts are provided for various CI instances in each example's respective subdirectory under `.github/`. The intended purpose of each is described below: * `github_actions`: exemplifies how to put a project depending on a Drake installation on GitHub Actions
Previously, github_actions and jenkins were subdirectories under scripts/continuous_integration. Please update this section to more accurately describe the current setup.
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)
README.md line 16 at r1 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
Previously,
github_actionsandjenkinswere subdirectories underscripts/continuous_integration. Please update this section to more accurately describe the current setup.
Updated for clarity.
drake_cmake_external/CMakeLists.txt line 101 at r1 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
It's not clear to me why these were updated?
The hashes left there were arbitrary, and I happened to leave the ones I was working with; I can revert those.
The main purpose of this change was to fix the typo in shasum.
drake_cmake_installed/README.md line 21 at r1 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
BTW Some of the example lines begin with
#and some don't. It seems like it would be more clear for the commands to not start with#?
Fair enough. I hadn't thought about this but it makes sense since this is a code block within a README, not an actual script.
|
Previously, BetsyMcPhail (Betsy McPhail) wrote…
Since this PR is intended to update documentation, and that issue involves updating scripts, I say we go ahead with the current version (be consistent in docs, even if technically wrong). Then I'll make a new branch with a PR to update the scripts with the new logic given by @jwnimmer-tri. With that said, I'm open to also fixing it here. |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, platform LGTM from [betsymcphail] (waiting on @jwnimmer-tri)
drake_bazel_external/README.md line 14 at r1 (raw file):
Previously, tyler-yankee wrote…
Since this PR is intended to update documentation, and that issue involves updating scripts, I say we go ahead with the current version (be consistent in docs, even if technically wrong). Then I'll make a new branch with a PR to update the scripts with the new logic given by @jwnimmer-tri.
With that said, I'm open to also fixing it here.
That sounds fine to me.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail]
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)
There was a problem hiding this comment.
Big picture this is on the right track (LGTM), but I'm probably not the right platform reviewer to be assigned, since I'm overly familiar with this content. Having a fresh set of platform eyes here is probably worthwhile.
Reviewed 3 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)
README.md line 14 at r2 (raw file):
## Continuous Integration Scripts are provided for Jenkins or GitHub Actions, depending on the example,
nit Remove trailing whitespace and ends of lines.
|
I guess I'd suggest +@rpoyner-tri for platform review, to help avoid overloading Grant tomorrow after the holiday. |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @rpoyner-tri and @tyler-yankee)
a discussion (no related file):
On a second look, the instructions in drake_bazel_download could be clarified a bit. The setup/install_prereqs script both downloads the latest binary package of drake and runs drake's setup scripts. This is not clear as written.
drake_cmake_installed/README.md line 15 at r2 (raw file):
# Various system dependencies sudo setup/install_prereqs
I had another look at the install_prereqs script. It installs drake, specifically https://drake-packages.csail.mit.edu/drake/nightly/drake-latest-[jammy|mac-arm64].tar.gz, to /opt and then runs drake's setup scripts. Therefore, both the section title and comment are not-quite-accurate.
The instructions below also install drake, but to $HOME/drake and don't mention the setup script.
Can you please look at how to make all of these instructions more clear/accurate?
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @BetsyMcPhail, @rpoyner-tri, and @tyler-yankee)
drake_cmake_installed/README.md line 15 at r2 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
I had another look at the
install_prereqsscript. It installs drake, specificallyhttps://drake-packages.csail.mit.edu/drake/nightly/drake-latest-[jammy|mac-arm64].tar.gz, to/optand then runs drake's setup scripts. Therefore, both the section title and comment are not-quite-accurate.The instructions below also install drake, but to $HOME/drake and don't mention the setup script.
Can you please look at how to make all of these instructions more clear/accurate?
In case it helps, the long-term direction we want to aim for is to stop using /opt in this particular example. (Of all the examples, the only only one that should use /opt is the drake_cmake_installed_apt.)
I thought we'd fixed the mis-use of /opt in #258 already, but it sounds like maybe we missed something.
Or course (as with the sudo thing), it's okay to make the docs consistent first and then change the logic in a follow-up, if that's easier.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @jwnimmer-tri, @rpoyner-tri, and @tyler-yankee)
drake_cmake_installed/README.md line 15 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
In case it helps, the long-term direction we want to aim for is to stop using
/optin this particular example. (Of all the examples, the only only one that should use/optis thedrake_cmake_installed_apt.)I thought we'd fixed the mis-use of
/optin #258 already, but it sounds like maybe we missed something.Or course (as with the sudo thing), it's okay to make the docs consistent first and then change the logic in a follow-up, if that's easier.
At the moment, both drake_bazel_download and drake_cmake_installed install to /opt within their setup/install_prereqs scripts.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @BetsyMcPhail and @rpoyner-tri)
drake_cmake_installed/README.md line 15 at r2 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
At the moment, both
drake_bazel_downloadanddrake_cmake_installedinstall to/optwithin theirsetup/install_prereqsscripts.
Okay, to follow the logic of the discussion above regarding sudo use, I'll update the docs for consistency here and then address this issue in a follow-up change by unpacking to $HOME instead.
consistent spaces between code blocks, use bash syntax highlighting, update description in drake_bazel_download
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @BetsyMcPhail, @jwnimmer-tri, and @rpoyner-tri)
a discussion (no related file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
On a second look, the instructions in
drake_bazel_downloadcould be clarified a bit. Thesetup/install_prereqsscript both downloads the latest binary package of drake and runs drake's setup scripts. This is not clear as written.
Fixed to match the statement made in the other READMEs: "First, run the `install_prereqs` script to download the Drake source to [location]. This also runs Drake's setup script to install the required Ubuntu packages:"
README.md line 14 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Remove trailing whitespace and ends of lines.
Fixed.
drake_cmake_installed/README.md line 15 at r2 (raw file):
Previously, tyler-yankee wrote…
Okay, to follow the logic of the discussion above regarding
sudouse, I'll update the docs for consistency here and then address this issue in a follow-up change by unpacking to$HOMEinstead.
Notably, drake_cmake_installed downloads to /opt/drake to run the setup script, but in the next step the README indicates to download to $HOME/drake for cmake. Seems like this can be cleaned up moving forward.
a discussion (no related file):
I also updated the markdown in the READMEs to use bash for code block syntax highlighting. Most had none before, while some had shell and some had bash.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @jwnimmer-tri and @rpoyner-tri)
drake_cmake_installed/README.md line 15 at r2 (raw file):
Previously, tyler-yankee wrote…
Notably,
drake_cmake_installeddownloads to/opt/draketo run the setup script, but in the next step theREADMEindicates to download to$HOME/drakeforcmake. Seems like this can be cleaned up moving forward.
Sounds reasonable to me
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri, platform LGTM from [betsymcphail] (waiting on @jwnimmer-tri and @rpoyner-tri)
There was a problem hiding this comment.
sudo situation is inconsistent across examples. I've recommended that some of them be removed.
Reviewed 1 of 3 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: 3 unresolved discussions, platform LGTM from [betsymcphail, rpoyner-tri] (waiting on @jwnimmer-tri and @tyler-yankee)
drake_bazel_external/README.md line 14 at r1 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
That sounds fine to me.
Having tried this one, I think think sudo should be removed here. I get that there may be problems with Docker etc., but I'm guessing the 80-90% use case is just naively following the instructions here as written. For that use of this README, sudo is strictly worse.
drake_cmake_external/README.md line 12 at r3 (raw file):
```bash sudo setup/install_prereqs
nit ditto above; sudo here is strictly worse.
drake_bazel_external_legacy/README.md line 20 at r3 (raw file):
```bash sudo setup/install_prereqs
nit ditto above; sudo here is strictly worse.
Updates to the docs for each example include the following:
gtestlocal install/build indrake_cmake_externalas it is included in the repoThis addresses #353.
This change is