Skip to content

Commit b6b6fe8

Browse files
README_DRAKE: Add basic instructions to bootstrap upstream review
1 parent b255eb7 commit b6b6fe8

File tree

1 file changed

+56
-10
lines changed

1 file changed

+56
-10
lines changed

β€ŽREADME_DRAKE.md

Lines changed: 56 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,11 @@ features, first try to make a descriptive upstream issue to see if it is
3232
something desirable for upstream, and make a PR for the community to see and
3333
possibly review. Then continue developing here.
3434

35-
Review should happen using Reviewable.
35+
Reviewable for non-upstream updates should happen using Reviewable.
3636

37-
Please avoid superfluous (non-functional) changes to the original `pybind11`
38-
source code (e.g. no whitespace reflowing), and try to stay relatively close to
39-
`pybind11`s style for consistency.
37+
Please do not make superfluous (non-functional) changes to the original
38+
`pybind11` source code (e.g. no whitespace reflowing), and try to stay
39+
relatively close to `pybind11`s style for consistency.
4040

4141
## Continuous Integration
4242

@@ -102,7 +102,6 @@ review.
102102
experimental CI passes, and be sure to include macOS. An example of requesting
103103
macOS testing in Drake:
104104

105-
@drake-jenkins-bot mac-mojave-clang-bazel-experimental-release please
106105
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-everything-release please
107106

108107
* Once your PR is reviewed, accepted, and CI passes, merge your
@@ -115,7 +114,7 @@ merge commit for the fork:
115114

116115
* Merge the Drake PR once it passes CI and review is finished.
117116

118-
## Pulling Upstream Changes
117+
### Pulling Upstream Changes
119118

120119
This repository should be merged with upstream (the official repository) about
121120
every 3 months. We use a merge strategy (not rebase) with Git so that updates
@@ -131,9 +130,56 @@ To update the repository, first checkout the branch and merge:
131130
git merge upstream/master # Resolve conflicts and commit
132131
git push --set-upstream origin <new-branch-name>
133132

133+
If you need to make non-trivial merge-conflict resolutions, please do so as
134+
additional commits so that reviewers can easily separate these more acute
135+
changes from the merge itself. (This is so that reviewers only review *novel*
136+
code, rather than code that upstream reviewers already looked at.)
137+
134138
Then create a `RobotLocomotion/pybind11` PR with your branch. Title the PR as
135-
`Merge 'upstream/master' (<sha_new>) from previous merge-base (<sha_old>)`,
136-
where `<sha_new>` is the short-form SHA of the current `upstream/master`, and
137-
`<sha_old>` is the short-form SHA of prior merge-base you recorded.
139+
`Merge 'upstream/master' (<sha_new>)`,
140+
where `<sha_new>` is the short-form SHA of the current `upstream/master`.
141+
142+
In the PR's overview, please add the following checklist for both you and the
143+
reviewer (replacing anything in brackets `<...>` with the appropriate values:
144+
145+
```md
146+
For the author:
147+
148+
- [ ] Ensure downstream CI passes (including Mac): <link to Drake PR>
149+
150+
For the reviewer:
151+
152+
- [ ] Does the commit title look correct? (does it only contain up to
153+
`<sha_new>` of upstream?)
154+
- Are the canary builds "correct"? (look at the [checks](#partial-pull-merging); for each CI configuration, expand the output
155+
under `Python tests C++17` - we shouldn't be accidentally missing tests, etc.)
156+
- [ ] `🐍 3.6 β€’ ubuntu-latest β€’ x64`: [base](<link>) vs. this PR's results for
157+
`CI / 🐍 3.6 β€’ ubuntu-latest β€’ x64`
158+
- [ ] `🐍 3.8 β€’ macos-latest β€’ x64`: [base](<link>) vs. this PR's results for
159+
`CI / 🐍 3.8 β€’ macos-latest β€’ x64`
160+
- [ ] Do the drake-specific changes look good? (all commits after the merge
161+
commit)
162+
```
138163

139-
Afterward, follow the normal PR process as outlined above.
164+
To get `<link>` for the given base CI results:
165+
* Use the most recent result from the following link:
166+
<https://github.com/RobotLocomotion/pybind11/actions?query=workflow%3ACI+branch%3Adrake+is%3Acompleted>
167+
* Click on the relevant job (i.e.
168+
`🐍 3.6 β€’ ubuntu-latest β€’ x64` or `🐍 3.8 β€’ macos-latest β€’ x64`)
169+
* Expand `Python tests C++17`, and click on the the line number where
170+
`short test summary info` is printed
171+
172+
Example:
173+
<https://github.com/RobotLocomotion/pybind11/runs/1086934889?check_suite_focus=true#step:13:54>
174+
175+
### Cherry-Pick Upstream Changes
176+
177+
This should only be done when there is an acute feature needed, but there is
178+
high overhead in incorporating all upstream changes.
179+
180+
Follow the same procedure as above, but name your PR as
181+
`Cherry pick <shas> from upstream/master`, and be sure to reference the
182+
upstream PRs.
183+
184+
Any non-trivial changes should still happen as separate commits to simplify
185+
review.

0 commit comments

Comments
Β (0)