Skip to content

Latest commit

 

History

History
149 lines (118 loc) · 7.13 KB

CONTRIBUTING.md

File metadata and controls

149 lines (118 loc) · 7.13 KB
Raw

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 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 and projects. If the bug has not already been reported open a new one here.

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 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 (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 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. 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 master. Also, this helps us update the 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
  • 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 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. The Squash and Merge 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.
  2. 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 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 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.