Re-format CONTRIBUTING.md - NFC.
I'm about to change this file. I re-formatted it for easier editing.
This commit is contained in:
Diego Novillo
Родитель
0dd4ee27b1
Коммит
2885c65daf
145
CONTRIBUTING.md
145
CONTRIBUTING.md
|
|
@ -1,95 +1,129 @@
|
||||||
# Contributing to SPIR-V Tools
|
# Contributing to SPIR-V Tools
|
||||||
|
|
||||||
|
|
||||||
## For users: Reporting bugs and requesting features
|
## For users: Reporting bugs and requesting features
|
||||||
|
|
||||||
|
We organize known future work in GitHub projects. See [Tracking SPIRV-Tools work
|
||||||
|
with GitHub
|
||||||
|
projects](https://github.com/KhronosGroup/SPIRV-Tools/blob/master/projects.md)
|
||||||
|
for more.
|
||||||
|
|
||||||
We organize known future work in GitHub projects. See [Tracking SPIRV-Tools work with GitHub projects](https://github.com/KhronosGroup/SPIRV-Tools/blob/master/projects.md) for more.
|
To report a new bug or request a new feature, please file a GitHub issue. Please
|
||||||
|
ensure the bug has not already been reported by searching
|
||||||
|
[issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) and
|
||||||
To report a new bug or request a new feature, please file a GitHub issue. Please ensure the bug has not already been reported by searching [issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) and [projects](https://github.com/KhronosGroup/SPIRV-Tools/projects). If the bug has not already been reported open a new one [here](https://github.com/KhronosGroup/SPIRV-Tools/issues/new).
|
[projects](https://github.com/KhronosGroup/SPIRV-Tools/projects). If the bug has
|
||||||
|
not already been reported open a new one
|
||||||
|
[here](https://github.com/KhronosGroup/SPIRV-Tools/issues/new).
|
||||||
|
|
||||||
When opening a new issue for a bug, make sure you provide the following:
|
When opening a new issue for a bug, make sure you provide the following:
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
* A clear and descriptive title.
|
* A clear and descriptive title.
|
||||||
* We want a title that will make it easy for people to remember what the issue is about. Simply using "Segfault in spirv-opt" is not helpful because there could be (but hopefully aren't) multiple bugs with segmentation faults with different causes.
|
* We want a title that will make it easy for people to remember what the
|
||||||
* A test case that exposes the bug, with the steps and commands to reproduce it.
|
issue is about. Simply using "Segfault in spirv-opt" is not helpful
|
||||||
* The easier it is for a developer to reproduce the problem, the quicker a fix can be found and verified. It will also make it easier for someone to possibly realize the bug is related to another issue.
|
because there could be (but hopefully aren't) multiple bugs with
|
||||||
|
segmentation faults with different causes.
|
||||||
For feature requests, we use [issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) as well. Please create a new issue, as with bugs. In the issue provide
|
* A test case that exposes the bug, with the steps and commands to reproduce
|
||||||
|
it.
|
||||||
|
* The easier it is for a developer to reproduce the problem, the quicker a
|
||||||
|
fix can be found and verified. It will also make it easier for someone
|
||||||
|
to possibly realize the bug is related to another issue.
|
||||||
|
|
||||||
|
For feature requests, we use
|
||||||
|
[issues](https://github.com/KhronosGroup/SPIRV-Tools/issues) as well. Please
|
||||||
|
create a new issue, as with bugs. In the issue provide
|
||||||
|
|
||||||
* A description of the problem that needs to be solved.
|
* A description of the problem that needs to be solved.
|
||||||
* Examples that demonstrate the problem.
|
* Examples that demonstrate the problem.
|
||||||
|
|
||||||
|
|
||||||
## For developers: Contributing a patch
|
## For developers: Contributing a patch
|
||||||
|
|
||||||
Before we can use your code, you must sign the [Khronos Open Source Contributor License Agreement](https://cla-assistant.io/KhronosGroup/SPIRV-Tools) (CLA), which you can do online. The CLA is necessary mainly because you own the copyright to your changes, even after your contribution becomes part of our codebase, so we need your permission to use and distribute your code. We also need to be sure of various other things -- for instance that you'll tell us if you know that your code infringes on other people's patents. You don't have to sign the CLA until after you've submitted your code for review and a member has approved it, but you must do it before we can put your code into our codebase.
|
Before we can use your code, you must sign the [Khronos Open Source Contributor
|
||||||
|
License Agreement](https://cla-assistant.io/KhronosGroup/SPIRV-Tools) (CLA),
|
||||||
|
which you can do online. The CLA is necessary mainly because you own the
|
||||||
|
copyright to your changes, even after your contribution becomes part of our
|
||||||
|
codebase, so we need your permission to use and distribute your code. We also
|
||||||
|
need to be sure of various other things -- for instance that you'll tell us if
|
||||||
|
you know that your code infringes on other people's patents. You don't have to
|
||||||
|
sign the CLA until after you've submitted your code for review and a member has
|
||||||
|
approved it, but you must do it before we can put your code into our codebase.
|
||||||
|
|
||||||
See [README.md](https://github.com/KhronosGroup/SPIRV-Tools/blob/master/README.md) for instruction on how to get, build, and test the source. Once you have made your changes:
|
See
|
||||||
|
[README.md](https://github.com/KhronosGroup/SPIRV-Tools/blob/master/README.md)
|
||||||
|
for instruction on how to get, build, and test the source. Once you have made
|
||||||
|
your changes:
|
||||||
|
|
||||||
|
* Ensure the code follows the [Google C++ Style
|
||||||
|
Guide](https://google.github.io/styleguide/cppguide.html). Running
|
||||||
* Ensure the code follows the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html). Running `clang-format -style=file -i [modified-files]` can help.
|
`clang-format -style=file -i [modified-files]` can help.
|
||||||
* Create a pull request (PR) with your patch.
|
* Create a pull request (PR) with your patch.
|
||||||
* Make sure the PR description clearly identified the problem, explains the solution, and references the issue if applicable.
|
* Make sure the PR description clearly identified the problem, explains the
|
||||||
|
solution, and references the issue if applicable.
|
||||||
* Watch the continuous builds to make sure they pass.
|
* Watch the continuous builds to make sure they pass.
|
||||||
* Request a code review.
|
* Request a code review.
|
||||||
|
|
||||||
The reviewer can either approve your PR or request changes. If changes are requested:
|
The reviewer can either approve your PR or request changes. If changes are
|
||||||
|
requested:
|
||||||
|
|
||||||
|
* Please add new commits to your branch, instead of amending your commit.
|
||||||
|
Adding new commits makes it easier for the reviewer to see what has changed
|
||||||
|
since the last review.
|
||||||
|
* Once you are ready for another round of reviews, add a comment at the
|
||||||
|
bottom, such as "Ready for review" or "Please take a look" (or "PTAL"). This
|
||||||
|
explicit handoff is useful when responding with multiple small commits.
|
||||||
|
|
||||||
|
After the PR has been reviewed it is the job of the reviewer to merge the PR.
|
||||||
* Please add new commits to your branch, instead of amending your commit. Adding new commits makes it easier for the reviewer to see what has changed since the last review.
|
Instructions for this are given below.
|
||||||
* Once you are ready for another round of reviews, add a comment at the bottom, such as "Ready for review" or "Please take a look" (or "PTAL"). This explicit handoff is useful when responding with multiple small commits.
|
|
||||||
|
|
||||||
After the PR has been reviewed it is the job of the reviewer to merge the PR. Instructions for this are given below.
|
|
||||||
|
|
||||||
|
|
||||||
## For maintainers: Reviewing a PR
|
## For maintainers: Reviewing a PR
|
||||||
|
|
||||||
The formal code reviews are done on GitHub. Reviewers are to look for all of the usual things:
|
The formal code reviews are done on GitHub. Reviewers are to look for all of the
|
||||||
|
usual things:
|
||||||
|
|
||||||
|
* Coding style follows the [Google C++ Style
|
||||||
|
Guide](https://google.github.io/styleguide/cppguide.html)
|
||||||
* Coding style follows the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html)
|
|
||||||
* Identify potential functional problems.
|
* Identify potential functional problems.
|
||||||
* Identify code duplication.
|
* Identify code duplication.
|
||||||
* Ensure the unit tests have enough coverage.
|
* Ensure the unit tests have enough coverage.
|
||||||
|
|
||||||
When looking for functional problems, there are some common problems reviewers should pay particular attention to:
|
When looking for functional problems, there are some common problems reviewers
|
||||||
|
should pay particular attention to:
|
||||||
|
|
||||||
|
|
||||||
* Does the code work for both Shader (Vulkan and OpenGL) and Kernel (OpenCL) scenarios? The respective SPIR-V dialects are slightly different.
|
|
||||||
* Changes are made to a container while iterating through it. You have to be careful that iterators are not invalidated or that elements are not skipped.
|
|
||||||
* C++11 and VS2013. We generally assume that we have a C++11 compliant compiler. However, on Windows, we still support Visual Studio 2013, which is not fully C++11 compliant. See [here](https://msdn.microsoft.com/en-us/library/hh567368.aspx). In particular, note that it does not provide default move-constructors or move-assignments for classes. In general, r-value references do not work the way you might assume they do.
|
|
||||||
* For SPIR-V transforms: The module is changed, but the analyses are not updated. For example, a new instruction is added, but the def-use manager is not updated. Later on, it is possible that the def-use manager will be used, and give wrong results.
|
|
||||||
|
|
||||||
|
* Does the code work for both Shader (Vulkan and OpenGL) and Kernel (OpenCL)
|
||||||
|
scenarios? The respective SPIR-V dialects are slightly different.
|
||||||
|
* Changes are made to a container while iterating through it. You have to be
|
||||||
|
careful that iterators are not invalidated or that elements are not skipped.
|
||||||
|
* C++11 and VS2013. We generally assume that we have a C++11 compliant
|
||||||
|
compiler. However, on Windows, we still support Visual Studio 2013, which is
|
||||||
|
not fully C++11 compliant. See
|
||||||
|
[here](https://msdn.microsoft.com/en-us/library/hh567368.aspx). In
|
||||||
|
particular, note that it does not provide default move-constructors or
|
||||||
|
move-assignments for classes. In general, r-value references do not work the
|
||||||
|
way you might assume they do.
|
||||||
|
* For SPIR-V transforms: The module is changed, but the analyses are not
|
||||||
|
updated. For example, a new instruction is added, but the def-use manager is
|
||||||
|
not updated. Later on, it is possible that the def-use manager will be used,
|
||||||
|
and give wrong results.
|
||||||
|
|
||||||
## For maintainers: Merging a PR
|
## For maintainers: Merging a PR
|
||||||
|
|
||||||
We intend to maintain a linear history on the GitHub master branch, and the build and its tests should pass at each commit in that history. A linear always-working history is easier to understand and to bisect in case we want to find which commit introduced a bug.
|
We intend to maintain a linear history on the GitHub master branch, and the
|
||||||
|
build and its tests should pass at each commit in that history. A linear
|
||||||
|
always-working history is easier to understand and to bisect in case we want to
|
||||||
|
find which commit introduced a bug.
|
||||||
|
|
||||||
These are the steps to follow to ensure this happens:
|
These are the steps to follow to ensure this happens:
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
* \<userid\> is the user id of the author of the PR.
|
* \<userid\> is the user id of the author of the PR.
|
||||||
* \<url\> is the url to that user's fork of SPIRV-Tools. Will often look like
|
* \<url\> is the url to that user's fork of SPIRV-Tools. Will often look like
|
||||||
|
|
||||||
```
|
```
|
||||||
git@github.com:<userid>/SPIRV-Tools.git
|
git@github.com:<userid>/SPIRV-Tools.git
|
||||||
```
|
```
|
||||||
|
|
||||||
|
|
||||||
* \<branch\> is the name of the branch that contains the PR
|
* \<branch\> is the name of the branch that contains the PR
|
||||||
* It is assumed that upstream points to [git@github.com](mailto:git@github.com):KhronosGroup/SPIRV-Tools.git. You can check this by doing
|
|
||||||
|
* It is assumed that upstream points to
|
||||||
|
[git@github.com](mailto:git@github.com):KhronosGroup/SPIRV-Tools.git. You
|
||||||
|
can check this by doing
|
||||||
|
|
||||||
```
|
```
|
||||||
git remote -v
|
git remote -v
|
||||||
|
|
@ -102,7 +136,7 @@ These are the steps to follow to ensure this happens:
|
||||||
```
|
```
|
||||||
|
|
||||||
* Get a local copy of the branch containing the PR.
|
* Get a local copy of the branch containing the PR.
|
||||||
|
|
||||||
```
|
```
|
||||||
git remote add <userid> <url>
|
git remote add <userid> <url>
|
||||||
git fetch <userid> <branch>
|
git fetch <userid> <branch>
|
||||||
|
|
@ -110,15 +144,20 @@ These are the steps to follow to ensure this happens:
|
||||||
git branch -D <branch> # if it already exists
|
git branch -D <branch> # if it already exists
|
||||||
git checkout -b <branch> # assuming it does not collide.
|
git checkout -b <branch> # assuming it does not collide.
|
||||||
```
|
```
|
||||||
|
|
||||||
* Rebase the branch on top of the master branch. If there are conflicts, send it back to the author and ask them to rebase. During the interactive rebase be sure to squash all of the commits down to a single commit.
|
* Rebase the branch on top of the master branch. If there are conflicts, send
|
||||||
|
it back to the author and ask them to rebase. During the interactive rebase
|
||||||
|
be sure to squash all of the commits down to a single commit.
|
||||||
|
|
||||||
```
|
```
|
||||||
git branch -u master
|
git branch -u master
|
||||||
git rebase -i
|
git rebase -i
|
||||||
```
|
```
|
||||||
|
|
||||||
* **Build and test the branch.**
|
* **Build and test the branch.**
|
||||||
* If all of the tests pass, push the commit
|
|
||||||
`git push upstream HEAD:master`
|
* If all of the tests pass, push the commit `git push upstream HEAD:master`
|
||||||
* Close the PR and add a comment saying it was push using the commit that you just pushed. See https://github.com/KhronosGroup/SPIRV-Tools/pull/935 as an example.
|
|
||||||
|
* Close the PR and add a comment saying it was push using the commit that you
|
||||||
|
just pushed. See https://github.com/KhronosGroup/SPIRV-Tools/pull/935 as an
|
||||||
|
example.
|
||||||
|
|
|
||||||
Загрузка…
Ссылка в новой задаче