Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 1 | --- |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 2 | title: 'How to submit a patch' |
| 3 | linkTitle: 'How to submit a patch' |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 4 | --- |
| 5 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 6 | ## Configure git |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 7 | |
| 8 | <!--?prettify lang=sh?--> |
| 9 | |
| 10 | git config --global user.name "Your Name" |
| 11 | git config --global user.email you@example.com |
| 12 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 13 | ## Making changes |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 14 | |
| 15 | First create a branch for your changes: |
| 16 | |
| 17 | <!--?prettify lang=sh?--> |
| 18 | |
| 19 | git config branch.autosetuprebase always |
Ravi Mistry | e5be65e | 2021-05-20 16:01:33 -0400 | [diff] [blame] | 20 | git checkout -b my_feature origin/main |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 21 | |
| 22 | After making your changes, create a commit |
| 23 | |
| 24 | <!--?prettify lang=sh?--> |
| 25 | |
| 26 | git add [file1] [file2] ... |
| 27 | git commit |
| 28 | |
| 29 | If your branch gets out of date, you will need to update it: |
| 30 | |
| 31 | <!--?prettify lang=sh?--> |
| 32 | |
| 33 | git pull |
| 34 | python2 tools/git-sync-deps |
| 35 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 36 | ## Adding a unit test |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 37 | |
| 38 | If you are willing to change Skia codebase, it's nice to add a test at the same |
| 39 | time. Skia has a simple unittest framework so you can add a case to it. |
| 40 | |
| 41 | Test code is located under the 'tests' directory. |
| 42 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 43 | See [Writing Unit and Rendering Tests](/docs/dev/testing/tests) for details. |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 44 | |
| 45 | Unit tests are best, but if your change touches rendering and you can't think of |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 46 | an automated way to verify the results, consider writing a GM test. Also, if |
| 47 | your change is in the GPU code, you may not be able to write it as part of the |
| 48 | standard unit test suite, but there are GPU-specific testing paths you can |
| 49 | extend. |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 50 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 51 | ## Submitting a patch |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 52 | |
| 53 | For your code to be accepted into the codebase, you must complete the |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 54 | [Individual Contributor License Agreement](http://code.google.com/legal/individual-cla-v1.0.html). |
| 55 | You can do this online, and it only takes a minute. If you are contributing on |
| 56 | behalf of a corporation, you must fill out the |
| 57 | [Corporate Contributor License Agreement](http://code.google.com/legal/corporate-cla-v1.0.html) |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 58 | and send it to us as described on that page. Add your (or your organization's) |
| 59 | name and contact info to the AUTHORS file as a part of your CL. |
| 60 | |
| 61 | Now that you've made a change and written a test for it, it's ready for the code |
| 62 | review! Submit a patch and getting it reviewed is fairly easy with depot tools. |
| 63 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 64 | Use `git-cl`, which comes with |
| 65 | [depot tools](http://sites.google.com/a/chromium.org/dev/developers/how-tos/install-depot-tools). |
| 66 | For help, run `git cl help`. Note that in order for `git cl` to work correctly, |
| 67 | it needs to run on a clone of <https://skia.googlesource.com/skia>. Using clones |
| 68 | of mirrors, including Google's mirror on GitHub, might lead to issues with |
| 69 | `git cl` usage. |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 70 | |
| 71 | ### Find a reviewer |
| 72 | |
| 73 | Ideally, the reviewer is someone who is familiar with the area of code you are |
| 74 | touching. Look at the git blame for the file to see who else has been editing |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 75 | it. If unsuccessful, another option is to click on the 'Suggested Reviewers' |
| 76 | button to add one of the listed Skia contacts. They should be able to add |
| 77 | appropriate reviewers for your change. The button is located here: |
| 78 | <img src="/docs/dev/contrib/SuggestedReviewers.png" style="display: inline-block; max-width: 75%" /> |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 79 | |
| 80 | ### Uploading changes for review |
| 81 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 82 | Skia uses the Gerrit code review tool. Skia's instance is |
| 83 | [skia-review](http://skia-review.googlesource.com). Use `git cl` to upload your |
| 84 | change: |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 85 | |
| 86 | <!--?prettify lang=sh?--> |
| 87 | |
| 88 | git cl upload |
| 89 | |
| 90 | You may have to enter a Google Account username and password to authenticate |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 91 | yourself to Gerrit. A free gmail account will do fine, or any other type of |
| 92 | Google account. It does not have to match the email address you configured using |
| 93 | `git config --global user.email` above, but it can. |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 94 | |
| 95 | The command output should include a URL, similar to |
| 96 | (https://skia-review.googlesource.com/c/4559/), indicating where your changelist |
| 97 | can be reviewed. |
| 98 | |
| 99 | ### Submit try jobs |
| 100 | |
| 101 | Skia's trybots allow testing and verification of changes before they land in the |
| 102 | repo. You need to have permission to trigger try jobs; if you need permission, |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 103 | ask a committer. After uploading your CL to |
| 104 | [Gerrit](https://skia-review.googlesource.com/), you may trigger a try job for |
| 105 | any job listed in tasks.json, either via the Gerrit UI, using `git cl try`, eg. |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 106 | |
| 107 | git cl try -B skia.primary -b Some-Tryjob-Name |
| 108 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 109 | or using bin/try, a small wrapper for `git cl try` which helps to choose try |
| 110 | jobs. From a Skia checkout: |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 111 | |
| 112 | bin/try --list |
| 113 | |
| 114 | You can also search using regular expressions: |
| 115 | |
| 116 | bin/try "Test.*GTX660.*Release" |
| 117 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 118 | For more information about testing, see |
| 119 | [testing infrastructure](/docs/dev/testing/automated_testing). |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 120 | |
| 121 | ### Request review |
| 122 | |
| 123 | Go to the supplied URL or go to the code review page and select the **Your** |
| 124 | dropdown and click on **Changes**. Select the change you want to submit for |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 125 | review and click **Reply**. Enter at least one reviewer's email address. Now add |
| 126 | any optional notes, and send your change off for review by clicking on **Send**. |
| 127 | Unless you send your change to reviewers, no one will know to look at it. |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 128 | |
| 129 | _Note_: If you don't see editing commands on the review page, click **Sign in** |
| 130 | in the upper right. _Hint_: You can add -r reviewer@example.com --send-mail to |
| 131 | send the email directly when uploading a change using `git-cl`. |
| 132 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 133 | ## The review process |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 134 | |
| 135 | If you submit a giant patch, or do a bunch of work without discussing it with |
| 136 | the relevant people, you may have a hard time convincing anyone to review it! |
| 137 | |
| 138 | Code reviews are an important part of the engineering process. The reviewer will |
| 139 | almost always have suggestions or style fixes for you, and it's important not to |
| 140 | take such suggestions personally or as a commentary on your abilities or ideas. |
| 141 | This is a process where we work together to make sure that the highest quality |
| 142 | code gets submitted! |
| 143 | |
| 144 | You will likely get email back from the reviewer with comments. Fix these and |
| 145 | update the patch set in the issue by uploading again. The upload will explain |
| 146 | that it is updating the current CL and ask you for a message explaining the |
| 147 | change. Be sure to respond to all comments before you request review of an |
| 148 | update. |
| 149 | |
| 150 | If you need to update code the code on an already uploaded CL, simply edit the |
| 151 | code, commit it again locally, and then run git cl upload again e.g. |
| 152 | |
| 153 | echo "GOATS" > whitespace.txt |
| 154 | git add whitespace.txt |
| 155 | git commit -m 'add GOATS fix to whitespace.txt' |
| 156 | git cl upload |
| 157 | |
| 158 | Once you're ready for another review, use **Reply** again to send another |
| 159 | notification (it is helpful to tell the reviewer what you did with respect to |
| 160 | each of their comments). When the reviewer is happy with your patch, they will |
| 161 | approve your change by setting the Code-Review label to "+1". |
| 162 | |
| 163 | _Note_: As you work through the review process, both you and your reviewers |
| 164 | should converse using the code review interface, and send notes. |
| 165 | |
| 166 | Once your change has received an approval, you can click the "Submit to CQ" |
| 167 | button on the codereview page and it will be committed on your behalf. |
| 168 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 169 | Once your commit has gone in, you should delete the branch containing your |
| 170 | change: |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 171 | |
Ravi Mistry | e5be65e | 2021-05-20 16:01:33 -0400 | [diff] [blame] | 172 | git checkout -q origin/main |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 173 | git branch -D my_feature |
| 174 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 175 | ## Final Testing |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 176 | |
| 177 | Skia's principal downstream user is Chromium, and any change to Skia rendering |
| 178 | output can break Chromium. If your change alters rendering in any way, you are |
| 179 | expected to test for and alleviate this. You may be able to find a Skia team |
| 180 | member to help you, but the onus remains on each individual contributor to avoid |
| 181 | breaking Chrome. |
| 182 | |
| 183 | ### Evaluating Impact on Chromium |
| 184 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 185 | Keep in mind that Skia is rolled daily into Blink and Chromium. Run local tests |
| 186 | and watch canary bots for results to ensure no impact. If you are submitting |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 187 | changes that will impact layout tests, follow the guides below and/or work with |
| 188 | your friendly Skia-Blink engineer to evaluate, rebaseline, and land your |
| 189 | changes. |
| 190 | |
| 191 | Resources: |
| 192 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 193 | [How to land Skia changes that change Blink layout test results](/docs/dev/chrome/blink/) |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 194 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 195 | If you're changing the Skia API, you may need to make an associated change in |
| 196 | Chromium. If you do, please follow these instructions: |
| 197 | [Landing Skia changes which require Chrome changes](/docs/dev/chrome/changes/) |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 198 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 199 | ## Check in your changes |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 200 | |
| 201 | ### Non-Skia-committers |
| 202 | |
| 203 | If you already have committer rights, you can follow the directions below to |
| 204 | commit your change directly to Skia's repository. |
| 205 | |
| 206 | If you don't have committer rights in https://skia.googlesource.com/skia.git ... |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 207 | first of all, thanks for submitting your patch! We really appreciate these |
| 208 | submissions. After receiving an approval from a committer, you will be able to |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 209 | click the "Submit to CQ" button and submit your patch via the commit queue. |
| 210 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 211 | In special instances, a Skia committer may assist you in landing the change by |
| 212 | uploading a new codereview containing your patch (perhaps with some small |
| 213 | adjustments at their discretion). If so, you can mark your change as |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 214 | "Abandoned", and update it with a link to the new codereview. |
| 215 | |
| 216 | ### Skia committers |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 217 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 218 | - tips on how to apply an externally provided patch are [here](../patch) |
| 219 | - when landing externally contributed patches, please note the original |
| 220 | contributor's identity (and provide a link to the original codereview) in the |
| 221 | commit message |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 222 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 223 | `git-cl` will squash all your commits into a single one with the description |
| 224 | you used when you uploaded your change. |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 225 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 226 | ``` |
| 227 | git cl land |
| 228 | ``` |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 229 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 230 | or |
Joe Gregorio | 02f7202 | 2021-03-27 10:12:45 -0400 | [diff] [blame] | 231 | |
Joe Gregorio | e296c56 | 2021-04-05 11:39:40 -0400 | [diff] [blame] | 232 | ``` |
| 233 | git cl land -c 'Contributor Name <email@example.com>' |
| 234 | ``` |