Add GHA CI to subdirs and sync with main CI

[nicolecheetham]

Creates copies of the reusable workflows for each respective GHA subdir and adds a CI sync test to ensure the subdir CIs exactly match the two places it is copied from.

Issue: #309

---

This change is 

[nicolecheetham]

+@BetsyMcPhail for feature review please

Reviewable status: all discussions resolved, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)

[jwnimmer-tri]

Reviewed 15 of 20 files at r1.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail and @nicolecheetham)

---

drake_ament_cmake_installed/.github/workflows/ci.yml line 1 at r1 (raw file):

# SPDX-License-Identifier: MIT-0

This copy-pasted file (and likewise for all of the others) must have a CI test that confirms that the copy-paste-edit is up to date. Otherwise, this will fall out of sync with the actual files in drake-external-examples/.github/workflows that it's copied from.

---

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

        "drake_cmake_installed_apt/.clang-format",
    ),
    (

We don't want to allow this new divergence. In this PR we should also fix the Jenkinsfile to chdir prior to calling the setup script, so that all of the ubuntu_setup scripts can uniformly drop the cd command.

[nicolecheetham]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail and @jwnimmer-tri)

---

drake_ament_cmake_installed/.github/workflows/ci.yml line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This copy-pasted file (and likewise for all of the others) must have a CI test that confirms that the copy-paste-edit is up to date. Otherwise, this will fall out of sync with the actual files in drake-external-examples/.github/workflows that it's copied from.

Okay. I thought the CI test would be a separate PR. I will work on incorporating into this PR.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We don't want to allow this new divergence. In this PR we should also fix the Jenkinsfile to chdir prior to calling the setup script, so that all of the ubuntu_setup scripts can uniformly drop the cd command.

Okay, will do

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail and @nicolecheetham)

---

drake_ament_cmake_installed/.github/workflows/ci.yml line 1 at r1 (raw file):

Previously, nicolecheetham (Nicole C.) wrote…

Okay. I thought the CI test would be a separate PR. I will work on incorporating into this PR.

Reviewable tip -- when you reply to a thread along the lines of "working on it" that's fine, but in that case you need to click the little circle drop-down in the discussion thread and set your disposition to "Working" so that reviewable knows the next action is on your plate instead of mine, so it gives you the red bubble instead of me.

---

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

Previously, nicolecheetham (Nicole C.) wrote…

Okay, will do

Thanks

[nicolecheetham]

@jwnimmer-tri For the GHA ci file sync test, is it preferrable to keep the the files sorted or have easily understood test logic?

The specific example ci file stays in the same position (3rd) however the other two types of files (the main ci.yml and specific reusable workflow) change places depending on the name of the drake example. If sorted, I cannot assume the position of these two positions. Therefore, I would check if both top and bottom part of the specific ci file would be in either of the two files instead of checking if one subset is in the relevant original file.

[jwnimmer-tri]

Big picture, for new cross-checking logic in this PR, it could live inside file_sync_test.py or it could be a new test program, either way is fine. However, it should NOT re-use the COPIES array at all. We want totally new/novel logic.

... if one subset is in the relevant original file.

The test criterion we want is not "subset". We must programmatically compute the exactly right answer for what flavor_subdir/.github/ci.yml should look like, and then fail if it differs even by a single line or byte. Just checking a rough "subset" alone isn't useful, because there exist subsets which would pass the test but be invalid GHA instructions.

[jwnimmer-tri]

I carved #342 out of this pull request so that it can land sooner than later. After #342 merges, you'll need to merge up this PR to the latest main.

Note that in #342 I removed the superfluous ./ from all of the ./subdir/command lines. When providing a relative subdirectory path, is not conventional to start with a no-op ./ in front of the path. Just say subdir/command directly.

[jwnimmer-tri]

This is moving in a good direction. I will let Betsy finish feature review before I circle back for a final look.

Reviewed 26 of 26 files at r5, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)

[BetsyMcPhail]

I made a few updates to create out-of-sync files and ran the file_sync_test locally. I confirmed that it caught and reported the mismatches.

Reviewed 19 of 20 files at r1, 8 of 8 files at r2, 3 of 3 files at r3, 1 of 6 files at r4, 26 of 26 files at r5, all commit messages.
Reviewable status: 3 unresolved discussions, platform LGTM from [betsymcphail] (waiting on @nicolecheetham)

---

private/test/file_sync_test.py line 59 at r5 (raw file):

)

CIS = (

Minor Similar to the function name below, consider renaming to something github action / workflow specific. WORKFLOW_FILES or GITHUB_WORKFLOWS or something else ??

---

private/test/file_sync_test.py line 131 at r5 (raw file):

def ci_check(index: int, paths: tuple[str]):

Minor Consider changing the name of this function to reflect that it is checking github action ci files.

---

private/test/file_sync_test.py line 152 at r5 (raw file):

    
    if events + ex_jobs != content[paths[2]]:
        error(f"{prologue} does not match")

Minor The error message printed is ERROR: The 4th list of files (containing ci.yml) does not match It would be more helpful to use a different filename that helps identify which check failed.

[BetsyMcPhail]

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, platform LGTM from [betsymcphail] (waiting on @nicolecheetham)

[BetsyMcPhail]

@jwnimmer-tri this is ready for a final review

Reviewable status: complete! all discussions resolved, platform LGTM from [betsymcphail] (waiting on @nicolecheetham)

[jwnimmer-tri]

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @nicolecheetham)

---

private/test/file_sync_test.py line 69 at r6 (raw file):

)

GITHUB_WORKFLOWS = (

This list as written has more copy-paste than necessary, which makes it difficult to understand and brittle to maintain. All we really need is the list of projects that use GHA:

GITHUB_WORKFLOWS = (
    "ament_cmake_installed",
    "bazel_download",
    "catkin_installed",
    "cmake_installed",
    "cmake_installed_apt",
)

Then inside of the gha_workflow_check function, we can calculate what the filenames are supposed to be:

root_ci_path = ".github/workflows/ci.yml"
subdir_workflow_path = f".github/workflows/{name}.yml"
subdir_ci_path = f"drake_{name}/.github/workflows/ci.yml"

---

private/test/file_sync_test.py line 141 at r6 (raw file):

def gha_workflow_check(index: int, paths: tuple[str]):

nit In line with the above comment about the constants, the function signature here is overly complicated. All we want as input is the workflow_name: str or something like that.

---

private/test/file_sync_test.py line 142 at r6 (raw file):

def gha_workflow_check(index: int, paths: tuple[str]):
    # For reabability

nit Stray comment? (And it's misspelled...)

I'm not sure what this is trying to say; I imagine we don't need any comment here?

---

private/test/file_sync_test.py line 144 at r6 (raw file):

    # For reabability
    workflow_name = Path(paths[1]).name
    prologue = (f"The {_ordinalize(index + 1)} list of files"

nit Printing the index is unnecessary complication. The workflow_name is a sufficient statistic on its own to enable the developer to understand the error.

---

private/test/file_sync_test.py line 146 at r6 (raw file):

    prologue = (f"The {_ordinalize(index + 1)} list of files"
                f" (containing {workflow_name})")
    

nit Spurious horizontal whitespace; should just be a blank line.

---

private/test/file_sync_test.py line 151 at r6 (raw file):

    for path in paths:
        try:
            with open(path, "r") as f:

nit When opening files in text mode (not binary), we must always specify an encoding.

Suggestion:

with open(path, "r", encoding="utf-8") as f:

---

private/test/file_sync_test.py line 157 at r6 (raw file):

    paths = list(content.keys())

    # Check for workflow mismatch.

nit Either here in this comment (meh) or else in the docstring of the function (probably better), somewhere it must be made very clear to the readers of this code exactly what condition this is enforcing.

I believe the intention is that the subdir_ci is made up of the root_ci content up through the jobs: line plus the subdir_workflow content after the jobs: line.

---

private/test/file_sync_test.py line 158 at r6 (raw file):

    # Check for workflow mismatch.
    events = content[paths[0]].split("jobs:")[0]

These two split is extremely brittle. What if the token "jobs:" appears in a comment? We need to anchor the match to the start of the line. Ideally we would also fail-fast if there was >1 match to notice that something wend wrong.

---

private/test/file_sync_test.py line 160 at r6 (raw file):

    events = content[paths[0]].split("jobs:")[0]
    ex_jobs = "jobs:" + content[paths[1]].split("jobs:")[1]
    

nit Spurious horizontal whitespace; should just be a blank line.

[jwnimmer-tri]

For Drake & related projects, we use Reviewable to coordinate reviews. Once you've adjusted the for the discussion(s), open up Reviewable, reply to them, and publish the messages. (Sometimes the reply is just the "Resolve" button, with no message.) Reviewers (like me) won't look until they see the ping-back happen in Reviewable.

[nicolecheetham]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @BetsyMcPhail and @jwnimmer-tri)

---

private/test/file_sync_test.py line 142 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Stray comment? (And it's misspelled...)

I'm not sure what this is trying to say; I imagine we don't need any comment here?

I decided to just remove the confusing comment and instead went with a docstring.

---

private/test/file_sync_test.py line 157 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Either here in this comment (meh) or else in the docstring of the function (probably better), somewhere it must be made very clear to the readers of this code exactly what condition this is enforcing.

I believe the intention is that the subdir_ci is made up of the root_ci content up through the jobs: line plus the subdir_workflow content after the jobs: line.

Went with a docstring.

---

private/test/file_sync_test.py line 158 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

These two split is extremely brittle. What if the token "jobs:" appears in a comment? We need to anchor the match to the start of the line. Ideally we would also fail-fast if there was >1 match to notice that something wend wrong.

That is a good point, I hadn't thought about comments. To make the split less brittle, I had the separator be the case "jobs:" had to be on its own line. This should resolve the issue with having the word appear in a comment. Also, there is now a check that all files only have the separator once, otherwise it fails.

[jwnimmer-tri]

There are some small code-style nits that need a tweak here, but I'll fix them in a follow-up aloneside some other related changes.

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all discussions resolved, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @nicolecheetham)