1# Contributing to SPIR-V Tools 2 3## For users: Reporting bugs and requesting features 4 5We organize known future work in GitHub projects. See [Tracking SPIRV-Tools work 6with GitHub 7projects](https://github.com/KhronosGroup/SPIRV-Tools/blob/master/projects.md) 8for more. 9 10To report a new bug or request a new feature, please file a GitHub issue. Please 11ensure the bug has not already been reported by searching 12[issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) and 13[projects](https://github.com/KhronosGroup/SPIRV-Tools/projects). If the bug has 14not already been reported open a new one 15[here](https://github.com/KhronosGroup/SPIRV-Tools/issues/new). 16 17When opening a new issue for a bug, make sure you provide the following: 18 19* A clear and descriptive title. 20 * We want a title that will make it easy for people to remember what the 21 issue is about. Simply using "Segfault in spirv-opt" is not helpful 22 because there could be (but hopefully aren't) multiple bugs with 23 segmentation faults with different causes. 24* A test case that exposes the bug, with the steps and commands to reproduce 25 it. 26 * The easier it is for a developer to reproduce the problem, the quicker a 27 fix can be found and verified. It will also make it easier for someone 28 to possibly realize the bug is related to another issue. 29 30For feature requests, we use 31[issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) as well. Please 32create a new issue, as with bugs. In the issue provide 33 34* A description of the problem that needs to be solved. 35* Examples that demonstrate the problem. 36 37## For developers: Contributing a patch 38 39Before we can use your code, you must sign the [Khronos Open Source Contributor 40License Agreement](https://cla-assistant.io/KhronosGroup/SPIRV-Tools) (CLA), 41which you can do online. The CLA is necessary mainly because you own the 42copyright to your changes, even after your contribution becomes part of our 43codebase, so we need your permission to use and distribute your code. We also 44need to be sure of various other things -- for instance that you'll tell us if 45you know that your code infringes on other people's patents. You don't have to 46sign the CLA until after you've submitted your code for review and a member has 47approved it, but you must do it before we can put your code into our codebase. 48 49See 50[README.md](https://github.com/KhronosGroup/SPIRV-Tools/blob/master/README.md) 51for instruction on how to get, build, and test the source. Once you have made 52your changes: 53 54* Ensure the code follows the [Google C++ Style 55 Guide](https://google.github.io/styleguide/cppguide.html). Running 56 `clang-format -style=file -i [modified-files]` can help. 57* Create a pull request (PR) with your patch. 58* Make sure the PR description clearly identified the problem, explains the 59 solution, and references the issue if applicable. 60* If your patch completely fixes bug 1234, the commit message should say 61 `Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1234` 62 When you do this, the issue will be closed automatically when the commit 63 goes into master. Also, this helps us update the [CHANGES](CHANGES) file. 64* Watch the continuous builds to make sure they pass. 65* Request a code review. 66 67The reviewer can either approve your PR or request changes. If changes are 68requested: 69 70* Please add new commits to your branch, instead of amending your commit. 71 Adding new commits makes it easier for the reviewer to see what has changed 72 since the last review. 73* Once you are ready for another round of reviews, add a comment at the 74 bottom, such as "Ready for review" or "Please take a look" (or "PTAL"). This 75 explicit handoff is useful when responding with multiple small commits. 76 77After the PR has been reviewed it is the job of the reviewer to merge the PR. 78Instructions for this are given below. 79 80## For maintainers: Reviewing a PR 81 82The formal code reviews are done on GitHub. Reviewers are to look for all of the 83usual things: 84 85* Coding style follows the [Google C++ Style 86 Guide](https://google.github.io/styleguide/cppguide.html) 87* Identify potential functional problems. 88* Identify code duplication. 89* Ensure the unit tests have enough coverage. 90* Ensure continuous integration (CI) bots run on the PR. If not run (in the 91 case of PRs by external contributors), add the "kokoro:run" label to the 92 pull request which will trigger running all CI jobs. 93 94When looking for functional problems, there are some common problems reviewers 95should pay particular attention to: 96 97* Does the code work for both Shader (Vulkan and OpenGL) and Kernel (OpenCL) 98 scenarios? The respective SPIR-V dialects are slightly different. 99* Changes are made to a container while iterating through it. You have to be 100 careful that iterators are not invalidated or that elements are not skipped. 101* C++11 and VS2013. We generally assume that we have a C++11 compliant 102 compiler. However, on Windows, we still support Visual Studio 2013, which is 103 not fully C++11 compliant. See 104 [here](https://msdn.microsoft.com/en-us/library/hh567368.aspx). In 105 particular, note that it does not provide default move-constructors or 106 move-assignments for classes. In general, r-value references do not work the 107 way you might assume they do. 108* For SPIR-V transforms: The module is changed, but the analyses are not 109 updated. For example, a new instruction is added, but the def-use manager is 110 not updated. Later on, it is possible that the def-use manager will be used, 111 and give wrong results. 112 113## For maintainers: Merging a PR 114 115We intend to maintain a linear history on the GitHub master branch, and the 116build and its tests should pass at each commit in that history. A linear 117always-working history is easier to understand and to bisect in case we want to 118find which commit introduced a bug. 119 120### Initial merge setup 121 122The following steps should be done exactly once (when you are about to merge a 123PR for the first time): 124 125* It is assumed that upstream points to 126 [git@github.com](mailto:git@github.com):KhronosGroup/SPIRV-Tools.git or 127 https://github.com/KhronosGroup/SPIRV-Tools.git. 128 129* Find out the local name for the main github repo in your git configuration. 130 For example, in this configuration, it is labeled `upstream`. 131 132 ``` 133 git remote -v 134 [ ... ] 135 upstream https://github.com/KhronosGroup/SPIRV-Tools.git (fetch) 136 upstream https://github.com/KhronosGroup/SPIRV-Tools.git (push) 137 ``` 138 139* Make sure that the `upstream` remote is set to fetch from the `refs/pull` 140 namespace: 141 142 ``` 143 git config --get-all remote.upstream.fetch 144 +refs/heads/*:refs/remotes/upstream/* 145 +refs/pull/*/head:refs/remotes/upstream/pr/* 146 ``` 147 148* If the line `+refs/pull/*/head:refs/remotes/upstream/pr/*` is not present in 149 your configuration, you can add it with the command: 150 151 ``` 152 git config --local --add remote.upstream.fetch '+refs/pull/*/head:refs/remotes/upstream/pr/*' 153 ``` 154 155### Merge workflow 156 157The following steps should be done for every PR that you intend to merge: 158 159* Make sure your local copy of the master branch is up to date: 160 161 ``` 162 git checkout master 163 git pull 164 ``` 165 166* Fetch all pull requests refs: 167 168 ``` 169 git fetch upstream 170 ``` 171 172* Checkout the particular pull request you are going to review: 173 174 ``` 175 git checkout pr/1048 176 ``` 177 178* Rebase the PR on top of the master branch. If there are conflicts, send it 179 back to the author and ask them to rebase. During the interactive rebase be 180 sure to squash all of the commits down to a single commit. 181 182 ``` 183 git rebase -i master 184 ``` 185 186* **Build and test the PR.** 187 188* If all of the tests pass, push the commit `git push upstream HEAD:master` 189 190* Close the PR and add a comment saying it was push using the commit that you 191 just pushed. See https://github.com/KhronosGroup/SPIRV-Tools/pull/935 as an 192 example. 193