1*60517a1eSAndroid Build Coastguard Worker# How to contribute 2*60517a1eSAndroid Build Coastguard Worker 3*60517a1eSAndroid Build Coastguard WorkerWe'd love to accept your patches and contributions to this project. There are 4*60517a1eSAndroid Build Coastguard Workerjust a few small guidelines you need to follow. 5*60517a1eSAndroid Build Coastguard Worker 6*60517a1eSAndroid Build Coastguard Worker## Getting started 7*60517a1eSAndroid Build Coastguard Worker 8*60517a1eSAndroid Build Coastguard WorkerBefore we can work on the code, we need to get a copy of it and setup some 9*60517a1eSAndroid Build Coastguard Workerlocal environment and tools. 10*60517a1eSAndroid Build Coastguard Worker 11*60517a1eSAndroid Build Coastguard WorkerFirst, fork the code to your user and clone your fork. This gives you a private 12*60517a1eSAndroid Build Coastguard Workerplayground where you can do any edits you'd like. For this guide, we'll use 13*60517a1eSAndroid Build Coastguard Workerthe [GitHub `gh` tool](https://github.com/cli/cli) 14*60517a1eSAndroid Build Coastguard Worker([Linux install](https://github.com/cli/cli/blob/trunk/docs/install_linux.md)). 15*60517a1eSAndroid Build Coastguard Worker(More advanced users may prefer the GitHub UI and raw `git` commands). 16*60517a1eSAndroid Build Coastguard Worker 17*60517a1eSAndroid Build Coastguard Worker```shell 18*60517a1eSAndroid Build Coastguard Workergh repo fork bazelbuild/rules_python --clone --remote 19*60517a1eSAndroid Build Coastguard Worker``` 20*60517a1eSAndroid Build Coastguard Worker 21*60517a1eSAndroid Build Coastguard WorkerNext, make sure you have a new enough version of Python installed that supports the 22*60517a1eSAndroid Build Coastguard Workervarious code formatters and other devtools. For a quick start, 23*60517a1eSAndroid Build Coastguard Worker[install pyenv](https://github.com/pyenv/pyenv-installer) and 24*60517a1eSAndroid Build Coastguard Workerat least Python 3.9.15: 25*60517a1eSAndroid Build Coastguard Worker 26*60517a1eSAndroid Build Coastguard Worker```shell 27*60517a1eSAndroid Build Coastguard Workercurl https://pyenv.run | bash 28*60517a1eSAndroid Build Coastguard Workerpyenv install 3.9.15 29*60517a1eSAndroid Build Coastguard Workerpyenv shell 3.9.15 30*60517a1eSAndroid Build Coastguard Worker``` 31*60517a1eSAndroid Build Coastguard Worker 32*60517a1eSAndroid Build Coastguard Worker## Development workflow 33*60517a1eSAndroid Build Coastguard Worker 34*60517a1eSAndroid Build Coastguard WorkerIt's suggested that you create what is called a "feature/topic branch" in your 35*60517a1eSAndroid Build Coastguard Workerfork when you begin working on code you want to eventually send or code review. 36*60517a1eSAndroid Build Coastguard Worker 37*60517a1eSAndroid Build Coastguard Worker``` 38*60517a1eSAndroid Build Coastguard Workergit checkout main # Start our branch from the latest code 39*60517a1eSAndroid Build Coastguard Workergit checkout -b my-feature # Create and switch to our feature branch 40*60517a1eSAndroid Build Coastguard Workergit push origin my-feature # Cause the branch to be created in your fork. 41*60517a1eSAndroid Build Coastguard Worker``` 42*60517a1eSAndroid Build Coastguard Worker 43*60517a1eSAndroid Build Coastguard WorkerFrom here, you then edit code and commit to your local branch. If you want to 44*60517a1eSAndroid Build Coastguard Workersave your work to github, you use `git push` to do so: 45*60517a1eSAndroid Build Coastguard Worker 46*60517a1eSAndroid Build Coastguard Worker``` 47*60517a1eSAndroid Build Coastguard Workergit push origin my-feature 48*60517a1eSAndroid Build Coastguard Worker``` 49*60517a1eSAndroid Build Coastguard Worker 50*60517a1eSAndroid Build Coastguard WorkerOnce the code is in your github repo, you can then turn it into a Pull Request 51*60517a1eSAndroid Build Coastguard Workerto the actual rules_python project and begin the code review process. 52*60517a1eSAndroid Build Coastguard Worker 53*60517a1eSAndroid Build Coastguard Worker 54*60517a1eSAndroid Build Coastguard Worker## Running tests 55*60517a1eSAndroid Build Coastguard Worker 56*60517a1eSAndroid Build Coastguard WorkerRunning tests is particularly easy thanks to Bazel, simply run: 57*60517a1eSAndroid Build Coastguard Worker 58*60517a1eSAndroid Build Coastguard Worker``` 59*60517a1eSAndroid Build Coastguard Workerbazel test //... 60*60517a1eSAndroid Build Coastguard Worker``` 61*60517a1eSAndroid Build Coastguard Worker 62*60517a1eSAndroid Build Coastguard WorkerAnd it will run all the tests it can find. The first time you do this, it will 63*60517a1eSAndroid Build Coastguard Workerprobably take long time because various dependencies will need to be downloaded 64*60517a1eSAndroid Build Coastguard Workerand setup. Subsequent runs will be faster, but there are many tests, and some of 65*60517a1eSAndroid Build Coastguard Workerthem are slow. If you're working on a particular area of code, you can run just 66*60517a1eSAndroid Build Coastguard Workerthe tests in those directories instead, which can speed up your edit-run cycle. 67*60517a1eSAndroid Build Coastguard Worker 68*60517a1eSAndroid Build Coastguard Worker## Formatting 69*60517a1eSAndroid Build Coastguard Worker 70*60517a1eSAndroid Build Coastguard WorkerStarlark files should be formatted by 71*60517a1eSAndroid Build Coastguard Worker[buildifier](https://github.com/bazelbuild/buildtools/blob/master/buildifier/README.md). 72*60517a1eSAndroid Build Coastguard WorkerOtherwise the Buildkite CI will fail with formatting/linting violations. 73*60517a1eSAndroid Build Coastguard WorkerWe suggest using a pre-commit hook to automate this. 74*60517a1eSAndroid Build Coastguard WorkerFirst [install pre-commit](https://pre-commit.com/#installation), 75*60517a1eSAndroid Build Coastguard Workerthen run 76*60517a1eSAndroid Build Coastguard Worker 77*60517a1eSAndroid Build Coastguard Worker```shell 78*60517a1eSAndroid Build Coastguard Workerpre-commit install 79*60517a1eSAndroid Build Coastguard Worker``` 80*60517a1eSAndroid Build Coastguard Worker 81*60517a1eSAndroid Build Coastguard Worker### Running buildifer manually 82*60517a1eSAndroid Build Coastguard Worker 83*60517a1eSAndroid Build Coastguard WorkerYou can also run buildifier manually. To do this, 84*60517a1eSAndroid Build Coastguard Worker[install buildifier](https://github.com/bazelbuild/buildtools/blob/master/buildifier/README.md), 85*60517a1eSAndroid Build Coastguard Workerand run the following command: 86*60517a1eSAndroid Build Coastguard Worker 87*60517a1eSAndroid Build Coastguard Worker```shell 88*60517a1eSAndroid Build Coastguard Worker$ buildifier --lint=fix --warnings=native-py -warnings=all WORKSPACE 89*60517a1eSAndroid Build Coastguard Worker``` 90*60517a1eSAndroid Build Coastguard Worker 91*60517a1eSAndroid Build Coastguard WorkerReplace the argument "WORKSPACE" with the file that you are linting. 92*60517a1eSAndroid Build Coastguard Worker 93*60517a1eSAndroid Build Coastguard Worker## Contributor License Agreement 94*60517a1eSAndroid Build Coastguard Worker 95*60517a1eSAndroid Build Coastguard WorkerContributions to this project must be accompanied by a Contributor License 96*60517a1eSAndroid Build Coastguard WorkerAgreement. You (or your employer) retain the copyright to your contribution, 97*60517a1eSAndroid Build Coastguard Workerthis simply gives us permission to use and redistribute your contributions as 98*60517a1eSAndroid Build Coastguard Workerpart of the project. Head over to <https://cla.developers.google.com/> to see 99*60517a1eSAndroid Build Coastguard Workeryour current agreements on file or to sign a new one. 100*60517a1eSAndroid Build Coastguard Worker 101*60517a1eSAndroid Build Coastguard WorkerYou generally only need to submit a CLA once, so if you've already submitted one 102*60517a1eSAndroid Build Coastguard Worker(even if it was for a different project), you probably don't need to do it 103*60517a1eSAndroid Build Coastguard Workeragain. 104*60517a1eSAndroid Build Coastguard Worker 105*60517a1eSAndroid Build Coastguard Worker## Code reviews 106*60517a1eSAndroid Build Coastguard Worker 107*60517a1eSAndroid Build Coastguard WorkerAll submissions, including submissions by project members, require review. We 108*60517a1eSAndroid Build Coastguard Workeruse GitHub pull requests for this purpose. Consult [GitHub Help] for more 109*60517a1eSAndroid Build Coastguard Workerinformation on using pull requests. 110*60517a1eSAndroid Build Coastguard Worker 111*60517a1eSAndroid Build Coastguard Worker[GitHub Help]: https://help.github.com/articles/about-pull-requests/ 112*60517a1eSAndroid Build Coastguard Worker 113*60517a1eSAndroid Build Coastguard Worker### Commit messages 114*60517a1eSAndroid Build Coastguard Worker 115*60517a1eSAndroid Build Coastguard WorkerCommit messages (upon merging) and PR messages should follow the [Conventional 116*60517a1eSAndroid Build Coastguard WorkerCommits](https://www.conventionalcommits.org/) style: 117*60517a1eSAndroid Build Coastguard Worker 118*60517a1eSAndroid Build Coastguard Worker``` 119*60517a1eSAndroid Build Coastguard Workertype(scope)!: <summary> 120*60517a1eSAndroid Build Coastguard Worker 121*60517a1eSAndroid Build Coastguard Worker<body> 122*60517a1eSAndroid Build Coastguard Worker 123*60517a1eSAndroid Build Coastguard WorkerBREAKING CHANGE: <summary> 124*60517a1eSAndroid Build Coastguard Worker``` 125*60517a1eSAndroid Build Coastguard Worker 126*60517a1eSAndroid Build Coastguard WorkerWhere `(scope)` is optional, and `!` is only required if there is a breaking change. 127*60517a1eSAndroid Build Coastguard WorkerIf a breaking change is introduced, then `BREAKING CHANGE:` is required; see 128*60517a1eSAndroid Build Coastguard Workerthe [Breaking Changes](#breaking-changes) section for how to introduce breaking 129*60517a1eSAndroid Build Coastguard Workerchanges. 130*60517a1eSAndroid Build Coastguard Worker 131*60517a1eSAndroid Build Coastguard WorkerCommon `type`s: 132*60517a1eSAndroid Build Coastguard Worker 133*60517a1eSAndroid Build Coastguard Worker* `build:` means it affects the building or development workflow. 134*60517a1eSAndroid Build Coastguard Worker* `docs:` means only documentation is being added, updated, or fixed. 135*60517a1eSAndroid Build Coastguard Worker* `feat:` means a user-visible feature is being added. 136*60517a1eSAndroid Build Coastguard Worker* `fix:` means a user-visible behavior is being fixed. 137*60517a1eSAndroid Build Coastguard Worker* `refactor:` means some sort of code cleanup that doesn't change user-visible behavior. 138*60517a1eSAndroid Build Coastguard Worker* `revert:` means a prior change is being reverted in some way. 139*60517a1eSAndroid Build Coastguard Worker* `test:` means only tests are being added. 140*60517a1eSAndroid Build Coastguard Worker 141*60517a1eSAndroid Build Coastguard WorkerFor the full details of types, see 142*60517a1eSAndroid Build Coastguard Worker[Conventional Commits](https://www.conventionalcommits.org/). 143*60517a1eSAndroid Build Coastguard Worker 144*60517a1eSAndroid Build Coastguard Worker## Generated files 145*60517a1eSAndroid Build Coastguard Worker 146*60517a1eSAndroid Build Coastguard WorkerSome checked-in files are generated and need to be updated when a new PR is 147*60517a1eSAndroid Build Coastguard Workermerged: 148*60517a1eSAndroid Build Coastguard Worker 149*60517a1eSAndroid Build Coastguard Worker* **requirements lock files**: These are usually generated by a 150*60517a1eSAndroid Build Coastguard Worker `compile_pip_requirements` update target, which is usually in the same directory. 151*60517a1eSAndroid Build Coastguard Worker e.g. `bazel run //docs:requirements.update` 152*60517a1eSAndroid Build Coastguard Worker 153*60517a1eSAndroid Build Coastguard Worker## Core rules 154*60517a1eSAndroid Build Coastguard Worker 155*60517a1eSAndroid Build Coastguard WorkerThe bulk of this repo is owned and maintained by the Bazel Python community. 156*60517a1eSAndroid Build Coastguard WorkerHowever, since the core Python rules (`py_binary` and friends) are still 157*60517a1eSAndroid Build Coastguard Workerbundled with Bazel itself, the Bazel team retains ownership of their stubs in 158*60517a1eSAndroid Build Coastguard Workerthis repository. This will be the case at least until the Python rules are 159*60517a1eSAndroid Build Coastguard Workerfully migrated to Starlark code. 160*60517a1eSAndroid Build Coastguard Worker 161*60517a1eSAndroid Build Coastguard WorkerPractically, this means that a Bazel team member should approve any PR 162*60517a1eSAndroid Build Coastguard Workerconcerning the core Python logic. This includes everything under the `python/` 163*60517a1eSAndroid Build Coastguard Workerdirectory except for `pip.bzl` and `requirements.txt`. 164*60517a1eSAndroid Build Coastguard Worker 165*60517a1eSAndroid Build Coastguard WorkerIssues should be triaged as follows: 166*60517a1eSAndroid Build Coastguard Worker 167*60517a1eSAndroid Build Coastguard Worker- Anything concerning the way Bazel implements the core Python rules should be 168*60517a1eSAndroid Build Coastguard Worker filed under [bazelbuild/bazel](https://github.com/bazelbuild/bazel), using 169*60517a1eSAndroid Build Coastguard Worker the label `team-Rules-python`. 170*60517a1eSAndroid Build Coastguard Worker 171*60517a1eSAndroid Build Coastguard Worker- If the issue specifically concerns the rules_python stubs, it should be filed 172*60517a1eSAndroid Build Coastguard Worker here in this repository and use the label `core-rules`. 173*60517a1eSAndroid Build Coastguard Worker 174*60517a1eSAndroid Build Coastguard Worker- Anything else, such as feature requests not related to existing core rules 175*60517a1eSAndroid Build Coastguard Worker functionality, should also be filed in this repository but without the 176*60517a1eSAndroid Build Coastguard Worker `core-rules` label. 177*60517a1eSAndroid Build Coastguard Worker 178*60517a1eSAndroid Build Coastguard Worker(breaking-changes)= 179*60517a1eSAndroid Build Coastguard Worker## Breaking Changes 180*60517a1eSAndroid Build Coastguard Worker 181*60517a1eSAndroid Build Coastguard WorkerBreaking changes are generally permitted, but we follow a 3-step process for 182*60517a1eSAndroid Build Coastguard Workerintroducing them. The intent behind this process is to balance the difficulty of 183*60517a1eSAndroid Build Coastguard Workerversion upgrades for users, maintaining multiple code paths, and being able to 184*60517a1eSAndroid Build Coastguard Workerintroduce modern functionality. 185*60517a1eSAndroid Build Coastguard Worker 186*60517a1eSAndroid Build Coastguard WorkerThe general process is: 187*60517a1eSAndroid Build Coastguard Worker 188*60517a1eSAndroid Build Coastguard Worker1. In version `N`, introduce the new behavior, but it must be disabled by 189*60517a1eSAndroid Build Coastguard Worker default. Users can opt into the new functionality when they upgrade to 190*60517a1eSAndroid Build Coastguard Worker version `N`, which lets them try it and verify functionality. 191*60517a1eSAndroid Build Coastguard Worker2. In version `N+1`, the new behavior can be enabled by default. Users can 192*60517a1eSAndroid Build Coastguard Worker opt out if necessary, but doing so causes a warning to be issued. 193*60517a1eSAndroid Build Coastguard Worker3. In version `N+2`, the new behavior is always enabled and cannot be opted out 194*60517a1eSAndroid Build Coastguard Worker of. The API for the control mechanism can be removed in this release. 195*60517a1eSAndroid Build Coastguard Worker 196*60517a1eSAndroid Build Coastguard WorkerNote that the `+1` and `+2` releases are just examples; the steps are not 197*60517a1eSAndroid Build Coastguard Workerrequired to happen in immedially subsequent releases. 198*60517a1eSAndroid Build Coastguard Worker 199*60517a1eSAndroid Build Coastguard Worker 200*60517a1eSAndroid Build Coastguard Worker### How to control breaking changes 201*60517a1eSAndroid Build Coastguard Worker 202*60517a1eSAndroid Build Coastguard WorkerThe details of the control mechanism will depend on the situation. Below is 203*60517a1eSAndroid Build Coastguard Workera summary of some different options. 204*60517a1eSAndroid Build Coastguard Worker 205*60517a1eSAndroid Build Coastguard Worker* Environment variables are best for repository rule behavior. Environment 206*60517a1eSAndroid Build Coastguard Worker variables can be propagated to rules and macros using the generated 207*60517a1eSAndroid Build Coastguard Worker `@rules_python_internal//:config.bzl` file. 208*60517a1eSAndroid Build Coastguard Worker* Attributes are applicable to macros and regular rules, especially when the 209*60517a1eSAndroid Build Coastguard Worker behavior is likely to vary on a per-target basis. 210*60517a1eSAndroid Build Coastguard Worker* [User defined build settings](https://bazel.build/extending/config#user-defined-build-settings) 211*60517a1eSAndroid Build Coastguard Worker (aka custom build flags) are applicable for rules when the behavior change 212*60517a1eSAndroid Build Coastguard Worker generally wouldn't vary on a per-target basis. They also have the benefit that 213*60517a1eSAndroid Build Coastguard Worker an entire code base can have them easily enabled by a bazel command line flag. 214*60517a1eSAndroid Build Coastguard Worker* Allowlists allow a project to centrally control if something is 215*60517a1eSAndroid Build Coastguard Worker enabled/disabled. Under the hood, they are basically a specialized custom 216*60517a1eSAndroid Build Coastguard Worker build flag. 217*60517a1eSAndroid Build Coastguard Worker 218*60517a1eSAndroid Build Coastguard WorkerNote that attributes and flags can seamlessly interoperate by having the default 219*60517a1eSAndroid Build Coastguard Workercontrolled by a flag, and an attribute can override the flag setting. This 220*60517a1eSAndroid Build Coastguard Workerallows a project to enable the new behavior by default while they work to fix 221*60517a1eSAndroid Build Coastguard Workerproblematic cases to prepare for the next upgrade. 222*60517a1eSAndroid Build Coastguard Worker 223*60517a1eSAndroid Build Coastguard Worker### What is considered a breaking change? 224*60517a1eSAndroid Build Coastguard Worker 225*60517a1eSAndroid Build Coastguard WorkerPrecisely defining what constitutes a breaking change is hard because it's 226*60517a1eSAndroid Build Coastguard Workereasy for _someone, somewhere_ to depend on _some_ observable behavior, despite 227*60517a1eSAndroid Build Coastguard Workerour best efforts to thoroughly document what is or isn't supported and hiding 228*60517a1eSAndroid Build Coastguard Workerany internal details. 229*60517a1eSAndroid Build Coastguard Worker 230*60517a1eSAndroid Build Coastguard WorkerIn general, something is considered a breaking change when it changes the 231*60517a1eSAndroid Build Coastguard Workerdirect behavior of a supported public API. Simply being able to observe a 232*60517a1eSAndroid Build Coastguard Workerbehavior change doesn't necessarily mean it's a breaking change. 233*60517a1eSAndroid Build Coastguard Worker 234*60517a1eSAndroid Build Coastguard WorkerLong standing undocumented behavior is a large grey area and really depends on 235*60517a1eSAndroid Build Coastguard Workerhow load-bearing it has become and what sort of reasonable expectation of 236*60517a1eSAndroid Build Coastguard Workerbehavior there is. 237*60517a1eSAndroid Build Coastguard Worker 238*60517a1eSAndroid Build Coastguard WorkerHere's some examples of what would or wouldn't be considered a breaking change. 239*60517a1eSAndroid Build Coastguard Worker 240*60517a1eSAndroid Build Coastguard WorkerBreaking changes: 241*60517a1eSAndroid Build Coastguard Worker * Renaming an function argument for public functions. 242*60517a1eSAndroid Build Coastguard Worker * Enforcing stricter validation than was previously required when there's a 243*60517a1eSAndroid Build Coastguard Worker sensible reason users would run afoul of it. 244*60517a1eSAndroid Build Coastguard Worker * Changing the name of a public rule. 245*60517a1eSAndroid Build Coastguard Worker 246*60517a1eSAndroid Build Coastguard WorkerNot breaking changes: 247*60517a1eSAndroid Build Coastguard Worker * Upgrading dependencies 248*60517a1eSAndroid Build Coastguard Worker * Changing internal details, such as renaming an internal file. 249*60517a1eSAndroid Build Coastguard Worker * Changing a rule to a macro. 250*60517a1eSAndroid Build Coastguard Worker 251*60517a1eSAndroid Build Coastguard Worker## FAQ 252*60517a1eSAndroid Build Coastguard Worker 253*60517a1eSAndroid Build Coastguard Worker### Installation errors when during `git commit` 254*60517a1eSAndroid Build Coastguard Worker 255*60517a1eSAndroid Build Coastguard WorkerIf you did `pre-commit install`, various tools are run when you do `git commit`. 256*60517a1eSAndroid Build Coastguard WorkerThis might show as an error such as: 257*60517a1eSAndroid Build Coastguard Worker 258*60517a1eSAndroid Build Coastguard Worker``` 259*60517a1eSAndroid Build Coastguard Worker[INFO] Installing environment for https://github.com/psf/black. 260*60517a1eSAndroid Build Coastguard Worker[INFO] Once installed this environment will be reused. 261*60517a1eSAndroid Build Coastguard Worker[INFO] This may take a few minutes... 262*60517a1eSAndroid Build Coastguard WorkerAn unexpected error has occurred: CalledProcessError: command: ... 263*60517a1eSAndroid Build Coastguard Worker``` 264*60517a1eSAndroid Build Coastguard Worker 265*60517a1eSAndroid Build Coastguard WorkerTo fix, you'll need to figure out what command is failing and why. Because these 266*60517a1eSAndroid Build Coastguard Workerare tools that run locally, its likely you'll need to fix something with your 267*60517a1eSAndroid Build Coastguard Workerenvironment or the installation of the tools. For Python tools (e.g. black or 268*60517a1eSAndroid Build Coastguard Workerisort), you can try using a different Python version in your shell by using 269*60517a1eSAndroid Build Coastguard Workertools such as [pyenv](https://github.com/pyenv/pyenv). 270