Run GHA jobs on custom builds of Drake

[tyler-yankee]

This introduces parameters on each of the GitHub Actions examples to be able to run the CI jobs with alternative versions of Drake. Towards #22574.

Parameterization for Individual Examples

• drake_cmake_installed: install an experimental package of Drake into $HOME/drake, and use that when cmake -DCMAKE_PREFIX_PATH=<path> .. is called to configure/generate
• drake_cmake_installed_apt: uses apt-get install <url> with a link to an experimental package
• drake_bazel_download: similar to drake_cmake_installed, but uses Bazel's --override-repository flag when calling bazel test //...
• drake_pip: uses pip install <url> with a link to wheels (note: does this after installing drake from requirements.txt, in case any other packages are added to that file in the future)
• drake_poetry: similar to drake_pip, but uses poetry add <url>

CI Config Modifications

• Modifies the CI config files to add optional parameters to the workflow_dispatch event.
  • linux_jammy_package_tar
  • linux_jammy_package_deb
  • mac_arm_sonoma_package_tar
  • linux_jammy_wheel
  • mac_arm_sonoma_wheel
• Overhauls the file_sync_test to add custom logic for the workflow_dispatch event. All parameters are defined in the root ci.yml, but only the parameters that are needed for a given example are defined in its subdirectory ci.yml. This makes syncing files a nontrivial problem.

Result Workflow

If any of the optional parameters are not given, the CI jobs run as they normally would on main with "stable" versions of Drake (nightly releases, master source, etc. depending on the example). The use case here is that a developer with a PR on Drake could test their changes on DEE with the following workflow:

• (Optionally) Run experimental packaging builds on Linux Jammy and macOS Sonoma and obtain the artifact URLs.
• (Optionally) Run experimental wheel builds on Linux Jammy and macOS Sonoma and obtain the artifact URLs.
• Navigate to DEE's Actions page and kick off a build of all GHA examples, inputting the URLs from above.

Testing

See the recent run on my DEE fork for verification of the new CI for all examples. Of course, verification that the existing CI (without workflow_dispatch and any associated parameters) will still occur via this PR.

---

This change is 

[williamjallen]

Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee aiden2244, platform LGTM missing (waiting on @Aiden2244)

---

drake_bazel_download/setup/install_prereqs line 75 at r1 (raw file):

drake_url="https://drake-packages.csail.mit.edu/drake/nightly/drake-latest-jammy.tar.gz"
[[ ! -z "${package_url}" ]] && drake_url="${package_url}"
wget -O drake.tar.gz ${drake_url}

Would something along the lines of this (note: untested) be cleaner? Similar changes could be made elsewhere as well.

Suggestion:

wget -O drake.tar.gz "${package_url:-https://drake-packages.csail.mit.edu/drake/nightly/drake-latest-jammy.tar.gz}"

---

private/test/file_sync_test.py line 222 at r1 (raw file):

    check_sep([root_ci_path, subdir_ci_path], seps[0])
    check_sep([root_ci_path, subdir_ci_path], seps[1])
    check_sep(paths, seps[2])

It feels wrong to use indices as magic values here. This will break if someone adds new elements to the seps list. Perhaps use a dict instead?

---

private/test/file_sync_test.py line 232 at r1 (raw file):

    # Custom logic for workflow dispatch based on options defined above.
    root_dispatch = (content[root_ci_path].split(seps[0]))[1].split(seps[1])[0].splitlines()
    subdir_dispatch = (content[subdir_ci_path].split(seps[0]))[1].split(seps[1])[0].splitlines()

Some of these parentheses appear to be unnecessary:

Suggestion:

    root_dispatch = content[root_ci_path].split(seps[0])[1].split(seps[1])[0].splitlines()
    subdir_dispatch = content[subdir_ci_path].split(seps[0])[1].split(seps[1])[0].splitlines()

---

private/test/file_sync_test.py line 235 at r1 (raw file):

    def get_option_definition(dispatch, option_line):
        dispatch_option = [option_line]
        l = dispatch.index(option_line) + 1

Let's give l a better name...

---

private/test/file_sync_test.py line 272 at r1 (raw file):

    root_conc = (content[root_ci_path].split(seps[1]))[1].split(seps[2])[0]
    subdir_conc = (content[subdir_ci_path].split(seps[1]))[1].split(seps[2])[0]

Same here:

Suggestion:

    root_conc = content[root_ci_path].split(seps[1])[1].split(seps[2])[0]
    subdir_conc = content[subdir_ci_path].split(seps[1])[1].split(seps[2])[0]

---

.github/workflows/pip.yml line 33 at r1 (raw file):

        working-directory: drake_pip
        run: |
          args=(--python-version '3.12')

Is there somewhere we can get the default value from so we don't have to change a bunch of places whenever python is next updated? The same applies throughout.

[tyler-yankee]

Reviewable status: 6 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee aiden2244, platform LGTM missing (waiting on @Aiden2244 and @williamjallen)

---

.github/workflows/pip.yml line 33 at r1 (raw file):

Previously, williamjallen (William Allen) wrote…

Is there somewhere we can get the default value from so we don't have to change a bunch of places whenever python is next updated? The same applies throughout.

Looks like I can set an environment variable in GHA, which I agree is much cleaner. Since I happen to have another PR out now addressing versioning in the PyPI examples (#375), I think I'm going to implement this feedback on that branch instead.

---

drake_bazel_download/setup/install_prereqs line 75 at r1 (raw file):

Previously, williamjallen (William Allen) wrote…

Would something along the lines of this (note: untested) be cleaner? Similar changes could be made elsewhere as well.

This is definitely cleaner, and WFM locally. It will only apply to drake_bazel_download and drake_cmake_installed, since in drake_cmake_installed_apt the difference in code is less trivial, and in the PyPI examples we make an additional call that doesn't run otherwise.

---

private/test/file_sync_test.py line 222 at r1 (raw file):

Previously, williamjallen (William Allen) wrote…

It feels wrong to use indices as magic values here. This will break if someone adds new elements to the seps list. Perhaps use a dict instead?

Fair enough. I did write the file_sync rather quick-and-dirty.

---

private/test/file_sync_test.py line 232 at r1 (raw file):

Previously, williamjallen (William Allen) wrote…

Some of these parentheses appear to be unnecessary:

Yeah, looks fine to me without.

[tyler-yankee]

private/test/file_sync_test.py line 235 at r1 (raw file):

Previously, williamjallen (William Allen) wrote…

Let's give l a better name...

Done.

[tyler-yankee]

private/test/file_sync_test.py line 272 at r1 (raw file):

Previously, williamjallen (William Allen) wrote…

Same here:

Done.

[Aiden2244]

Reviewed CI Workflow on RobotLocomotion/drake-external-examples and ran CI locally on my machine using act. In both cases, CI is building from passed-in parameters.

Reviewable status: 6 unresolved discussions, LGTM missing from assignee williamjallen, platform LGTM missing (waiting on @williamjallen)

[williamjallen]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee williamjallen, platform LGTM missing (waiting on @Aiden2244)

[tyler-yankee]

+@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)

[tyler-yankee]

Note that the CI error on drake_cmake_installed is due to #22703. Happy to re-run it once that fix has landed.

[jwnimmer-tri]

Checkpoint. I ran out of stream before doing the file_sync test.

Reviewed 20 of 23 files at r1, 4 of 5 files at r2, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @Aiden2244)

---

drake_bazel_download/.github/ubuntu_setup line 6 at r2 (raw file):

set -euxo pipefail

install_args=()

Why do we need this whole block of code? It seems like all its doing is copying our arguments down into the call to install_prereqs. Wouldn't it be simpler to just pass $@ down to install_prereqs directly? Sure, we're checking for syntax errors here, but so does install_prereqs.

---

drake_cmake_installed/.github/ubuntu_setup line 6 at r2 (raw file):

set -euxo pipefail

install_args=()

Why do we need this whole block of code? It seems like all its doing is copying our arguments down into the call to install_prereqs. Wouldn't it be simpler to just pass $@ down to install_prereqs directly? Sure, we're checking for syntax errors here, but so does install_prereqs.

---

drake_bazel_download/README.md line 43 at r2 (raw file):

You may generally want to stick to using `bazel run` when able.

### Using a local checkout of Drake

Working

I am not convinced that we should put this in the user-facing documentation. For one, I'm not sure we actually want to support this in the first place for users, and also I wonder whether it will confuse them more than it helps.

I'll ponder and post back again later.

---

drake_pip/setup/install_prereqs line 6 at r2 (raw file):

set -euxo pipefail

setup_env_args=()

Why do we need this whole block of code? Wouldn't it be simpler to just pass $@ down to setup_env directly?

---

drake_pip/.github/ubuntu_setup line 6 at r2 (raw file):

set -euxo pipefail

install_args=()

Why do we need this whole block of code? It seems like all its doing is copying our arguments down into the call to install_prereqs. Wouldn't it be simpler to just pass $@ down to install_prereqs directly? Sure, we're checking for syntax errors here, but so does install_prereqs.

---

drake_pip/setup/setup_env line 39 at r2 (raw file):

"env/bin/pip$python_version" install -r requirements.txt

# Use custom wheels if specified, on the respective OS.

nit I am not sure what this part of the comment means?

Code quote:

on the respective OS

---

drake_bazel_download/.github/ci_build_test line 21 at r2 (raw file):

drake_override_flag=()
if [[ ${drake_override} -eq 1 ]]; then

I am not sure why this is governed by a flag. It seems to me like if we set this override unconditionally then the correctness and coverage would be exactly the same (even when the CI build didn't specify a override), but the code complexity in these ci scripts would be much simpler and therefore easier / cheaper to maintain correctly?

[tyler-yankee]

Thanks, Jeremy - let me quickly run this by CI, then back to you for the file_sync_test.

Reviewable status: 7 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @Aiden2244, @jwnimmer-tri, and @williamjallen)

---

drake_bazel_download/.github/ci_build_test line 21 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am not sure why this is governed by a flag. It seems to me like if we set this override unconditionally then the correctness and coverage would be exactly the same (even when the CI build didn't specify a override), but the code complexity in these ci scripts would be much simpler and therefore easier / cheaper to maintain correctly?

I hadn't realized this; that always overriding to $HOME/drake doesn't affect the end user at all but simplifies our CI workflow and maintenance. I suppose there is a small caveat of not exercising any issues with Bazel's http_archive, but that would probably break lots of other things anyway ....

---

drake_bazel_download/.github/ubuntu_setup line 6 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why do we need this whole block of code? It seems like all its doing is copying our arguments down into the call to install_prereqs. Wouldn't it be simpler to just pass $@ down to install_prereqs directly? Sure, we're checking for syntax errors here, but so does install_prereqs.

Done.

---

drake_cmake_installed/.github/ubuntu_setup line 6 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why do we need this whole block of code? It seems like all its doing is copying our arguments down into the call to install_prereqs. Wouldn't it be simpler to just pass $@ down to install_prereqs directly? Sure, we're checking for syntax errors here, but so does install_prereqs.

Done.

---

drake_pip/.github/ubuntu_setup line 6 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why do we need this whole block of code? It seems like all its doing is copying our arguments down into the call to install_prereqs. Wouldn't it be simpler to just pass $@ down to install_prereqs directly? Sure, we're checking for syntax errors here, but so does install_prereqs.

Done.

---

drake_pip/setup/install_prereqs line 6 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why do we need this whole block of code? Wouldn't it be simpler to just pass $@ down to setup_env directly?

Done.

---

drake_pip/setup/setup_env line 39 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I am not sure what this part of the comment means?

I think this is leftover from an earlier implementation in which I was checking the OS to determine which URL to use; either way, should be removed now.

[tyler-yankee]

Note the positive workflow dispatch result with all parameters given: https://github.com/tyler-yankee/drake-external-examples/actions/runs/13683143567/job/38260144371.

[jwnimmer-tri]

Reviewed 8 of 8 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @Aiden2244 and @tyler-yankee)

---

drake_bazel_download/README.md line 43 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I am not convinced that we should put this in the user-facing documentation. For one, I'm not sure we actually want to support this in the first place for users, and also I wonder whether it will confuse them more than it helps.

I'll ponder and post back again later.

Yeah, definitely not in user-facing documentation. For end users, using a local copy would be replacing the http_archive in MODULE.bazel with a something else.

If it helps us as maintainers it's fine to keep around but it'll need to be moved to somewhere else.

---

private/test/file_sync_test.py line 48 at r4 (raw file):

    # runs in Jenkins, and only GHA has been updated so far.
    # Perhaps this can be brought back once the Jenkins changes
    # are introduced.

nit It doesn't seem to me like these will ever be in sync anymore. The BUILD and WORKSPACE symlinking that drake_bazel_download does now will never be part of the drake_bazel_external code. Remove the TODO?

---

private/test/file_sync_test.py line 230 at r4 (raw file):

        error(f"{workflow_name} subdir CI events do not match")

    # Custom logic for workflow dispatch based on options defined above.

This new logic is an unmaintainable disaster. I see a few choices for a path forward:

(0) Possibly we should move "concurrency" text up between "name" and "on"? I think this would reduce the number of different "seps" we need to parse by defragmenting the parts of the text which are identical. We could do this no matter which of 1..2 we choose below.

(1) Copy the full "workflow_dispatch" into every example, without trying to cull it. This means that there will be unused inputs declared in all of the examples, but possibly that is mostly harmless? We could revisit that in a follow-up PR if need be.

(2) Use a yaml parser (pyyaml) for the workflow_dispatch logic instead of doing it longhand with string-slicing stuff. For each of the examples:

• Load the main ci.yml with the yaml parser.
• Find the workflow_dispatch node.
• Delete the child nodes we don't want.
• yaml.dump the workflow_dispatch node back into a string (possibly padding it with leading whitespace on each line afterward).
• assert that this section of the example's ci.yaml text matches the dumped string.

Tradeoffs:
(1) is simpler but ends up with unused yaml code in each example.
(2) makes the examples cleaner but means we can't have yaml comments in the workflow_dispatch section (since the parser discards them).

[tyler-yankee]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @Aiden2244 and @jwnimmer-tri)

---

drake_bazel_download/README.md line 43 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yeah, definitely not in user-facing documentation. For end users, using a local copy would be replacing the http_archive in MODULE.bazel with a something else.

If it helps us as maintainers it's fine to keep around but it'll need to be moved to somewhere else.

That's fair, though I'd point out the original reason I put it there was to mirror the documentation of the --override_module flag inside drake_bazel_external/README.md. I don't have an opinion other than that the two examples should be kept consistent with each other.

---

private/test/file_sync_test.py line 48 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit It doesn't seem to me like these will ever be in sync anymore. The BUILD and WORKSPACE symlinking that drake_bazel_download does now will never be part of the drake_bazel_external code. Remove the TODO?

Fair point.

---

private/test/file_sync_test.py line 230 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This new logic is an unmaintainable disaster. I see a few choices for a path forward:

(0) Possibly we should move "concurrency" text up between "name" and "on"? I think this would reduce the number of different "seps" we need to parse by defragmenting the parts of the text which are identical. We could do this no matter which of 1..2 we choose below.

(1) Copy the full "workflow_dispatch" into every example, without trying to cull it. This means that there will be unused inputs declared in all of the examples, but possibly that is mostly harmless? We could revisit that in a follow-up PR if need be.

(2) Use a yaml parser (pyyaml) for the workflow_dispatch logic instead of doing it longhand with string-slicing stuff. For each of the examples:

• Load the main ci.yml with the yaml parser.
• Find the workflow_dispatch node.
• Delete the child nodes we don't want.
• yaml.dump the workflow_dispatch node back into a string (possibly padding it with leading whitespace on each line afterward).
• assert that this section of the example's ci.yaml text matches the dumped string.

Tradeoffs:
(1) is simpler but ends up with unused yaml code in each example.
(2) makes the examples cleaner but means we can't have yaml comments in the workflow_dispatch section (since the parser discards them).

I agree that this code is a mess; I wrote it understanding that the file_sync wasn't perfect but got the job done before, and perhaps this change is enough reason to make it more robust.

I should have explored moving the concurrency text; originally didn't want to touch it because I wasn't sure if it would work otherwise, and because the "on" text is more important to see towards the top of the file.

(1) is what I was trying to avoid. If the philosophy is to provide self-contained examples within each directory that give users a start on using Drake, including with lightweight GHA CI, then providing dead CI params that belong to the other examples flies in the face of that (despite being easier).

I'm not sure what you mean about (2) not allowing yaml comments; to resolve that, we could just load the subdirectory's workflow_dispatch section from string and immediately dump it. That just means we can't sync the comments between the two files.

[tyler-yankee]

private/test/file_sync_test.py line 230 at r4 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

I agree that this code is a mess; I wrote it understanding that the file_sync wasn't perfect but got the job done before, and perhaps this change is enough reason to make it more robust.

I should have explored moving the concurrency text; originally didn't want to touch it because I wasn't sure if it would work otherwise, and because the "on" text is more important to see towards the top of the file.

(1) is what I was trying to avoid. If the philosophy is to provide self-contained examples within each directory that give users a start on using Drake, including with lightweight GHA CI, then providing dead CI params that belong to the other examples flies in the face of that (despite being easier).

I'm not sure what you mean about (2) not allowing yaml comments; to resolve that, we could just load the subdirectory's workflow_dispatch section from string and immediately dump it. That just means we can't sync the comments between the two files.

Should clarify on concurrency; not necessarily that it's more important to see, but that almost always conventionally concurrency comes after "on".

[jwnimmer-tri]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @Aiden2244 and @tyler-yankee)

---

drake_bazel_download/README.md line 43 at r2 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

That's fair, though I'd point out the original reason I put it there was to mirror the documentation of the --override_module flag inside drake_bazel_external/README.md. I don't have an opinion other than that the two examples should be kept consistent with each other.

Yeah, I suppose ideally we would be consistent but I think removing this from README is the lesser of evils. The problem is the "repo_rules2" is an unstable implementation detail do we don't want it in the README. If we had a simpler recipe, it could go here, but this current one is too brittle.

---

private/test/file_sync_test.py line 230 at r4 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Should clarify on concurrency; not necessarily that it's more important to see, but that almost always conventionally concurrency comes after "on".

(0) Probably it's okay if you want to keep it as-is. We can see how the code looks without changing this.

(1) Fine be me to veto this option.

(2) My point is that the unit test shouldn't be "parse both yamls and make sure their parsed content are equal". The test needs to be "load the main yaml, compute the expected answer, and then check the subdir text for an exact textual match". We need the per-example files to allow no variation from the expectation, and there is no reasonable way (that I can think of) to compute the expectation from the main file without losing any comments. I guess the main file could still have comments and just not the per-subdir ones. Maybe that's okay? I guess I was thinking if there is a comment in the main file which gets lost in translation, that would be bad.

[jwnimmer-tri]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @Aiden2244 and @tyler-yankee)

---

drake_bazel_download/README.md line 43 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yeah, I suppose ideally we would be consistent but I think removing this from README is the lesser of evils. The problem is the "repo_rules2" is an unstable implementation detail do we don't want it in the README. If we had a simpler recipe, it could go here, but this current one is too brittle.

On further thought, possibly if we change this example's MODULE.bazel file to use https://bazel.build/rules/lib/globals/module#archive_override instead of http_archive, then the --override_module spelling might work and be robust, and we could document it here.

(I'm OK if you want to try that here, or defer to a future PR, or skip it.)

[tyler-yankee]

drake_bazel_download/README.md line 43 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

On further thought, possibly if we change this example's MODULE.bazel file to use https://bazel.build/rules/lib/globals/module#archive_override instead of http_archive, then the --override_module spelling might work and be robust, and we could document it here.

(I'm OK if you want to try that here, or defer to a future PR, or skip it.)

I was just looking into that, but wasn't sure if this example was specifically targeting the use of http_archive. In that case, (assuming it works) I'm happy to sync up the examples in this PR.

[tyler-yankee]

private/test/file_sync_test.py line 230 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(0) Probably it's okay if you want to keep it as-is. We can see how the code looks without changing this.

(1) Fine be me to veto this option.

(2) My point is that the unit test shouldn't be "parse both yamls and make sure their parsed content are equal". The test needs to be "load the main yaml, compute the expected answer, and then check the subdir text for an exact textual match". We need the per-example files to allow no variation from the expectation, and there is no reasonable way (that I can think of) to compute the expectation from the main file without losing any comments. I guess the main file could still have comments and just not the per-subdir ones. Maybe that's okay? I guess I was thinking if there is a comment in the main file which gets lost in translation, that would be bad.

I agree that it's non-ideal to have comments lost in translation, but it might be the best option available save the current messy implementation.

[jwnimmer-tri]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee williamjallen, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @Aiden2244 and @tyler-yankee)

---

drake_bazel_download/README.md line 43 at r2 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

I was just looking into that, but wasn't sure if this example was specifically targeting the use of http_archive. In that case, (assuming it works) I'm happy to sync up the examples in this PR.

Changing would be OK with me. This example was written before drake_bazel_external was ported to bzlmod, so isn't quite as fresh.

I imagine the change looks like copying this part from drake_bazel_external/MODULE.bazel into here instead of the http_archive ...

# Here we introduce Drake as a module dependency, but note that Drake is not
# published to any Bazel registry. Below, we'll override it with a github
# source archive.
bazel_dep(name = "drake")

... and then using the archive_override(...) in MODULE.bazel to point to the package tgz.

---

private/test/file_sync_test.py line 230 at r4 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

I agree that it's non-ideal to have comments lost in translation, but it might be the best option available save the current messy implementation.

I'm okay with it.