Use the source version of drake's install_prereqs for external examples #313
Conversation
BetsyMcPhail
force-pushed
the
split-up-jenkinsfile
branch
5 times, most recently
from
26ed3a0
to
74c8fce
Compare
BetsyMcPhail
force-pushed
the
split-up-jenkinsfile
branch
from
74c8fce
to
38717a6
Compare
BetsyMcPhail
changed the title
There was a problem hiding this comment.
+@nicolecheetham for review
Reviewable status: all discussions resolved, LGTM missing from assignee nicolecheetham, platform LGTM missing (waiting on @nicolecheetham)
BetsyMcPhail
force-pushed
the
split-up-jenkinsfile
branch
from
38717a6
to
ef99218
Compare
There was a problem hiding this comment.
The Jenkins job can't find the ubuntu install_prereqs file. Everything else looks good
Reviewable status: all discussions resolved, LGTM missing from assignee nicolecheetham, platform LGTM missing
There was a problem hiding this comment.
Something must have been lost in the rebase! I will fix it.
Reviewable status: all discussions resolved, LGTM missing from assignee nicolecheetham, platform LGTM missing (waiting on @nicolecheetham)
BetsyMcPhail
force-pushed
the
split-up-jenkinsfile
branch
10 times, most recently
from
f4555d7
to
a320c09
Compare
There was a problem hiding this comment.
@nicolecheetham this is ready for another look. I used the relevant changes from your PR #333.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee nicolecheetham, platform LGTM missing (waiting on @nicolecheetham)
drake_bazel_external/.github/setup line 9 at r3 (raw file):
sudo ./.github/ubuntu_setup # Download the drake source
Some background, in both the CMake and Bazel cases, the following needs to happen for setup:
- CI-only setup as sudo (
.github/ubuntu_setup) - Get drake source NOT as sudo, in a location that can written to as the normal user AND, for the bazel example, NOT in a subdirectory to the current example (e.g.
drake_bazel_external/<not here>) - Run the install script from 2 as sudo
I've removed setup/install_prereqs script, placed the code here and copied it in the READMEs. To avoid duplication, I could move this chunk of code to setup/install_prereqs, but I'm not sure where the best place to install drake source?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee nicolecheetham, platform LGTM missing (waiting on @BetsyMcPhail)
drake_bazel_external/.github/setup line 9 at r3 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
Some background, in both the CMake and Bazel cases, the following needs to happen for setup:
- CI-only setup as sudo (
.github/ubuntu_setup)- Get drake source NOT as sudo, in a location that can written to as the normal user AND, for the bazel example, NOT in a subdirectory to the current example (e.g.
drake_bazel_external/<not here>)- Run the install script from 2 as sudo
I've removed
setup/install_prereqsscript, placed the code here and copied it in the READMEs. To avoid duplication, I could move this chunk of code tosetup/install_prereqs, but I'm not sure where the best place to install drake source?
I agree with moving the download and installation to setup/install_prereqs. It would also keep the README instructions simple. And if installing the drake source can't be done in a subdirectory why don't you tell people to put it right next to the specific example directory? This way it will be close by.
BetsyMcPhail
force-pushed
the
split-up-jenkinsfile
branch
2 times, most recently
from
e880da1
to
91b1e27
Compare
Previously, the binary version of the script was called in addition to replicating much of the source version. With this change, the copy-and-paste is removed.
BetsyMcPhail
force-pushed
the
split-up-jenkinsfile
branch
from
91b1e27
to
a7cbdc4
Compare
There was a problem hiding this comment.
@nicolecheetham this is ready for another look please
Reviewable status: 1 unresolved discussion, LGTM missing from assignee nicolecheetham, platform LGTM missing (waiting on @nicolecheetham)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM missing (waiting on @BetsyMcPhail)
There was a problem hiding this comment.
+@xuchenhan-tri for platform review please. Note that the Mac failure is a known CI issue and that neither of the examples updated here are tested on Mac.
Reviewable status: all discussions resolved, LGTM missing from assignee xuchenhan-tri, platform LGTM missing (waiting on @nicolecheetham and @xuchenhan-tri)
There was a problem hiding this comment.
Reviewed 6 of 15 files at r2, 1 of 13 files at r3, 13 of 13 files at r4, all commit messages.
Reviewable status:complete! all discussions resolved, platform LGTM from [xuchenhan-tri] (waiting on @BetsyMcPhail)
Towards #61
This change is