Run Jenkins jobs on custom builds of Drake #376
Conversation
There was a problem hiding this comment.
+@williamjallen and +@Aiden2244 ... how about we do the same sort of thing for feature review on this that we did for #374? Let's hold off on testing @drake-jenkins-bot for now, since it probably should work but I'll need a PR on Drake first. If you want to run a test, run with parameters from this job and make sure to use this branch of DEE.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, LGTM missing from assignee aiden2244, platform LGTM missing (waiting on @Aiden2244 and @williamjallen)
There was a problem hiding this comment.
I should also note that the pipeline script on the Jenkins job linked above will eventually be put into drake-jenkins-jobs, but I wanted to test and get something working before delving into that adventure.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, LGTM missing from assignee aiden2244, platform LGTM missing (waiting on @Aiden2244 and @williamjallen)
There was a problem hiding this comment.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, platform LGTM missing (waiting on @williamjallen)
There was a problem hiding this comment.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, platform LGTM missing (waiting on @williamjallen)
There was a problem hiding this comment.
I did a code-only review. Things look good in general, but I'm worried about the duplicated case statements between examples. Does anything verify that they are all the same? Even if all that matters is having the end result be the same, I worry that having different code to do the same or similar tasks in different places will create a maintenance nightmare in the future.
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee williamjallen, platform LGTM missing (waiting on @Aiden2244)
drake_cmake_external/.github/ci_build_test line 23 at r1 (raw file):
chmod -R a+w build || true rm -rf build
This whitespace-only change isn't relevant to the rest of the PR and should be reverted.
There was a problem hiding this comment.
It is unfortunate that the only thing preventing us from including (drake_bazel_external/setup/install_prereqs, drake_cmake_installed/setup/install_prereqs) in the file_sync_test is the last line, passing --with-bazel to Drake's prereq installation.
Since it's only two examples, and they have some logical grouping (with Jenkins CI), I could see maintenance being slightly annoying in certain cases but not a nightmare.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee williamjallen, platform LGTM missing (waiting on @Aiden2244 and @williamjallen)
drake_cmake_external/.github/ci_build_test line 23 at r1 (raw file):
Previously, williamjallen (William Allen) wrote…
This whitespace-only change isn't relevant to the rest of the PR and should be reverted.
If I understand correctly, this is a whitespace after the newline termination. My editor must have automatically made the change.
I'd argue we're making enough modifications to this file in this PR, that a change to keep the whitespace and newline style consistent with the rest of the code base should be warranted.
There was a problem hiding this comment.
Comments have been addressed with reasonable justifications for the current approach.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, platform LGTM missing (waiting on @Aiden2244)
There was a problem hiding this comment.
+@jwnimmer-tri for platform review, please!
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @Aiden2244 and @jwnimmer-tri)
There was a problem hiding this comment.
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee williamjallen, platform LGTM from [jwnimmer-tri] (waiting on @Aiden2244)
drake_bazel_external/.bazelignore line 1 at r1 (raw file):
# Used for setup, do not build
nit This comment seems stale now? It is used for the build (sometimes).
Ditto in legacy.
drake_bazel_external_legacy/.github/ci_build_test line 10 at r1 (raw file):
# found in drake_bazel_external_legacy/WORKSPACE. export EXAMPLES_LOCAL_DRAKE_PATH=$(realpath drake) trap 'unset EXAMPLES_LOCAL_DRAKE_PATH' EXIT
nit Why is this here? Nothing runs after this script, does it? And certainly not with its same shell environment anyway?
drake_cmake_external/README.md line 40 at r1 (raw file):
* `DRAKE_COMMIT_HASH`: the commit hash * `DRAKE_COMMIT_SHA256`: the checksum of the archive downloaded from GitHub (download https://github.com/RobotLocomotion/drake/archive/<DRAKE_COMMIT_HASH>.tar.gz
nit I don't like copy-pasting the instructions for checksums into two places. We should remove one copy and (if necessary) replace it with an xref to the intact copy. If we think README is the best place, then the CMakeLists can just declare the variables and xref the README for the semantics and instructions.
drake_cmake_external/.github/ci_build_test line 38 at r1 (raw file):
cmake_args+=(-DDRAKE_COMMIT_HASH=${drake_commit_hash}) # Find the SHA-256 by downloading and running shasum.
I'm not sure why this is here. Can't we just leave it blank / unset? That's what we used to do.
If the CMakeLists requires that it's set now, then that's a regression that we need to fix in the CMakeLists. Users should not be strictly required to set it.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee williamjallen, platform LGTM from [jwnimmer-tri] (waiting on @Aiden2244 and @jwnimmer-tri)
drake_bazel_external_legacy/.github/ci_build_test line 10 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Why is this here? Nothing runs after this script, does it? And certainly not with its same shell environment anyway?
Yeah that's fair. I was originally just thinking to clean up after ourselves but there shouldn't be a need here.
drake_cmake_external/README.md line 40 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit I don't like copy-pasting the instructions for checksums into two places. We should remove one copy and (if necessary) replace it with an xref to the intact copy. If we think README is the best place, then the CMakeLists can just declare the variables and xref the README for the semantics and instructions.
Makes sense, I'll remove it from the CMakeLists and xref the README (leaving the comment # Options to override Drake's latest source on master with a specific commit. in CMakeLists, though).
drake_cmake_external/.github/ci_build_test line 38 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I'm not sure why this is here. Can't we just leave it blank / unset? That's what we used to do.
If the CMakeLists requires that it's set now, then that's a regression that we need to fix in the CMakeLists. Users should not be strictly required to set it.
We can leave it blank and remove this code. I hadn't actually realized that.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, platform LGTM from [jwnimmer-tri] (waiting on @Aiden2244)
tyler-yankee
force-pushed
the
drake-pr-ci-jenkins
branch
from
6b465bd to
4ed22f8
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, platform LGTM from [jwnimmer-tri] (waiting on @Aiden2244)
There was a problem hiding this comment.
Approving again to make Reviewable happy...
Reviewed all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, platform LGTM from [jwnimmer-tri] (waiting on @Aiden2244)
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, platform LGTM from [jwnimmer-tri] (waiting on @Aiden2244)
This introduces parameters on each of the Jenkins examples to be able to be run the CI jobs with alternative versions of Drake. Towards #22574, and a follow-up of #374.
Parameterization of Individual Examples
drake_bazel_external: uses Bazel's--override-moduleflag when callingbazel test //...drake_bazel_external_legacy: sets theEXAMPLES_LOCAL_DRAKE_PATHenvironment variable to use the locally pulled copy of Drakedrake_cmake_external: adds options to theCMakeLists.txtfor the commit hash and sha256 (no longer have to manually comment-out code for this - yay!)Other Incidental Changes
setup/install_prereqsand.gitHub/ci_build_testscripts as necessarydrake(and adds to.bazelignore, etc.) rather than the defaultdrake-master, since names would be inconsistent with arbitrary commit hashesResult Workflow
A developer with a PR on Drake could test their changes on DEE by requesting a build from the bot:
Of course, it's also possible to run via the Jenkins UI.The test Jenkins job has been deleted as part of Jenkins maintenance/updates, but the definition for it is coming via drake-jenkins-jobs #161.This change is