Skip to content

Conversation

@tyler-yankee
Copy link
Contributor

@tyler-yankee tyler-yankee commented Oct 21, 2025

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

-(status: do not merge) -(status: do not review) +a:@Aiden2244 for feature review, please.

Reviewable status: LGTM missing from assignee Aiden2244, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @tyler-yankee)

Copy link
Contributor

@Aiden2244 Aiden2244 left a comment

Choose a reason for hiding this comment

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

:lgtm:, barring nits.

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers


-- commits line 2 at r1:
nit "DEE packages" is a little vague to me, I might suggest "...upgrade DEE CMake externals" to be a little more specific and match the naming used in the new python script.


tools/workspace/README.md line 198 at r1 (raw file):

process.

The packages are listed in Drake's `MODULE.bazel`, and the corresponding user-

nit at the risk of being overly pedantic, "user-provided" should be newlined as a single word to avoid an erroneous space after the hyphen when this text is rendered on GitHub.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

thanks!

Possibly +a:@jwnimmer-tri would have more context on this PR for platform than regular on-call? Feel free to delegate as needed.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform)


-- commits line 2 at r1:

Previously, Aiden2244 (Aiden McCormack) wrote…

nit "DEE packages" is a little vague to me, I might suggest "...upgrade DEE CMake externals" to be a little more specific and match the naming used in the new python script.

done


tools/workspace/README.md line 198 at r1 (raw file):

Previously, Aiden2244 (Aiden McCormack) wrote…

nit at the risk of being overly pedantic, "user-provided" should be newlined as a single word to avoid an erroneous space after the hyphen when this text is rendered on GitHub.

done

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:

@jwnimmer-tri reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion


tools/workspace/README.md line 198 at r2 (raw file):

process.

The packages are listed in Drake's `MODULE.bazel`, and the corresponding

BTW This sentence could probably be more precise that thedrake/MODULE.bazel are the canonical versions (and drake_cmake_external/CMakeLists.txt are the derivative versions that should follow along).

@tyler-yankee tyler-yankee changed the title [doc] Add instructions to upgrade DEE packages [doc] Add instructions to upgrade DEE CMake externals Oct 22, 2025
Copy link
Contributor Author

@tyler-yankee tyler-yankee 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


tools/workspace/README.md line 198 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW This sentence could probably be more precise that thedrake/MODULE.bazel are the canonical versions (and drake_cmake_external/CMakeLists.txt are the derivative versions that should follow along).

I think I simplified while adding the clarification you were looking for, lmk your thoughts.

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.

@jwnimmer-tri reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),Aiden2244


tools/workspace/README.md line 198 at r2 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

I think I simplified while adding the clarification you were looking for, lmk your thoughts.

Definitely better.

@jwnimmer-tri jwnimmer-tri merged commit 79ccc4c into RobotLocomotion:master Oct 22, 2025
9 checks passed
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

Development

Successfully merging this pull request may close these issues.

3 participants