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)
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.
This change is