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

[workspace] Upgrade lcm to latest release v1.5.1 #22379

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

nicolecheetham
Copy link
Contributor

@nicolecheetham nicolecheetham commented Jan 2, 2025

Towards: #22348


This change is Reviewable

@nicolecheetham nicolecheetham reopened this Jan 6, 2025
@nicolecheetham nicolecheetham added release notes: fix This pull request contains fixes (no new features) status: single reviewer ok https://drake.mit.edu/reviewable.html labels Jan 6, 2025
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.

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

Copy link
Contributor Author

@nicolecheetham nicolecheetham 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, 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.

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.

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.)

Copy link
Contributor Author

@nicolecheetham nicolecheetham 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, 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

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: 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.

@nicolecheetham nicolecheetham marked this pull request as ready for review January 6, 2025 18:01
Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

Tested locally. :lgtm:

Reviewed 2 of 3 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee sammy-tri(platform)

@sammy-tri sammy-tri merged commit b912eee into RobotLocomotion:master Jan 9, 2025
10 checks passed
jwnimmer-tri added a commit that referenced this pull request Jan 13, 2025
@jwnimmer-tri jwnimmer-tri added release notes: none This pull request should not be mentioned in the release notes and removed release notes: fix This pull request contains fixes (no new features) labels Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants