-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[workspace] Upgrade lcm to latest release v1.5.1 #22379
Conversation
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.
+@sammy-tri for both reviews, please.
Please also test on TRI's robots, if necessary.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
tools/workspace/lcm/repository.bzl
line 12 at r1 (raw file):
When updating, the version numbers within the two lcm-*.cmake files in this directory must also be updated to match the new version. """,
nit Please also add to the advice here that LCM needs to be its own PR, not part of the big one.
986f384
to
5a6a15e
Compare
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 sammy-tri(platform)
tools/workspace/lcm/repository.bzl
line 12 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Please also add to the advice here that LCM needs to be its own PR, not part of the big one.
The upgrade advice has been updated accordingly.
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
tools/workspace/lcm/repository.bzl
line 12 at r2 (raw file):
When updating, the version numbers within the two lcm-*.cmake files in this directory must also be updated to match the new version. Moreover, lcm needs its own pull request separate from the rest of the monthly
nit Trailing whitespace at end of line. (I imagine the linter is going to yell.)
5a6a15e
to
7fa8be5
Compare
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 sammy-tri(platform)
tools/workspace/lcm/repository.bzl
line 12 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Trailing whitespace at end of line. (I imagine the linter is going to yell.)
Should be removed now
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: LGTM missing from assignee sammy-tri(platform)
tools/workspace/lcm/repository.bzl
line 12 at r2 (raw file):
Previously, nicolecheetham (Nicole C.) wrote…
Should be removed now
FYI When posting a "nit" or "BTW" the nominal action on the author side is to resolve the discussion on their own, either by clicking "Resolve" with no comment, or by posting "Done" which automatically resolves the thread, or if writing out a longer reply, to use the other UI element(s) to mark it as resolved. You don't need to wait for or ping the reviewer to return to the discussion -- saying "nit" or "BTW" is the reviewer's way of communicating that they are OK without explicitly closing the loop.
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.
Reviewed 2 of 3 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignee sammy-tri(platform)
This reverts commit b912eee.
Towards: #22348
This change is