Skip to content

Commit 155de6a

Browse files
README_DRAKE: Improve docs for reviewers
1 parent 58d9205 commit 155de6a

File tree

2 files changed

+62
-10
lines changed

2 files changed

+62
-10
lines changed

README_DRAKE.md

Lines changed: 62 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,62 @@ 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
155+
[checks](#partial-pull-merging); for each CI configuration, expand the output
156+
under `Python tests` (which are for C++17) - we shouldn't be accidentally
157+
missing tests, etc.)
158+
- [ ] `CI / 🐍 3.6 • ubuntu-latest • x64 [...]`: [base](<link>) vs. this PR's results for
159+
`CI / 🐍 3.6 • ubuntu-latest • x64`
160+
- [ ] `CI / 🐍 3.9 • macos-latest • x64`: [base](<link>) vs. this PR's results for
161+
`CI / 🐍 3.9 • macos-latest • x64`
162+
- [ ] Do the drake-specific changes look good? (all commits after the merge
163+
commit)
164+
```
165+
166+
To get `<link>` for the given base CI results:
138167

139-
Afterward, follow the normal PR process as outlined above.
168+
* Use the most recent result from the following link:
169+
[workflow: CI, branch: drake](https://github.com/RobotLocomotion/pybind11/actions?query=workflow%3ACI+branch%3Adrake+is%3Acompleted)
170+
* Click on the relevant job (i.e.
171+
`🐍 3.6 • ubuntu-latest • x64` or `🐍 3.9 • macos-latest • x64`)
172+
* Expand `Python tests C++17`, and click on the the line number where
173+
`short test summary info` is printed
174+
175+
Example:
176+
<https://github.com/RobotLocomotion/pybind11/runs/2036808833?check_suite_focus=true#step:17:63> (this link may
177+
expire, hence the screenshot)
178+
179+
<img src="./docs/drake-github-ci-example.png"/>
180+
181+
### Cherry-Pick Upstream Changes
182+
183+
This should only be done when there is an acute feature needed, but there is
184+
high overhead in incorporating all upstream changes.
185+
186+
Follow the same procedure as above, but name your PR as
187+
`Cherry pick <shas> from upstream/master`, and be sure to reference the
188+
upstream PRs.
189+
190+
Any non-trivial changes should still happen as separate commits to simplify
191+
review.

docs/drake-github-ci-example.png

141 KB
Loading

0 commit comments

Comments
 (0)