Skip to content
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

Upgrade Externals January 2023 #18516

Merged
merged 7 commits into from
Jan 6, 2023

Conversation

mwoehlke-kitware
Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware commented Jan 3, 2023

Towards #18510. Fixes #17910.


This change is Reviewable

@mwoehlke-kitware
Copy link
Contributor Author

+@jwnimmer-tri for feature review, please.

@jwnimmer-tri jwnimmer-tri added release notes: fix This pull request contains fixes (no new features) status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits status: single reviewer ok https://drake.mit.edu/reviewable.html labels Jan 4, 2023
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: both reviews +(status: single reviewer ok).

+(release notes: fix) +(status: commits are properly curated)

Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @mwoehlke-kitware)

@jwnimmer-tri
Copy link
Collaborator

@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please.


The https://github.com/RobotLocomotion/drake/blob/master/tools/workspace/README.md#semi-automated-monthly-upgrades document says:

Once the all Jenkins builds of the pull request have passed, additionally launch a macOS build, per

https://drake.mit.edu/jenkins.html#scheduling-an-on-demand-build

Once the macOS build passes, assign the pull request for review.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @mwoehlke-kitware)


-- commits line 2 at r7:
The README instructions say "add the commit using the instructions that were printed by new_release". The commit instructions printed by the tool do not include the git commit sha in the subject like (e.g., for abseil_cpp_internal):

...
Done.  Be sure to review and commit the changes:
  git add tools/workspace/abseil_cpp_internal/repository.bzl
  git commit -m'[workspace] Upgrade abseil_cpp_internal to latest commit'

Since we need to rebase this PR anyway prior to merge, during the rebase we might as well remove the commit sha's from the three subject lines where it crept in. That will save the release manager the effort of editing them out later on.

Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware left a 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 (waiting on @jwnimmer-tri)


-- commits line 2 at r7:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The README instructions say "add the commit using the instructions that were printed by new_release". The commit instructions printed by the tool do not include the git commit sha in the subject like (e.g., for abseil_cpp_internal):

...
Done.  Be sure to review and commit the changes:
  git add tools/workspace/abseil_cpp_internal/repository.bzl
  git commit -m'[workspace] Upgrade abseil_cpp_internal to latest commit'

Since we need to rebase this PR anyway prior to merge, during the rebase we might as well remove the commit sha's from the three subject lines where it crept in. That will save the release manager the effort of editing them out later on.

The README also gives an example of what to do, which includes the new "version". Maybe that example should either be more complete, or should not exist. (Although, as the script knows how to make the commit, why doesn't it just do so?)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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 (waiting on @mwoehlke-kitware)


-- commits line 2 at r7:

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

The README also gives an example of what to do, which includes the new "version". Maybe that example should either be more complete, or should not exist. (Although, as the script knows how to make the commit, why doesn't it just do so?)

Per f2f: yup, a command-line option to run the git commands on behalf of the user sounds find. That would reduce manual steps. (Users who are particular about how they invoke git could still commit manually.) In that case, the README could switch to use it, which would remove the misleading example commands.

@jwnimmer-tri
Copy link
Collaborator

This is just waiting on a rebase. Shall I push that on your behalf?

@mwoehlke-kitware
Copy link
Contributor Author

Done. Didn't I already do that once, though? IIRC, usually someone with force-merge permission does the final rebase, because otherwise the rebase needs to be coordinated with said person actually doing the merge. (Or get lucky that CI completes without master changing out from under the PR.)

@jwnimmer-tri jwnimmer-tri merged commit 04df3eb into RobotLocomotion:master Jan 6, 2023
@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Jan 6, 2023

Didn't I already do that once, though?

Not since review was assigned, at least according to my interpretation of the github timeline.

IIRC, usually someone with force-merge permission does the final rebase, because otherwise the rebase needs to be coordinated with said person actually doing the merge.

That's probably a valid way to do it, but I don't know if we've ever done it that way. My advice is to do the rebase and then immediately ping one of the two super-admins (myself and Sherm) and we can override CI to merge immediately.

@mwoehlke-kitware
Copy link
Contributor Author

(Rebased or not?) Huh. I could've sworn I rebased it when I removed the SHAs from the commit messages. Maybe I forgot to update my local master first. 😛

(Who rebases?) Hmm, I thought that was how it usually happened. Maybe I'm misremembering. 🤷

@mwoehlke-kitware
Copy link
Contributor Author

-- commits line 2 at r7:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Per f2f: yup, a command-line option to run the git commands on behalf of the user sounds find. That would reduce manual steps. (Users who are particular about how they invoke git could still commit manually.) In that case, the README could switch to use it, which would remove the misleading example commands.

(This thread is resolved. Also, see #18548.)

@mwoehlke-kitware mwoehlke-kitware deleted the upgrades branch January 6, 2023 19:05
@jwnimmer-tri
Copy link
Collaborator

I wonder if GitHub has a "rebase this PR onto the tip of master" button on the web somewhere. That would probably be the easiest (especially if platform reviewers could just click it).

@mwoehlke-kitware
Copy link
Contributor Author

Github has an option to rebase a PR. (I believe it doesn't create a merge commit, though, just adds the commits directly to master.)

RussTedrake added a commit to RussTedrake/drake that referenced this pull request Feb 8, 2023
The upgrade in RobotLocomotion#18516 broke meshcat on Deepnote again.  See RobotLocomotion#18289.
RussTedrake added a commit to RussTedrake/drake that referenced this pull request Feb 8, 2023
The upgrade in RobotLocomotion#18516 broke meshcat on Deepnote again.  See RobotLocomotion#18289.
RussTedrake added a commit to RussTedrake/drake that referenced this pull request Feb 8, 2023
The upgrade in RobotLocomotion#18516 broke meshcat on Deepnote again.  See RobotLocomotion#18289.
RussTedrake added a commit that referenced this pull request Feb 9, 2023
The upgrade in #18516 broke meshcat on Deepnote again.  See #18289.
marcoag pushed a commit to marcoag/drake that referenced this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features) status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

usockets and uwebsockets needs update to latest
2 participants