1# Contributing 2 3## How to report bugs 4 5We use Google issue tracker. Please use 6[the public crosvm component](https://issuetracker.google.com/issues?q=status:open%20componentid:1161302). 7 8**For Googlers**: See [go/crosvm#filing-bugs](https://goto.google.com/crosvm#filing-bugs). 9 10## Contributing code 11 12### Gerrit Account 13 14You need to set up a user account with [gerrit](https://chromium-review.googlesource.com/). Once 15logged in, you can obtain 16[HTTP Credentials](https://chromium-review.googlesource.com/settings/#HTTPCredentials) to set up git 17to upload changes. 18 19Once set up, run `./tools/cl` to install the gerrit commit message hook. This will insert a unique 20"Change-Id" into all commit messages so gerrit can identify changes. 21 22### Contributor License Agreement 23 24Contributions to this project must be accompanied by a Contributor License Agreement (CLA). You (or 25your employer) retain the copyright to your contribution; this simply gives us permission to use and 26redistribute your contributions as part of the project. Head over to 27<https://cla.developers.google.com/> to see your current agreements on file or to sign a new one. 28 29You generally only need to submit a CLA once, so if you've already submitted one (even if it was for 30a different project), you probably don't need to do it again. 31 32### Commit Messages 33 34As for commit messages, we follow 35[ChromeOS's guideline](https://chromium.googlesource.com/chromiumos/docs/+/HEAD/contributing.md#commit-messages) 36in general. 37 38Here is an example of a good commit message: 39 40``` 41devices: vhost: user: vmm: Add Connection type 42 43This abstracts away the cross-platform differences: cfg(unix) uses a 44Unix domain stream socket to connect to the vhost-user backend, and 45cfg(windows) uses a Tube. 46 47BUG=b:249361790 48TEST=tools/presubmit --all 49 50Change-Id: I47651060c2ce3a7e9f850b7ed9af8bd035f82de6 51``` 52 53- The first line is a subject that starts with a tag that represents which components your commit 54 relates to. Tags are usually the name of the crate you modified such as `devices:` or `base:`. If 55 you only modified a specific component in a crate, you can specify the path to the component as a 56 tag like `devices: vhost: user:`. If your commit modified multiple crates, specify the crate where 57 your main change exists. The subject should be no more than 50 characters, including any tags. 58- The body should consist of a motivation followed by an impact/action. The body text should be 59 wrapped to 72 characters. 60- `BUG` lines are used to specify an associated issue number. If the issue is filed at 61 [Google's issue tracker](https://issuetracker.google.com/), write `BUG=b:<bug number>`. If no 62 issue is associated, write `BUG=None`. You can have multiple `BUG` lines. 63- `TEST` lines are used to describe how you tested your commit in a free form. You can have multiple 64 `TEST` lines. 65- `Change-Id` is used to identify your change on Gerrit. It's inserted by the gerrit commit message 66 hook as explained in 67 [the previous section](https://crosvm.dev/book/contributing/index.html#gerrit-account). If a new 68 commit is uploaded with the same `Change-Id` as an existing CL's `Change-Id`, gerrit will 69 recognize the new commit as a new patchset of the existing CL. 70 71### Uploading changes 72 73To make changes to crosvm, start your work on a new branch tracking `origin/main`. 74 75```bash 76git checkout --branch myfeature --track origin/main 77``` 78 79After making the necessary changes, and testing them via 80[Presubmit Checks](https://crosvm.dev/book/building_crosvm.html#presubmit-checks), you can commit 81and upload them: 82 83```bash 84git commit 85./tools/cl upload 86``` 87 88If you need to revise your change, you can amend the existing commit and upload again: 89 90```bash 91git commit --amend 92./tools/cl upload 93``` 94 95This will create a new version of the same change in gerrit. 96 97> Note: We don't accept any pull requests on the [GitHub mirror]. 98 99### Getting Reviews 100 101All submissions needs to be reviewed by one of the [crosvm owners]. Use the gerrit UI to request a 102review. If you are uncertain about the correct person to review, reach out to the team via 103[chat](https://matrix.to/#/#crosvm:matrix.org) or 104[email list](https://groups.google.com/a/chromium.org/g/crosvm-dev). 105 106### Submitting code 107 108Crosvm uses a Commit Queue, which will run pre-submit testing on all changes before merging them 109into crosvm. 110 111Once one of the [crosvm owners] has voted "Code-Review+2" on your change, you can use the "Submit to 112CQ" button, which will trigger the test process. 113 114Gerrit will show any test failures. Refer to 115[Building Crosvm](https://crosvm.dev/book/building_crosvm.html) for information on how to run the 116same tests locally. 117 118When all tests pass, your change is merged into `origin/main`. 119 120## Philosophy 121 122The following is high level guidance for producing contributions to crosvm. 123 124- Prefer mechanism to policy. 125- Use existing protocols when they are adequate, such as virtio. 126- Prefer security over code re-use and speed of development. 127- Only the version of Rust in use by the ChromeOS toolchain is supported. This is ordinarily the 128 stable version of Rust, but can be behind a version for a few weeks. 129- Avoid distribution specific code. 130 131## Style guidelines 132 133### Formatting 134 135To format all code, crosvm defers to `rustfmt`. In addition, the code adheres to the following 136rules: 137 138Each `use` statement should import a single item, as produced by `rustfmt` with 139[`imports_granularity=item`]. Do not use braces to import multiple items. 140 141The `use` statements for each module should be grouped into blocks separated by whitespace in the 142order produced by `rustfmt` with [`group_imports=StdExternalCrate`] and sorted alphabetically: 143 1441. `std` 1451. third-party + crosvm crates 1461. `crate` + `super` 147 148The import formatting options of `rustfmt` are currently unstable, so these are not enforced 149automatically. If a nightly Rust toolchain is present, it is possible to automatically reformat the 150code to match these guidelines by running `tools/fmt --nightly`. 151 152crosvm uses the [remain](https://github.com/dtolnay/remain) crate to keep error enums sorted, along 153with the `#[sorted]` attribute to keep their corresponding match statements in the same order. 154 155### Unit test code 156 157Unit tests and other highly-specific tests (which may include some small, but not all, integration 158tests) should be written differently than how non-test code is written. Tests prevent regressions 159from being committed, show how APIs can be used, and help with understanding bugs in code. That 160means tests must be clear both now and in the future to a developer with low familiarity of the code 161under test. They should be understandable by reading from top to bottom without referencing any 162other code. Towards these goals, tests should: 163 164- To a reasonable extent, be structured as Arrange-Act-Assert. 165- Test the minimum number of behaviors in a single test. Make separate tests for separate behavior. 166- Avoid helper methods that send critical inputs or assert outputs within the helper itself. It 167 should be easy to read a test and determine the critical inputs/outputs without digging through 168 helper methods. Setup common to many tests is fine to factor out, but lean toward duplicating code 169 if it aids readability. 170- Avoid branching statements like conditionals and loops (which can make debugging more difficult). 171- Document the reason constants were chosen in the test, including if they were picked arbitrarily 172 such that in the future, changing the value is okay. (This can be done with constant variable 173 names, which is ideal if the value is used more than once, or in a comment.) 174- Name tests to describe what is being tested and the expected outcome, for example 175 `test_foo_invalid_bar_returns_baz`. 176 177Less-specific tests, such as most integration tests and system tests, are more likely to require 178obfuscating work behind helper methods. It is still good to strive for clarity and ease of debugging 179in those tests, but they do not need to follow these guidelines. 180 181## Contributing to the documentation 182 183[The book of crosvm] is built with [mdBook]. Each markdown file must follow 184[Google Markdown style guide]. 185 186To render the book locally, you need to install mdbook and [mdbook-mermaid], which should be 187installed when you run `./tools/install-deps` script. Or you can use the `tools/dev_container` 188environment. 189 190```sh 191cd docs/book/ 192mdbook build 193``` 194 195Output is found at `docs/book/book/html/`. 196 197[crosvm owners]: https://chromium.googlesource.com/crosvm/crosvm/+/HEAD/OWNERS 198[github mirror]: https://github.com/google/crosvm 199[google markdown style guide]: https://github.com/google/styleguide/blob/gh-pages/docguide/style.md 200[mdbook]: https://rust-lang.github.io/mdBook/ 201[mdbook-mermaid]: https://github.com/badboy/mdbook-mermaid 202[the book of crosvm]: https://crosvm.dev/book/ 203[`group_imports=stdexternalcrate`]: https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#group_imports 204[`imports_granularity=item`]: https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#imports_granularity 205