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 6[Tracking SPIRV-Tools work with GitHub projects](https://github.com/KhronosGroup/SPIRV-Tools/blob/main/docs/projects.md) 7for more. 8 9To report a new bug or request a new feature, please file a GitHub issue. Please 10ensure the bug has not already been reported by searching 11[issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) and 12[projects](https://github.com/KhronosGroup/SPIRV-Tools/projects). If the bug has 13not already been reported open a new one 14[here](https://github.com/KhronosGroup/SPIRV-Tools/issues/new). 15 16When opening a new issue for a bug, make sure you provide the following: 17 18* A clear and descriptive title. 19 * We want a title that will make it easy for people to remember what the 20 issue is about. Simply using "Segfault in spirv-opt" is not helpful 21 because there could be (but hopefully aren't) multiple bugs with 22 segmentation faults with different causes. 23* A test case that exposes the bug, with the steps and commands to reproduce 24 it. 25 * The easier it is for a developer to reproduce the problem, the quicker a 26 fix can be found and verified. It will also make it easier for someone 27 to possibly realize the bug is related to another issue. 28 29For feature requests, we use 30[issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) as well. Please 31create a new issue, as with bugs. In the issue provide 32 33* A description of the problem that needs to be solved. 34* Examples that demonstrate the problem. 35 36## For developers: Contributing a patch 37 38Before we can use your code, you must sign the 39[Khronos Open Source Contributor License Agreement](https://cla-assistant.io/KhronosGroup/SPIRV-Tools) 40(CLA), which you can do online. The CLA is necessary mainly because you own the 41copyright to your changes, even after your contribution becomes part of our 42codebase, so we need your permission to use and distribute your code. We also 43need to be sure of various other things -- for instance that you'll tell us if 44you know that your code infringes on other people's patents. You don't have to 45sign the CLA until after you've submitted your code for review and a member has 46approved it, but you must do it before we can put your code into our codebase. 47 48See 49[README.md](https://github.com/KhronosGroup/SPIRV-Tools/blob/main/README.md) 50for instruction on how to get, build, and test the source. Once you have made 51your changes: 52 53* Ensure the code follows the 54 [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html). 55 Running `clang-format -style=file -i [modified-files]` can help. 56* Create a pull request (PR) with your patch. 57* Make sure the PR description clearly identified the problem, explains the 58 solution, and references the issue if applicable. 59* If your patch completely fixes bug 1234, the commit message should say 60 `Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1234` When you do 61 this, the issue will be closed automatically when the commit goes into 62 main. Also, this helps us update the [CHANGES](CHANGES) file. 63* Watch the continuous builds to make sure they pass. 64* Request a code review. 65 66The reviewer can either approve your PR or request changes. If changes are 67requested: 68 69* Please add new commits to your branch, instead of amending your commit. 70 Adding new commits makes it easier for the reviewer to see what has changed 71 since the last review. 72* Once you are ready for another round of reviews, add a comment at the 73 bottom, such as "Ready for review" or "Please take a look" (or "PTAL"). This 74 explicit handoff is useful when responding with multiple small commits. 75 76After the PR has been reviewed it is the job of the reviewer to merge the PR. 77Instructions for this are given below. 78 79## For maintainers: Reviewing a PR 80 81The formal code reviews are done on GitHub. Reviewers are to look for all of the 82usual things: 83 84* Coding style follows the 85 [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html) 86* Identify potential functional problems. 87* Identify code duplication. 88* Ensure the unit tests have enough coverage. 89* Ensure continuous integration (CI) bots run on the PR. If not run (in the 90 case of PRs by external contributors), add the "kokoro:run" label to the 91 pull request which will trigger running all CI jobs. 92 93When looking for functional problems, there are some common problems reviewers 94should pay particular attention to: 95 96* Does the code work for both Shader (Vulkan and OpenGL) and Kernel (OpenCL) 97 scenarios? The respective SPIR-V dialects are slightly different. 98* Changes are made to a container while iterating through it. You have to be 99 careful that iterators are not invalidated or that elements are not skipped. 100* For SPIR-V transforms: The module is changed, but the analyses are not 101 updated. For example, a new instruction is added, but the def-use manager is 102 not updated. Later on, it is possible that the def-use manager will be used, 103 and give wrong results. 104* If a pass gets the id of a type from the type manager, make sure the type is 105 not a struct or array. It there are two structs that look the same, the type 106 manager can return the wrong one. 107 108## For maintainers: Merging a PR 109 110We intend to maintain a linear history on the GitHub main branch, and the 111build and its tests should pass at each commit in that history. A linear 112always-working history is easier to understand and to bisect in case we want to 113find which commit introduced a bug. The 114[Squash and Merge](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits) 115button on the GitHub web interface. All other ways of merging on the web 116interface have been disabled. 117 118Before merging, we generally require: 119 1201. All tests except for the smoke test pass. See 121 [failing smoke test](#failing-smoke-test). 1221. The PR is approved by at least one of the maintainers. If the PR modifies 123 different parts of the code, then multiple reviewers might be necessary. 124 125The squash-and-merge button will turn green when these requirements are met. 126Maintainers have the to power to merge even if the button is not green, but that 127is discouraged. 128 129### Failing smoke test 130 131The purpose of the smoke test is to let us know if 132[shaderc](https://github.com/google/shaderc) fails to build with the change. If 133it fails, the maintainer needs to determine if the reason for the failure is a 134problem in the current PR or if another repository needs to be changed. Most of 135the time [Glslang](https://github.com/KhronosGroup/glslang) needs to be updated 136to account for the change in SPIR-V Tools. 137 138The PR can still be merged if the problem is not with that PR. 139 140## For maintainers: Running tests 141 142For security reasons, not all tests will run automatically. When they do not, a 143maintainer will have to start the tests. 144 145If the Github actions tests do not run on a PR, they can be initiated by closing 146and reopening the PR. 147 148If the kokoro tests are not run, they can be run by adding the label 149`kokoro:run` to the PR. 150