From 578473324a153922cadfcc12e42a9b6f7e83c2cf Mon Sep 17 00:00:00 2001 From: Scott Todd Date: Thu, 9 Nov 2023 12:02:39 -0800 Subject: [PATCH] Merge CONTRIBUTING.md into website contributing guide. (#15449) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Following up on some ideas noted at https://github.com/openxla/iree/issues/15116#issuecomment-1791036129. We had previously split "contributing" docs between the root `CONTRIBUTING.md` and a separate developer doc. Now that the developer doc is published on our website at https://iree.dev/developers/general/contributing/, we can move more of the documentation there and just have the root file point at the website. Preview for review: https://scotttodd.github.io/iree/developers/general/contributing/ * I rewrote and reordered most of the sections * I have more ideas for how to streamline these docs... trying to not spent too much time on this right now though 😛 * Separate "infrastructure" page explaining how to interact with the CI (build/test/benchmark/etc.) * Deemphasize "contributor tips" and/or add more tips (like how to set up Visual Studio Code) I took some inspiration from https://llvm.org/docs/Contributing.html and https://llvm.org/docs/DeveloperPolicy.html. --- CONTRIBUTING.md | 85 +--- .../docs/developers/general/contributing.md | 374 ++++++++++++------ 2 files changed, 278 insertions(+), 181 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bcf8ddf703060..a79fbe70e16d3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,77 +1,20 @@ -# How to Contribute +# How to contribute -We'd love to accept your patches and contributions to this project. There are -just a few small guidelines you need to follow. +We'd love to accept your patches and contributions to this project. -## Contributor License Agreement +To get started with contributing, please take a look at the +[Contributing](https://iree.dev/developers/general/contributing/) guide. -Contributions to this project must be accompanied by a Contributor License -Agreement. You (or your employer) retain the copyright to your contribution; -this simply gives us permission to use and redistribute your contributions as -part of the project. Head over to to see -your current agreements on file or to sign a new one. +## Getting in touch -You generally only need to submit a CLA once, so if you've already submitted one -(even if it was for a different project), you probably don't need to do it -again. +* [GitHub issues](https://github.com/openxla/iree/issues): Feature requests, + bugs, and other work tracking +* [IREE Discord server](https://discord.gg/26P4xW4): Daily development + discussions with the core team and collaborators +* [iree-discuss email list](https://groups.google.com/forum/#!forum/iree-discuss): + Announcements, general and low-priority discussion -## Changes Accepted +## Community guidelines -Please file issues before doing substantial work; this will ensure that others -don't duplicate the work and that there's a chance to discuss any design issues. - -Changes only tweaking style are unlikely to be accepted unless they are applied -consistently across the project. Most of the code style is derived from the -[Google Style Guides](http://google.github.io/styleguide/) for the appropriate -language and is generally not something we accept changes on (as clang-format -and other linters set that for us). The compiler portion of the project follows -[MLIR style](https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide). -Improvements to code structure and clarity are welcome but please file issues to -track such work first. - -## AUTHORS file - -If you would like to receive additional recognition for your contribution, you -may add yourself (or your organization) to the AUTHORS file. This keeps track of -those who have made significant contributions to the project. Please add the -entity who owns the copyright for your contribution. The source control history -remains the most accurate source for individual contributions. - -## Code reviews - -All submissions, including submissions by maintainers, require review. We -use GitHub pull requests (PRs) for this purpose. Consult -[GitHub Help](https://help.github.com/articles/about-pull-requests/) for more -information on using pull requests. - -## Presubmits - -Most of our presubmit workflows will only run automatically on PRs if you are a -project collaborator. Otherwise a maintainer must -[approve workflow runs](https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks). -If you are sending code changes to the project, please ask to be added as a -collaborator, so that these can run automatically. It is generally expected that -PRs will only be merged when all checks are passing. In some cases, pre-existing -failures may be ignored by a maintainer or admin. - -## Merging - -After review and presubmit checks, PRs should be merged with a "squash and -merge". The squashed commit summary should match the PR title and the commit -description should match the PR body (this is the default behavior). -Accordingly, please write these as you would a helpful commit message. Please -also keep PRs small (focused on a single issue) to streamline review and ease -later culprit-finding. It is assumed that the PR author will merge their change -unless they ask someone else to merge it for them (e.g. because they don't have -write access). - -## Details - -For further details, see our -[detailed contributing guide](https://iree.dev/developers/contributing/), which is -separate to avoid notifying everyone every time it changes. - -## Community Guidelines - -This project follows -[Google's Open Source Community Guidelines](https://opensource.google.com/conduct/). +This project follows the +[OpenXLA Code of Conduct](https://github.com/openxla/community/blob/main/CODE-OF-CONDUCT.md). diff --git a/docs/website/docs/developers/general/contributing.md b/docs/website/docs/developers/general/contributing.md index 170bb81c17ef1..9781b966b56d4 100644 --- a/docs/website/docs/developers/general/contributing.md +++ b/docs/website/docs/developers/general/contributing.md @@ -2,21 +2,160 @@ icon: octicons/code-review-16 --- -# Contributing +# Contributing to IREE -This is a more detailed version of the top-level -[CONTRIBUTING.md](https://github.com/openxla/iree/blob/main/CONTRIBUTING.md) -file. We keep it separate to avoid everyone getting a pop-up when creating a PR -after each time it changes. +We'd love to accept your patches and contributions to this project. - +Please [file issues](https://github.com/openxla/iree/issues/new/choose) or +reach out on any of our other +[communication channels](../../index.md#communication-channels) before doing +substantial work; this will ensure that others don't duplicate the work and +that there's a chance to discuss any design issues. -## Build systems +## Developer policies + +### :octicons-code-of-conduct-16: Code of conduct + +This project follows the +[OpenXLA Code of Conduct](https://github.com/openxla/community/blob/main/CODE-OF-CONDUCT.md). + +### :octicons-law-16: Contributor License Agreement + +Contributions to this project must be accompanied by a Contributor License +Agreement (CLA). Head over to to see +your current agreements on file or to sign a new one. + +* You (or your employer) retain the copyright to your contribution; this simply + gives us permission to use and redistribute your contributions as part of the + project. +* You generally only need to submit a CLA once, so if you've already submitted + one (even if it was for a different project), you probably don't need to do it + again. + +### :octicons-pencil-16: Coding style guidelines + +Most of the code style is derived from the +[Google Style Guides](http://google.github.io/styleguide/) for the appropriate +language and is generally not something we accept changes on (as clang-format +and other linters set that for us). The C++ compiler portion of the project +follows the +[MLIR/LLVM style guide](https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide). + +Improvements to code structure and clarity are welcome but please file issues +to track such work first. Pure style changes are unlikely to be accepted unless +they are applied consistently across the project. + +??? tip - "Tip - code formatters and lint scripts" + + Formatters like + [`clang-format`](https://clang.llvm.org/docs/ClangFormat.html) (C/C++) and + [_Black_](https://black.readthedocs.io/en/stable/) (Python) can be set to + run automatically in your editor of choice. + + The script at + [`build_tools/scripts/lint.sh`](https://github.com/openxla/iree/blob/main/build_tools/scripts/lint.sh) + can also be used to run the full suite of lint checks. + +### :octicons-code-review-16: Code reviews + +All submissions, including submissions by maintainers, require review. We +use GitHub pull requests (PRs) for this purpose. Consult +[GitHub Help](https://help.github.com/articles/about-pull-requests/) for more +information on using pull requests. + +* Please keep PRs small (focused on a single issue) to make reviews and later + culprit-finding easier. +* You may see trusted core contributors bending this rule for project + maintenance and major subsystem renovation. If you feel like the rules aren't + working for a certain situation, please ask as we bias towards pragmatism for + cases that require it. + +### :material-check-all: GitHub Actions workflows + +We use [GitHub Actions](https://docs.github.com/en/actions) to automatically +build and test various parts of the project. + +* Most presubmit workflows will only run automatically on PRs if you are a + project collaborator. Otherwise a maintainer must + [approve workflow runs](https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks). + If you are sending code changes to the project, please ask to be added as a + collaborator, so that these can run automatically. +* It is generally expected that PRs will only be merged when all checks are + passing. In some cases, pre-existing failures may be bypassed by a maintainer. + +??? tip - "Tip - adjusting workflow behavior" + + Some workflows only run on commits after they are merged. See the + [CI behavior manipulation](#ci-behavior-manipulation) section below to + learn how to customize this behavior. + + + + +### :octicons-git-pull-request-16: Merging approved changes + +After review and presubmit checks, PRs should typically be merged using +"squash and merge". + +* The squashed commit summary should match the PR title and the commit + description should match the PR body (this is the default behavior). + Accordingly, please write these as you would a helpful commit message. + +It is assumed that the PR author will merge their change unless they ask +someone else to merge it for them (e.g. because they don't have write access +yet). + +### :octicons-git-merge-16: Obtaining commit access + +Access to affiliated repositories is divided into three tiers: + +| Tier | Description | Team link | +| ---- | ----------- | --------- | +Triage | :material-check: Can be [assigned issues](https://docs.github.com/en/issues/tracking-your-work-with-issues/assigning-issues-and-pull-requests-to-other-github-users)
:material-check: Can apply labels to issues / PRs
:material-check: Can run workflows [without approval](https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks) | [iree-triage](https://github.com/orgs/openxla/teams/iree-triage) +Write | **Most developers should request this access**
:material-check: Can [merge approved pull requests](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/merging-a-pull-request)
:material-check: Can create branches | [iree-write](https://github.com/orgs/openxla/teams/iree-write) +Maintain | :material-check: Can [edit repository settings](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features)
:material-check: Can push to [protected branches](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches) | [iree-maintain](https://github.com/orgs/openxla/teams/iree-maintain) + +All access tiers first require joining the +[OpenXLA GitHub organization](https://github.com/openxla/). + + +[Fill out this form to request access :fontawesome-solid-paper-plane:](https://docs.google.com/forms/d/e/1FAIpQLSfEwANtMvLJWq-ED4lub_xsMch0MgNY02VxgtXE61FqNvNVUg/viewform){ .md-button .md-button--primary } + +Once you are a member of the OpenXLA GitHub organization, you can request to +join any of the teams on . + +!!! note - "Note: other GitHub organizations" + + Work on IREE sometimes spans other GitHub organizations like + [iree-org](https://github.com/iree-org) and + [shark-infra](https://github.com/shark-infra/). Reach out to a project + member if you would also like access to repositories in those organizations. + +### :octicons-people-16: Credits in the AUTHORS file + +If you would like additional recognition for your contributions, you may add +yourself or your organization to the +[AUTHORS file](https://github.com/openxla/iree/blob/main/AUTHORS) that keeps +track of those who have made significant contributions to the project. + +* Please add the entity who owns the copyright for your contribution. +* The source control history remains the most accurate source for individual + contributions. + + + +## Tips for contributors + +### Tool recommendations + +| Program or tool | Description | +| -- | -- | +[:material-microsoft-visual-studio-code: Visual Studio Code (VSCode)]() | The most commonly used editor amongst IREE developers +[:simple-cmake: Ccache]() | A fast C/C++ compiler cache. See the [CMake with `ccache`](../building/cmake-with-ccache.md) page +[:simple-github: GitHub CLI]() | A CLI for interacting with GitHub +[:simple-github: "Refined GitHub" browser extensions]() | Extension that add features to the GitHub UI + +### :material-hammer-wrench: Build systems IREE supports building from source with both Bazel and CMake. @@ -28,13 +167,13 @@ IREE supports building from source with both Bazel and CMake. PyTorch, etc.) may be difficult to support with one build system or the other, so the project may configure these as optional -## Continuous integration (CI) +### :octicons-server-16: Continuous integration (CI) IREE uses [GitHub Actions](https://docs.github.com/en/actions) for CI. The primary CI is configured in the [ci.yml workflow file](https://github.com/openxla/iree/blob/main/.github/workflows/ci.yml). -### Self-hosted runners +#### Self-hosted runners In addition to the default runners GitHub provides, IREE uses [self-hosted runners](https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners) @@ -43,23 +182,27 @@ custom configurations such as accelerators. Configuration scripting is checked in to this repository (see the [README for that directory](https://github.com/openxla/iree/blob/main/build_tools/github_actions/runner/README.md)). -### Custom managed runners +#### Custom managed runners In addition to our self-hosted runners, we use GitHub's [large managed runners](https://docs.github.com/en/actions/using-github-hosted-runners/about-larger-runners) -for some platforms that are more trouble to configure ourselves (e.g. Mac). +for some platforms. -### CI behavior manipulation +#### CI behavior manipulation The setup step of the CI determines which CI jobs to run. This is controlled by the [configure_ci.py](https://github.com/openxla/iree/blob/main/build_tools/github_actions/configure_ci.py) script. It will generally run a pre-determined set of jobs on presubmit with some jobs kept as post-submit only. If changes are only to a certain set of -excluded files that we know don't affect CI (e.g. docs), then it will skip the -jobs. You can customize which jobs run using +excluded files that we know don't affect CI (e.g. Markdown files), then it will +skip the jobs. + +You can customize which jobs run using [git trailers](https://git-scm.com/docs/git-interpret-trailers) in the PR -description. The available options are +description. + +The available options are ``` text ci-skip: jobs,to,skip @@ -71,64 +214,98 @@ benchmark-extra: extra,benchmarks,to,run runner-env: [testing|prod] ``` -The first three follow the same format and instruct the setup script on which -jobs to include or exclude from its run. They take a comma-separated list of -jobs which must be from the set of top-level job identifiers in ci.yml file or -the special keyword "all" to indicate all jobs. `ci-skip` removes jobs that -would otherwise be included, though it is not an error to list jobs that would -not be included by default. `ci-extra` adds additional jobs that would not have -otherwise been run, though it is not an error to list jobs that would have been -included anyway. It *is* an error to list a job in both of these fields. -`ci-exactly` provides an exact list of jobs that should run. It is mutually -exclusive with both `ci-skip` and `ci-extra`. In all these cases, the setup does -not make any effort to ensure that job dependencies are satisfied. Thus, if you -request skipping the `build_all` job, all the jobs that depend on it will fail, -not be skipped. `skip-ci` is an older option that simply skips all jobs. Its -usage is deprecated and it is mutually exclusive with all of the other `ci-*` -options. Prefer `ci-skip: all`. - -Benchmarks don't run by default on PRs, and must be specifically requested. They -*do* run by default on PRs detected to be an integration of LLVM into IREE, but -this behavior can be disabled with `skip-llvm-integrate-benchmark`. The -`benchmark-extra` option allows specifying additional benchmark presets to run -as part of benchmarking. It accepts a comma-separated list of benchmark presets. -This combines with labels added to the PR (which are a more limited set of -options). See the -[benchmark suites documentation](../performance/benchmark-suites.md). - -The `runner-env` option controls which runner environment to target for our -self-hosted runners. We maintain a test environment to allow testing out new -configurations prior to rolling them out. This trailer is for advanced users who -are working on the CI infrastructure itself. - -#### CI configuration recipes +??? info - "Using `skip-ci`" + + `skip-ci` skips all jobs. It is mutually exclusive with the other `ci-*` + options and is synonomous with `ci-skip: all`. + + ``` text + skip-ci: free form reason + ``` + +??? info - "Using `ci-skip`, `ci-extra`, `ci-exactly`" + + The `ci-*` options instruct the setup script on which jobs to include or + exclude from its run. They take a comma-separated list of jobs which must be + from the set of top-level job identifiers in the `ci.yml` file or the + special keyword "all" to indicate all jobs. + + ``` text + ci-skip: jobs,to,skip + ci-extra: extra,jobs,to,run + ci-exactly: exact,set,of,jobs,to,run + ``` + + * `ci-skip` removes jobs that would otherwise be included, though it is not + an error to list jobs that would not be included by default. + * `ci-extra` adds additional jobs that would not have otherwise been run, + though it is not an error to list jobs that would have been included anyway. + It *is* an error to list a job in both "skip" and "extra". + * `ci-exactly` provides an exact list of jobs that should run. It is + mutually exclusive with both "skip" and "extra". + + In all these cases, the setup does not make any effort to ensure that job + dependencies are satisfied. Thus, if you request skipping the `build_all` + job, all the jobs that depend on it will fail, not be skipped. + +??? info - "Using `benchmark-extra`, `skip-llvm-integrate-benchmark`" + + ``` text + benchmark-extra: extra,benchmarks,to,run + skip-llvm-integrate-benchmark: free form reason + ``` + + Benchmarks don't run by default on PRs, and must be specifically requested. + + The `benchmark-extra` option allows specifying additional benchmark presets + to run as part of benchmarking. It accepts a comma-separated list of + benchmark presets. This combines with labels added to the PR (which are a + more limited set of options). See the + [benchmark suites documentation](../performance/benchmark-suites.md). + + Benchmarks *do* run by default on PRs detected to be an integration of LLVM + into IREE, but this behavior can be disabled with + `skip-llvm-integrate-benchmark`. + +??? info - "Using `runner-env`" + + The `runner-env` option controls which runner environment to target for our + self-hosted runners. We maintain a test environment to allow testing out new + configurations prior to rolling them out. This trailer is for advanced users + who are working on the CI infrastructure itself. + + ``` text + runner-env: [testing|prod] + ``` + +##### CI configuration recipes Copy/paste any of these at the bottom of a PR description to change what the CI runs. * Also run Windows and macOS builds that are normally post-merge only: - ``` text - ci-extra: build_test_all_windows,build_test_all_macos_arm64,build_test_all_macos_x86_64 - ``` + ``` text + ci-extra: build_test_all_windows,build_test_all_macos_arm64,build_test_all_macos_x86_64 + ``` * Also run GPU tests on NVIDIA A100 runners (opt-in due to low availability): - ``` text - ci-extra: test_a100 - ``` + ``` text + ci-extra: test_a100 + ``` * Skip all CI builds and tests, e.g. for comment-only changes: - ``` text - skip-ci: Comment-only change. - ``` + ``` text + skip-ci: Comment-only change. + ``` * Only run Bazel builds, e.g. for changes only affecting Bazel rules: - ``` text - ci-exactly: build_test_all_bazel - ``` + ``` text + ci-exactly: build_test_all_bazel + ``` For example, this PR opted in to running the `build_test_all_windows` job: @@ -138,35 +315,7 @@ The enabled jobs can be viewed from the Summary page of an action run: ![ci_enabled_jobs](./contributing-ci-enabled-jobs.png) -## Contributor tips - -These are opinionated tips documenting workflows that some members of the team -have found useful. They are focused on meta-tooling, not on IREE code -specifically (you will find the latter in the -[Developer Overview](./developer-overview.md)). - -!!! note - - It is certainly possible to use workflows other than these. Some common - tasks, especially for maintainers, will likely be made easier by using - these flows. - -We assume a basic knowledge -of `git` and GitHub and suggests some specific ways of using it. - -### Useful tools - -* GitHub CLI (). A CLI for interacting with GitHub. - Most importantly, it allows scripting the creation of pull requests. -* Refined GitHub Chrome and Firefox Extension: - . Nice extension that adds a - bunch of features to the GitHub UI. -* VSCode: . The most commonly used IDE amongst - IREE developers. -* [Ccache](https://ccache.dev/), a fast C/C++ compiler cache. See our - [CMake with `ccache`](../building/cmake-with-ccache.md) page - -### Git structure +### Git workflows We tend to use the "triangular" or "forking" workflow. Develop primarily on a clone of the repository on your development machine. Any local branches named @@ -190,30 +339,33 @@ different branch on your public fork and create the PR from there. a. If you already cloned from the main repository (e.g. by following the getting started guide): - ```shell - # From your existing git repo - $ git remote rename origin upstream - $ git remote add origin https://github.com//iree.git - ``` + ```shell + # From your existing git repo + $ git remote rename origin upstream + $ git remote add origin https://github.com//iree.git + ``` b. If you haven't already cloned: - ```shell - # From whatever directory under which you want to nest your repo - $ git clone https://github.com//iree.git - $ cd iree - $ git remote add upstream https://github.com/openxla/iree.git - ``` + ```shell + # From whatever directory under which you want to nest your repo + $ git clone https://github.com//iree.git + $ cd iree + $ git remote add upstream https://github.com/openxla/iree.git + ``` This is especially important for maintainers who have write access (so can push directly to the main repository) and admins who have elevated - privileges (so can push directly to protected branches). These names are - just suggestions, but you might find some scripts where the defaults are for - remotes named like this. For extra safety, you can make it difficult to push - directly to upstream by setting the push url to something invalid: `git - remote set-url --push upstream DISABLE`, which requires re-enabling the push - URL explicitly before pushing. You can wrap this behavior in a custom git - command like + privileges (so can push directly to protected branches). + + These names are just suggestions, but you might find some scripts where the + defaults are for remotes named like this. + + For extra safety, you can make it difficult to push directly to upstream by + setting the push url to something invalid: + `git remote set-url --push upstream DISABLE`, which requires re-enabling the + push URL explicitly before pushing. You can wrap this behavior in a custom + git command like [git-sudo](https://gist.github.com/GMNGeoffrey/42dd9a9792390094a43bdb69659320c0). 3. Use a script like @@ -228,6 +380,8 @@ These are some additional options you could put in your top-level `.gitconfig` or repository-specific `.git/config` files that are conducive the recommended workflow + + ```ini [push] default = current