mirror of
https://github.com/KhronosGroup/SPIRV-Tools
synced 2024-11-25 13:00:04 +00:00
727f4346db
The default branch was renamed from `master` to `main`. Update some stale references. Signed-off-by: Sven van Haastregt <sven.vanhaastregt@arm.com>
150 lines
7.1 KiB
Markdown
150 lines
7.1 KiB
Markdown
# Contributing to SPIR-V Tools
|
|
|
|
## 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/main/docs/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
|
|
[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:
|
|
|
|
* 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.
|
|
* 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.
|
|
* Examples that demonstrate the problem.
|
|
|
|
## 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.
|
|
|
|
See
|
|
[README.md](https://github.com/KhronosGroup/SPIRV-Tools/blob/main/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 `clang-format -style=file -i [modified-files]` can help.
|
|
* 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.
|
|
* If your patch completely fixes bug 1234, the commit message should say
|
|
`Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1234` When you do
|
|
this, the issue will be closed automatically when the commit goes into
|
|
main. Also, this helps us update the [CHANGES](CHANGES) file.
|
|
* Watch the continuous builds to make sure they pass.
|
|
* Request a code review.
|
|
|
|
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.
|
|
Instructions for this are given below.
|
|
|
|
## For maintainers: Reviewing a PR
|
|
|
|
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)
|
|
* Identify potential functional problems.
|
|
* Identify code duplication.
|
|
* Ensure the unit tests have enough coverage.
|
|
* Ensure continuous integration (CI) bots run on the PR. If not run (in the
|
|
case of PRs by external contributors), add the "kokoro:run" label to the
|
|
pull request which will trigger running all CI jobs.
|
|
|
|
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.
|
|
* 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.
|
|
* If a pass gets the id of a type from the type manager, make sure the type is
|
|
not a struct or array. It there are two structs that look the same, the type
|
|
manager can return the wrong one.
|
|
|
|
## For maintainers: Merging a PR
|
|
|
|
We intend to maintain a linear history on the GitHub main 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. The
|
|
[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)
|
|
button on the GitHub web interface. All other ways of merging on the web
|
|
interface have been disabled.
|
|
|
|
Before merging, we generally require:
|
|
|
|
1. All tests except for the smoke test pass. See
|
|
[failing smoke test](#failing-smoke-test).
|
|
1. The PR is approved by at least one of the maintainers. If the PR modifies
|
|
different parts of the code, then multiple reviewers might be necessary.
|
|
|
|
The squash-and-merge button will turn green when these requirements are met.
|
|
Maintainers have the to power to merge even if the button is not green, but that
|
|
is discouraged.
|
|
|
|
### Failing smoke test
|
|
|
|
The purpose of the smoke test is to let us know if
|
|
[shaderc](https://github.com/google/shaderc) fails to build with the change. If
|
|
it fails, the maintainer needs to determine if the reason for the failure is a
|
|
problem in the current PR or if another repository needs to be changed. Most of
|
|
the time [Glslang](https://github.com/KhronosGroup/glslang) needs to be updated
|
|
to account for the change in SPIR-V Tools.
|
|
|
|
The PR can still be merged if the problem is not with that PR.
|
|
|
|
## For maintainers: Running tests
|
|
|
|
For security reasons, not all tests will run automatically. When they do not, a
|
|
maintainer will have to start the tests.
|
|
|
|
If the Github actions tests do not run on a PR, they can be initiated by closing
|
|
and reopening the PR.
|
|
|
|
If the kokoro tests are not run, they can be run by adding the label
|
|
`kokoro:run` to the PR.
|