-
Notifications
You must be signed in to change notification settings - Fork 262
Home
This repository contains a set of core Rust crates that power much of the Zcash ecosystem. As such, they have many different downstream consumers with differing needs. In this wiki we document how we develop the crates, and how changes can flow through from a proposed idea to being included in a published production crate release.
This repository is currently developed with an "unstable main
" workflow. The current contents of the main
branch is a preview of what the next full release of all crates may look like, but is not stable. For example, as-yet-unreleased zcash_client_sqlite
migrations may be altered incompatibly at any time.
In the main
branch, all crates have the version corresponding to their most recent stable release on https://crates.io; this enables the preview state to be tested ahead-of-time by downstream users via [patch.crates-io]
directives.
Individual crates have their own tags, e.g. zcash_primitives-0.19.0
. These tags point to the Git commit at which that crate version was published (which in general is not the merge commit for a release branch, but the actual commit that incremented the crate's version). Note however that other crates should not be considered stable at that revision, even if the tagged crate depends on them (there may be backwards-incompatible changes to upstream crates in the branch that are not used by the tagged crate and do not affect its public API).
Changes are generally made by branching from the current (at time of branching) state of main
, creating the desired commits, and then opening a PR targeting main
.
We have a strong preference for a clean commit history. We will actively rebase PRs to squash changes (such as bugfixes or responses to review comments) into the relevant earlier branch commits.
We have a strong preference for preserving commit history. PRs are generally merged to their target branch with merge commits. We do not use the "rebase-merge" option in GitHub. We will avoid using the "squash-merge" option in GitHub except on a case-by-case basis for PRs that do not have clean commit histories.
If the contents of main
changes in a way that creates a merge conflict in a PR (either explicit such that GitHub detects it and prevents PR merging, or implicit such that CI detects it via test failures when testing the merged state), we rebase the PR on top of the latest state of main
, updating each commit as necessary to address the conflicts.
In order to keep larger changes to a manageable size for review, we use Stacked PRs:
- Each PR after the first branches from, and targets, the branch of the "parent" PR.
- When an earlier PR changes, each subsequent PR's branch is rebased in sequence on its "parent" PR's branch.
We do not currently use specific tooling to aid with PR stacking.
Our rebase-heavy development workflow can interact poorly with PR review, because GitHub prevents reviewers from adding review comments to a pre-rebase PR state and forces them to refresh their webpage (losing review state).
To get around this GitHub UI limitation, the general process we follow is:
- Before a PR gets any review, PR authors rebase whenever they want.
- If anyone does not want the PR to be rebased (e.g. because they are actively reviewing it or because rebasing would make future reviews more difficult), they add the
S-please-do-not-rebase
label. - While the PR author sees this label or while they know someone is reviewing the PR, they avoid rebasing or force-pushing.
- The PR author adjusts the branch as necessary to address any comments. They may always add new commits. If
S-please-do-not-rebase
is not present then they can also force-push or rebase previous commits. In any case they push the result to the branch. - In cases where it is likely to aid reviewers, the PR author also posts a comment to the PR with a diff link between the previous branch tip and the new branch tip. When submitting a review for a PR, reviewers note the commit up to which the review covers; this aids PR authors in constructing these diff links.
- If the author would like to rebase the branch but
S-please-do-not-rebase
is present, they ask on Slack whether rebasing is okay. If everyone is agreed that it is no longer needed, they remove the label. - PR authors try to separate target branch rebases from addressing comments. If a rebase is needed to fix a merge conflict, that rebase is performed and force-pushed first (and a comment created with the corresponding diff link). After that, the necessary commit alterations are made to address review comments, followed by a second force-push (with a separate diff link).
If for whatever reason a particular PR becomes "too large" (for example, due to there not being a good way to split the contents down into stacked PRs), and significant review has started, then older commits in the PR will generally ossify. In that case we will add S-please-do-not-rebase
permanently, and avoid rebasing the PR from then on. We will switch to merging the target branch (e.g. main
) into the PR branch for merge conflict resolution, and commit changes in response to PR review as separate commits rather than updating the ossified earlier ones. Recent commits might still be okay to amend via force-push if they have not been reviewed yet, but if a PR is in this state then we generally tend to just eat the cost of the lower-value "addressed review comments" commits. This is a generally undesirable state for "leaf-level" change PRs, and we avoid it where possible.
If a PR author is non-responsive to review comments, we will generally make the necessary changes to the PR ourselves. For PRs created from user forks we can generally do this in the same PR. PRs from an organization forks do not allow changes from maintainers (due to missing cross-organization permissions); in this case (or if a user's PR has "allow maintainers to edit" disabled), we will close the PR and open a new PR containing the commits from the old PR.