-
Notifications
You must be signed in to change notification settings - Fork 57
Add CI step for drake_cmake_external dependencies #474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
tyler-yankee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, platform LGTM missing
a discussion (no related file):
Working
This one will sort of be debug-on-the-fly (unless it completely devolves, and then I'll debug on my fork only).
Currently nothing is being printed in Actions and the job is passing, which I wouldn't expect because this branch isn't rebased to include the recent updates (and I don't think Actions merges with main?).
4ad86e5 to
53b32a0
Compare
77430af to
d98ea56
Compare
tyler-yankee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-(status: not ready for review) -(status: not ready to merge)
Okay, so the key thing in terms of verifying this works is that GHA (the actions/checkout step) creates a merge commit with main, as seen here:
Note: switching to 'refs/remotes/pull/474/merge'.
HEAD is now at 429ad0a Merge 77430af888d5c97aa5170185cec6dde3c8e8259f into f75c4a40a25b3c0df62e30da90bc213feb78504d
This means that this will currently pass in CI. However, this git branch is not rebased on the recent commit which actually did the upgrades, so if you run locally (without the merge commit), you will see a failure, as we'd expect.
I copy-pasted the contents of this step into a Bash script, as well as ran under a neat tool to locally debug GHA steps: https://github.com/nektos/act. Both failed locally, which I would expect as described above.
So, aside from a reviewer verifying my logic above is correct, I think the only thing left to hash out is semantics on the workflow itself. Could +a:@BetsyMcPhail please feature review or defer if needed?
Reviewable status: all discussions resolved, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)
tyler-yankee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)
.github/workflows/ci.yml line 62 at r2 (raw file):
python ./private/test/file_sync_test.py check_cmake_deps: name: sync drake_cmake_external deps
We could consider only running this on nightly, continuous, or PR.
I think nightly possibly makes the most sense, because landing a Drake change would be independent of this repo, and nightly would then pick up on the latest Drake master. Running on PRs makes sense, because it verifies that a PR which does the upgrades works. Continuous then verifies that change on main.
The only issue with the PR case is that some unrelated PR could experience a failure which is really main's fault. (Although this is just how CI goes in general...)
Just wanted to raise the discussion, but I think it makes sense to run this job all the time.
tyler-yankee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-assignee:@BetsyMcPhail +a:@Aiden2244 for feature review, please.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee aiden2244, platform LGTM missing (waiting on @Aiden2244)
Aiden2244
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested using act on tri-workstation (sorry for taking so long!).
@Aiden2244 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, platform LGTM missing.
Aiden2244
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+a:@sherm1 for platform review please, per schedule.
@Aiden2244 made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1, platform LGTM missing (waiting on @sherm1).
tyler-yankee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyler-yankee resolved 1 discussion.
Reviewable status: all discussions resolved, LGTM missing from assignee sherm1, platform LGTM missing (waiting on @sherm1).
tyler-yankee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sherm1: sorry for any confusion -- need corresponding review+merge here as for Drake -- thanks!
@tyler-yankee made 1 comment.
Reviewable status: all discussions resolved, LGTM missing from assignee sherm1, platform LGTM missing (waiting on @sherm1).
sherm1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reassigning to +a:@jwnimmer-tri as SME for platform review (and also today's on-call). -a:@sherm1
@sherm1 made 1 comment.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri).
jwnimmer-tri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwnimmer-tri reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: 1 unresolved discussion, platform LGTM from [jwnimmer-tri] (waiting on @tyler-yankee).
.github/workflows/ci.yml line 73 at r2 (raw file):
python ./private/upgrade_cmake_externals.py | tee output.log output="$(cat output.log)" if [[ -n "$(echo $output)" ]]; then
BTW Do we really need a named variable?
Suggestion:
if [[ -n "$(cat output.log)" ]]; thend98ea56 to
d6e1494
Compare
tyler-yankee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyler-yankee made 1 comment and resolved 1 discussion.
Reviewable status:complete! all discussions resolved, platform LGTM from [jwnimmer-tri] (waiting on @tyler-yankee).
.github/workflows/ci.yml line 73 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Do we really need a named variable?
done
Use the new upgrade script in CI to verify that dependencies remain synced over time.
jwnimmer-tri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwnimmer-tri reviewed 1 file and all commit messages.
Reviewable status:complete! all discussions resolved, platform LGTM from [jwnimmer-tri] (waiting on @tyler-yankee).
Use the new upgrade script in CI to verify that dependencies remain synced over time.
Towards #432.
This change is