[workspace] Reorganize patches headed to upstream #22052
Conversation
jwnimmer-tri
added
priority: low
release notes: none
|
+@rpoyner-tri and +@SeanCurtis-TRI for reviews, please. |
There was a problem hiding this comment.
Reviewed 107 of 107 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)
tools/workspace/clarabel_cpp_internal/patches/sdp.patch line 4 at r1 (raw file):
Reasoning for not upstreaming this patch: changing an option, which is our perogative but not relevant for upstream.
typo
Suggestion:
prerogative
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)
There was a problem hiding this comment.
I didn't go through and confirm consistency and correctness (i.e., were files not put in upstream that should be or vice versa). I'm relying on CI to report that the files that did move and their application in repository.bzl is consistent.
Let me know if a deeper independent survey is required from platform review.
(All I have is some questions on the guidance in README.md.)
Reviewed 106 of 107 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions
tools/workspace/README.md line 196 at r2 (raw file):
When we need to adjust an upstream source release for our purposes, we do that with `*.patch` files in Drake, not by forking the repository and pointing to the fork.
In this PR you've provided a bunch of reasons for why patches are not included in the upstream directory. Is that something we want to actively encourage? If so, something should probably be said here.
tools/workspace/README.md line 201 at r2 (raw file):
change should be upstreamed (even if we haven't filed that pull request yet), put the patch in tools/workspace/foobar/patches/upstream/quux.patch to make it easier to search for patches that require follow-up action.
It's not clear to me what the guiding rule is when it comes to upstream. Should it go in upstream if we feel the patch as is should be upstreamed. Or some behavior akin to the patch should be upstreamed. For example, civil_time_linkopts.patch explicitly says that as patches go, it's fine for Drake, but upstream would require something different.
So, perhaps the instructions should elaborate a bit more on this nuance.
There was a problem hiding this comment.
I didn't go through and confirm consistency and correctness (i.e., were files not put in upstream that should be or vice versa).
Hopefully Rico did that to some degree. Anyway, it probably doesn't need to be perfect. Getting it 80% right is still an improvement.
Reviewable status: 2 unresolved discussions
tools/workspace/README.md line 196 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
In this PR you've provided a bunch of reasons for why patches are not included in the upstream directory. Is that something we want to actively encourage? If so, something should probably be said here.
Done.
tools/workspace/README.md line 201 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
It's not clear to me what the guiding rule is when it comes to upstream. Should it go in upstream if we feel the patch as is should be upstreamed. Or some behavior akin to the patch should be upstreamed. For example,
civil_time_linkopts.patchexplicitly says that as patches go, it's fine for Drake, but upstream would require something different.So, perhaps the instructions should elaborate a bit more on this nuance.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions
This change is