|
| 1 | +# Contributing to Foundry Compilers |
| 2 | + |
| 3 | +:balloon: Thanks for your help improving the project! We are so happy to have |
| 4 | +you! |
| 5 | + |
| 6 | +There are opportunities to contribute to Foundry Compilers at any level. It doesn't |
| 7 | +matter if you are just getting started with Rust or are the most weathered |
| 8 | +expert, we can use your help. |
| 9 | + |
| 10 | +**No contribution is too small and all contributions are valued.** |
| 11 | + |
| 12 | +This guide will help you get started. **Do not let this guide intimidate you**. |
| 13 | +It should be considered a map to help you navigate the process. |
| 14 | + |
| 15 | +The [dev channel][dev] is available for any concerns not covered in this guide, |
| 16 | +please join us! |
| 17 | + |
| 18 | +[dev]: https://t.me/foundry_rs |
| 19 | + |
| 20 | +## Conduct |
| 21 | + |
| 22 | +The Foundry Compilers project adheres to the [Rust Code of Conduct][coc]. This describes |
| 23 | +the _minimum_ behavior expected from all contributors. Instances of violations |
| 24 | +of the Code of Conduct can be reported by contacting the project team at |
| 25 | +[enrique@ortiz ](mailto:[email protected]). |
| 26 | + |
| 27 | +[coc]: https://www.rust-lang.org/policies/code-of-conduct |
| 28 | + |
| 29 | +## Contributing in Issues |
| 30 | + |
| 31 | +For any issue, there are fundamentally three ways an individual can contribute: |
| 32 | + |
| 33 | +1. By opening the issue for discussion: For instance, if you believe that you |
| 34 | + have uncovered a bug in Foundry Compilers, creating a new issue in the Foundry Compilers |
| 35 | + issue tracker is the way to report it. |
| 36 | + |
| 37 | +2. By helping to triage the issue: This can be done by providing |
| 38 | + supporting details (a test case that demonstrates a bug), providing |
| 39 | + suggestions on how to address the issue, or ensuring that the issue is tagged |
| 40 | + correctly. |
| 41 | + |
| 42 | +3. By helping to resolve the issue: Typically this is done either in the form of |
| 43 | + demonstrating that the issue reported is not a problem after all, or more |
| 44 | + often, by opening a Pull Request that changes some bit of something in |
| 45 | + Foundry Compilers in a concrete and reviewable manner. |
| 46 | + |
| 47 | +**Anybody can participate in any stage of contribution**. We urge you to |
| 48 | +participate in the discussion around bugs and participate in reviewing PRs. |
| 49 | + |
| 50 | +### Asking for General Help |
| 51 | + |
| 52 | +If you have reviewed existing documentation and still have questions or are |
| 53 | +having problems, you can open an issue asking for help. |
| 54 | + |
| 55 | +In exchange for receiving help, we ask that you contribute back a documentation |
| 56 | +PR that helps others avoid the problems that you encountered. |
| 57 | + |
| 58 | +### Submitting a Bug Report |
| 59 | + |
| 60 | +When opening a new issue in the Foundry Compilers issue tracker, users will be presented |
| 61 | +with a [basic template][template] that should be filled in. If you believe that |
| 62 | +you have uncovered a bug, please fill out this form, following the template to |
| 63 | +the best of your ability. Do not worry if you cannot answer every detail, just |
| 64 | +fill in what you can. |
| 65 | + |
| 66 | +The two most important pieces of information we need in order to properly |
| 67 | +evaluate the report is a description of the behavior you are seeing and a simple |
| 68 | +test case we can use to recreate the problem on our own. If we cannot recreate |
| 69 | +the issue, it becomes impossible for us to fix. |
| 70 | + |
| 71 | +In order to rule out the possibility of bugs introduced by userland code, test |
| 72 | +cases should be limited, as much as possible, to using only Foundry Compilers APIs. |
| 73 | + |
| 74 | +See [How to create a Minimal, Complete, and Verifiable example][mcve]. |
| 75 | + |
| 76 | +[mcve]: https://stackoverflow.com/help/mcve |
| 77 | +[template]: .github/PULL_REQUEST_TEMPLATE.md |
| 78 | + |
| 79 | +### Triaging a Bug Report |
| 80 | + |
| 81 | +Once an issue has been opened, it is not uncommon for there to be discussion |
| 82 | +around it. Some contributors may have differing opinions about the issue, |
| 83 | +including whether the behavior being seen is a bug or a feature. This discussion |
| 84 | +is part of the process and should be kept focused, helpful, and professional. |
| 85 | + |
| 86 | +Short, clipped responses—that provide neither additional context nor supporting |
| 87 | +detail—are not helpful or professional. To many, such responses are simply |
| 88 | +annoying and unfriendly. |
| 89 | + |
| 90 | +Contributors are encouraged to help one another make forward progress as much as |
| 91 | +possible, empowering one another to solve issues collaboratively. If you choose |
| 92 | +to comment on an issue that you feel either is not a problem that needs to be |
| 93 | +fixed, or if you encounter information in an issue that you feel is incorrect, |
| 94 | +explain why you feel that way with additional supporting context, and be willing |
| 95 | +to be convinced that you may be wrong. By doing so, we can often reach the |
| 96 | +correct outcome much faster. |
| 97 | + |
| 98 | +### Resolving a Bug Report |
| 99 | + |
| 100 | +In the majority of cases, issues are resolved by opening a Pull Request. The |
| 101 | +process for opening and reviewing a Pull Request is similar to that of opening |
| 102 | +and triaging issues, but carries with it a necessary review and approval |
| 103 | +workflow that ensures that the proposed changes meet the minimal quality and |
| 104 | +functional guidelines of the Foundry Compilers project. |
| 105 | + |
| 106 | +## Pull Requests |
| 107 | + |
| 108 | +Pull Requests are the way concrete changes are made to the code, documentation, |
| 109 | +and dependencies in the Foundry Compilers repository. |
| 110 | + |
| 111 | +Even tiny pull requests (e.g., one character pull request fixing a typo in API |
| 112 | +documentation) are greatly appreciated. Before making a large change, it is |
| 113 | +usually a good idea to first open an issue describing the change to solicit |
| 114 | +feedback and guidance. This will increase the likelihood of the PR getting |
| 115 | +merged. |
| 116 | + |
| 117 | +When opening a PR **please select the "Allow Edits From Maintainers" option**. |
| 118 | +Foundry Compilers maintains strict standards for code quality and style, as well as |
| 119 | +commit signing. This option allows us to make small changes to your PR to bring |
| 120 | +it in line with these standards. It helps us get your PR in faster, and with |
| 121 | +less work from you. |
| 122 | + |
| 123 | +### Cargo Commands |
| 124 | + |
| 125 | +This section lists some commonly needed commands. |
| 126 | + |
| 127 | +```sh |
| 128 | +cargo check --all-features |
| 129 | +cargo +nightly fmt --all |
| 130 | +cargo build --all-features |
| 131 | +cargo test --all-features |
| 132 | +cargo +nightly clippy --all-features |
| 133 | +``` |
| 134 | + |
| 135 | +### Tests |
| 136 | + |
| 137 | +If the change being proposed alters code (as opposed to only documentation for |
| 138 | +example), it is either adding new functionality to Foundry Compilers or it is fixing |
| 139 | +existing, broken functionality. In both of these cases, the pull request should |
| 140 | +include one or more tests to ensure that Foundry Compilers does not regress in the |
| 141 | +future. |
| 142 | + |
| 143 | +#### Unit Tests |
| 144 | + |
| 145 | +Functions which have very specific tasks should be unit tested. We encourage |
| 146 | +using table tests to cover a large number of cases in a succinct readable |
| 147 | +manner. |
| 148 | + |
| 149 | +#### Integration tests |
| 150 | + |
| 151 | +Integration tests go in the same crate as the code they are testing, in the |
| 152 | +`tests/` directory. |
| 153 | + |
| 154 | +The best strategy for writing a new integration test is to look at existing |
| 155 | +integration tests in the crate and follow the style. |
| 156 | + |
| 157 | +#### Documentation tests |
| 158 | + |
| 159 | +Ideally, every API has at least one [documentation test] that demonstrates how |
| 160 | +to use the API. Documentation tests are run with `cargo test --doc`. This |
| 161 | +ensures that the example is correct and provides additional test coverage. |
| 162 | + |
| 163 | +The trick to documentation tests is striking a balance between being succinct |
| 164 | +for a reader to understand and actually testing the API. |
| 165 | + |
| 166 | +Lines that start with `/// #` are removed when the documentation is generated. |
| 167 | +They are only there to get the test to run. Use this trick to keep your example |
| 168 | +succinct in the user-facing tests :) |
| 169 | + |
| 170 | +### Commits |
| 171 | + |
| 172 | +It is a recommended best practice to keep your changes as logically grouped as |
| 173 | +possible within individual commits. There is no limit to the number of commits |
| 174 | +any single Pull Request may have, and many contributors find it easier to review |
| 175 | +changes that are split across multiple commits. |
| 176 | + |
| 177 | +That said, if you have a number of commits that are "checkpoints" and don't |
| 178 | +represent a single logical change, please squash those together. |
| 179 | + |
| 180 | +Note that multiple commits often get squashed when they are landed (see the |
| 181 | +notes about [commit squashing](#commit-squashing)). |
| 182 | + |
| 183 | +#### Commit message guidelines |
| 184 | + |
| 185 | +Commit messages should follow the |
| 186 | +[Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) |
| 187 | +specification. |
| 188 | + |
| 189 | +Here's a few examples from the master branch's commit log: |
| 190 | + |
| 191 | +- feat(abigen): support empty events |
| 192 | +- chore: bump crypto deps |
| 193 | +- test: simplify test cleanup |
| 194 | +- fmt: run rustfmt |
| 195 | + |
| 196 | +### Opening the Pull Request |
| 197 | + |
| 198 | +From within GitHub, opening a new Pull Request will present you with a |
| 199 | +[template] that should be filled out. Please try to do your best at filling out |
| 200 | +the details, but feel free to skip parts if you're not sure what to put. |
| 201 | + |
| 202 | +[template]: .github/PULL_REQUEST_TEMPLATE.md |
| 203 | + |
| 204 | +### Discuss and update |
| 205 | + |
| 206 | +You will probably get feedback or requests for changes to your Pull Request. |
| 207 | +This is a big part of the submission process so don't be discouraged! Some |
| 208 | +contributors may sign off on the Pull Request right away, others may have |
| 209 | +more detailed comments or feedback. This is a necessary part of the process |
| 210 | +in order to evaluate whether the changes are correct and necessary. |
| 211 | + |
| 212 | +**Any community member can review a PR and you might get conflicting feedback**. |
| 213 | +Keep an eye out for comments from code owners to provide guidance on conflicting |
| 214 | +feedback. |
| 215 | + |
| 216 | +**Once the PR is open, do not rebase the commits**. See |
| 217 | +[Commit Squashing](#commit-squashing) for more details. |
| 218 | + |
| 219 | +### Commit Squashing |
| 220 | + |
| 221 | +In most cases, **do not squash commits that you add to your Pull Request during |
| 222 | +the review process**. When the commits in your Pull Request land, they may be |
| 223 | +squashed into one commit per logical change. Metadata will be added to the |
| 224 | +commit message (including links to the Pull Request, links to relevant issues, |
| 225 | +and the names of the reviewers). The commit history of your Pull Request, |
| 226 | +however, will stay intact on the Pull Request page. |
| 227 | + |
| 228 | +## Reviewing Pull Requests |
| 229 | + |
| 230 | +**Any Foundry Compilers community member is welcome to review any pull request**. |
| 231 | + |
| 232 | +All Foundry Compilers contributors who choose to review and provide feedback on Pull |
| 233 | +Requests have a responsibility to both the project and the individual making the |
| 234 | +contribution. Reviews and feedback must be helpful, insightful, and geared |
| 235 | +towards improving the contribution as opposed to simply blocking it. If there |
| 236 | +are reasons why you feel the PR should not land, explain what those are. Do not |
| 237 | +expect to be able to block a Pull Request from advancing simply because you say |
| 238 | +"No" without giving an explanation. Be open to having your mind changed. Be open |
| 239 | +to working with the contributor to make the Pull Request better. |
| 240 | + |
| 241 | +Reviews that are dismissive or disrespectful of the contributor or any other |
| 242 | +reviewers are strictly counter to the Code of Conduct. |
| 243 | + |
| 244 | +When reviewing a Pull Request, the primary goals are for the codebase to improve |
| 245 | +and for the person submitting the request to succeed. **Even if a Pull Request |
| 246 | +does not land, the submitters should come away from the experience feeling like |
| 247 | +their effort was not wasted or unappreciated**. Every Pull Request from a new |
| 248 | +contributor is an opportunity to grow the community. |
| 249 | + |
| 250 | +### Review a bit at a time. |
| 251 | + |
| 252 | +Do not overwhelm new contributors. |
| 253 | + |
| 254 | +It is tempting to micro-optimize and make everything about relative performance, |
| 255 | +perfect grammar, or exact style matches. Do not succumb to that temptation. |
| 256 | + |
| 257 | +Focus first on the most significant aspects of the change: |
| 258 | + |
| 259 | +1. Does this change make sense for Foundry Compilers? |
| 260 | +2. Does this change make Foundry Compilers better, even if only incrementally? |
| 261 | +3. Are there clear bugs or larger scale issues that need attending to? |
| 262 | +4. Is the commit message readable and correct? If it contains a breaking change |
| 263 | + is it clear enough? |
| 264 | + |
| 265 | +Note that only **incremental** improvement is needed to land a PR. This means |
| 266 | +that the PR does not need to be perfect, only better than the status quo. Follow |
| 267 | +up PRs may be opened to continue iterating. |
| 268 | + |
| 269 | +When changes are necessary, _request_ them, do not _demand_ them, and **do not |
| 270 | +assume that the submitter already knows how to add a test or run a benchmark**. |
| 271 | + |
| 272 | +Specific performance optimization techniques, coding styles and conventions |
| 273 | +change over time. The first impression you give to a new contributor never does. |
| 274 | + |
| 275 | +Nits (requests for small changes that are not essential) are fine, but try to |
| 276 | +avoid stalling the Pull Request. Most nits can typically be fixed by the |
| 277 | +Foundry Compilers Collaborator landing the Pull Request but they can also be an |
| 278 | +opportunity for the contributor to learn a bit more about the project. |
| 279 | + |
| 280 | +It is always good to clearly indicate nits when you comment: e.g. |
| 281 | +`Nit: change foo() to bar(). But this is not blocking.` |
| 282 | + |
| 283 | +If your comments were addressed but were not folded automatically after new |
| 284 | +commits or if they proved to be mistaken, please, [hide them][hiding-a-comment] |
| 285 | +with the appropriate reason to keep the conversation flow concise and relevant. |
| 286 | + |
| 287 | +### Be aware of the person behind the code |
| 288 | + |
| 289 | +Be aware that _how_ you communicate requests and reviews in your feedback can |
| 290 | +have a significant impact on the success of the Pull Request. Yes, we may land |
| 291 | +a particular change that makes Foundry Compilers better, but the individual might just |
| 292 | +not want to have anything to do with Foundry Compilers ever again. The goal is not just |
| 293 | +having good code. |
| 294 | + |
| 295 | +### Abandoned or Stalled Pull Requests |
| 296 | + |
| 297 | +If a Pull Request appears to be abandoned or stalled, it is polite to first |
| 298 | +check with the contributor to see if they intend to continue the work before |
| 299 | +checking if they would mind if you took it over (especially if it just has nits |
| 300 | +left). When doing so, it is courteous to give the original contributor credit |
| 301 | +for the work they started (either by preserving their name and email address in |
| 302 | +the commit log, or by using an `Author: ` meta-data tag in the commit. |
| 303 | + |
| 304 | +_Adapted from the [Tokio contributing guide]_. |
| 305 | + |
| 306 | +[hiding-a-comment]: https://help.github.com/articles/managing-disruptive-comments/#hiding-a-comment |
| 307 | +[documentation test]: https://doc.rust-lang.org/rustdoc/documentation-tests.html |
| 308 | +[Tokio contributing guide]: https://github.com/tokio-rs/tokio/blob/master/CONTRIBUTING.md |
0 commit comments