CONTRIBUTING.md

   1 # Contributing to SPIR-V Tools
   2
   3 ## For users: Reporting bugs and requesting features
   4
   5 We organize known future work in GitHub projects. See [Tracking SPIRV-Tools work
   6 with GitHub
   7 projects](https://github.com/KhronosGroup/SPIRV-Tools/blob/master/projects.md)
   8 for more.
   9
  10 To report a new bug or request a new feature, please file a GitHub issue. Please
  11 ensure 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
  14 not already been reported open a new one
  15 [here](https://github.com/KhronosGroup/SPIRV-Tools/issues/new).
  16
  17 When 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
  30 For feature requests, we use
  31 [issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) as well. Please
  32 create 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
  39 Before we can use your code, you must sign the [Khronos Open Source Contributor
  40 License Agreement](https://cla-assistant.io/KhronosGroup/SPIRV-Tools) (CLA),
  41 which you can do online. The CLA is necessary mainly because you own the
  42 copyright to your changes, even after your contribution becomes part of our
  43 codebase, so we need your permission to use and distribute your code. We also
  44 need to be sure of various other things -- for instance that you'll tell us if
  45 you know that your code infringes on other people's patents. You don't have to
  46 sign the CLA until after you've submitted your code for review and a member has
  47 approved it, but you must do it before we can put your code into our codebase.
  48
  49 See
  50 [README.md](https://github.com/KhronosGroup/SPIRV-Tools/blob/master/README.md)
  51 for instruction on how to get, build, and test the source. Once you have made
  52 your 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
  67 The reviewer can either approve your PR or request changes. If changes are
  68 requested:
  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
  77 After the PR has been reviewed it is the job of the reviewer to merge the PR.
  78 Instructions for this are given below.
  79
  80 ## For maintainers: Reviewing a PR
  81
  82 The formal code reviews are done on GitHub. Reviewers are to look for all of the
  83 usual 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
  91 When looking for functional problems, there are some common problems reviewers
  92 should pay particular attention to:
  93
  94 *   Does the code work for both Shader (Vulkan and OpenGL) and Kernel (OpenCL)
  95     scenarios? The respective SPIR-V dialects are slightly different.
  96 *   Changes are made to a container while iterating through it. You have to be
  97     careful that iterators are not invalidated or that elements are not skipped.
  98 *   C++11 and VS2013. We generally assume that we have a C++11 compliant
  99     compiler. However, on Windows, we still support Visual Studio 2013, which is
 100     not fully C++11 compliant. See
 101     [here](https://msdn.microsoft.com/en-us/library/hh567368.aspx). In
 102     particular, note that it does not provide default move-constructors or
 103     move-assignments for classes. In general, r-value references do not work the
 104     way you might assume they do.
 105 *   For SPIR-V transforms: The module is changed, but the analyses are not
 106     updated. For example, a new instruction is added, but the def-use manager is
 107     not updated. Later on, it is possible that the def-use manager will be used,
 108     and give wrong results.
 109
 110 ## For maintainers: Merging a PR
 111
 112 We intend to maintain a linear history on the GitHub master branch, and the
 113 build and its tests should pass at each commit in that history. A linear
 114 always-working history is easier to understand and to bisect in case we want to
 115 find which commit introduced a bug.
 116
 117 ### Initial merge setup
 118
 119 The following steps should be done exactly once (when you are about to merge a
 120 PR for the first time):
 121
 122 *   It is assumed that upstream points to
 123     [git@github.com](mailto:git@github.com):KhronosGroup/SPIRV-Tools.git or
 124     https://github.com/KhronosGroup/SPIRV-Tools.git.
 125
 126 *   Find out the local name for the main github repo in your git configuration.
 127     For example, in this configuration, it is labeled `upstream`.
 128
 129     ```
 130     git remote -v
 131     [ ... ]
 132     upstream https://github.com/KhronosGroup/SPIRV-Tools.git (fetch)
 133     upstream https://github.com/KhronosGroup/SPIRV-Tools.git (push)
 134     ```
 135
 136 *   Make sure that the `upstream` remote is set to fetch from the `refs/pull`
 137     namespace:
 138
 139     ```
 140     git config --get-all remote.upstream.fetch
 141     +refs/heads/*:refs/remotes/upstream/*
 142     +refs/pull/*/head:refs/remotes/upstream/pr/*
 143     ```
 144
 145 *   If the line `+refs/pull/*/head:refs/remotes/upstream/pr/*` is not present in
 146     your configuration, you can add it with the command:
 147
 148     ```
 149     git config --local --add remote.upstream.fetch '+refs/pull/*/head:refs/remotes/upstream/pr/*'
 150     ```
 151
 152 ### Merge workflow
 153
 154 The following steps should be done for every PR that you intend to merge:
 155
 156 *   Make sure your local copy of the master branch is up to date:
 157
 158     ```
 159     git checkout master
 160     git pull
 161     ```
 162
 163 *   Fetch all pull requests refs:
 164
 165     ```
 166     git fetch upstream
 167     ```
 168
 169 *   Checkout the particular pull request you are going to review:
 170
 171     ```
 172     git checkout pr/1048
 173     ```
 174
 175 *   Rebase the PR on top of the master branch. If there are conflicts, send it
 176     back to the author and ask them to rebase. During the interactive rebase be
 177     sure to squash all of the commits down to a single commit.
 178
 179     ```
 180     git rebase -i master
 181     ```
 182
 183 *   **Build and test the PR.**
 184
 185 *   If all of the tests pass, push the commit `git push upstream HEAD:master`
 186
 187 *   Close the PR and add a comment saying it was push using the commit that you
 188     just pushed. See https://github.com/KhronosGroup/SPIRV-Tools/pull/935 as an
 189     example.