Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Write some docs about the release process #2366

Closed
wants to merge 2 commits into from
Closed

Write some docs about the release process #2366

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d83ae2
Write some docs about the release process
bkchr Nov 16, 2023
cd768f8
Apply suggestions from code review
bkchr Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 75 additions & 6 deletions docs/CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ If a change does not alter any logic (e.g. comments, dependencies, docs), then i
`A1-insubstantial` and merged faster.
If it is an urgent fix with no large change to logic, then it may be merged after a non-author
contributor has reviewed it well and approved the review once CI is complete.
No PR should be merged until all reviews' comments are addressed.
No PR should be merged until all reviews' comments are addressed. It is expected that
there is no PR merged that is not ready. It is only allowed to merge such a PR if
the code is disabled, not used or in any other way clearly tagged as experimental.
It is expected that master is always releasable and that all PRs got tested well
enough that the likelihood of critical bugs is quite small.

### Labels

Expand All @@ -50,8 +54,6 @@ The set of labels and their description can be found [here](https://paritytech.g
`T13-documentation`. The docs team will get in touch.
5. If your PR changes files in these paths:

`polkadot` : `^runtime/polkadot`
`polkadot` : `^runtime/kusama`
`polkadot` : `^primitives/src/`
`polkadot` : `^runtime/common`
`substrate` : `^frame/`
Expand Down Expand Up @@ -133,9 +135,76 @@ Please label issues with the following labels:
2. `D*` issue difficulty, suggesting the level of complexity this issue has. AT MOST ONE ALLOWED.
3. `T*` Issue topic. MULTIPLE ALLOWED.

## Releases

Declaring formal releases remains the prerogative of the project maintainer(s).
## Release Process

We are aiming for a **two week** release process of the `polkadot-sdk` repository. The output of a release
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth mentioning/explaining the merge window? I.e. just because a PR got merged before a release, doesn't mean it made it into the release.

are the Parity `polkadot` node implementation, the runtimes for the Westend & Rococo networks and new versions
of the crates. Given the cadence of two weeks, there is no need to halt a release process for any kind
of PR (no BRAKES for the release train). There is only to be made an exception for high security issues
for code that is already running in production. However, these critical bug fixes may warrant an out of
band release anyway, but this should be decided based on the severity on a case by case basis.

### Versioning

We are releasing multiple different things from this repository in one release, but
we don't want to use the same version for everything. Thus, in the following we explain
the versioning story for the node, Westend & Rococo and the crates. The version associated
to a particular release is taken from the node version.

#### Node

The versioning of the node is done 99% of the time by only incrementing the `minor` version.
The `major` version is only bumped for special releases and the `patch` can be used for an
out of band release that fixes some critical bug. The node version is not following SemVer.
This means that the version doesn't express if there are any breaking changes in the CLI
interface or similar. The node version is declared in the `NODE_VERSION` variable in
`polkadot/node/primitives/src/lib.rs`.

#### Westend & Rococo

For the test networks we only increment the `spec_version`. The spec version is also following
the node version. So, `10020` is for example the node release `1.2.0`. This versioning has no
further meaning and is only done to map from an on chain `spec_version` easily to the
release in this repository.
Comment on lines +165 to +168
Copy link
Contributor

@joepetrowski joepetrowski Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have already not followed this. Rococo and Westend Relays are on 103000 (with 1.3.0 release) and the system paras are on 1_003_000 (using the Fellowship "style"). Switching to this would mean decreasing the version. Can we just use the Fellowship style for consistency, M_mmm_ppp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care. We can do this.


#### Crate

We want to follow SemVer as best as possible when it comes to crates versioning.
Following SemVer isn't that easy, but there exists [a guide](https://doc.rust-lang.org/cargo/reference/semver.html)
in the Rust documentation that explains the small details on when to bump what. The bumping
should be done in between the releases in the PRs. However, it isn't required to
bump e.g. the `patch` version multiple times in between two releases. There
exists a CI job that checks that the versions are not bumped multiple times to help
the developer. Another CI job is also checking for SemVer breaking changes. It is using
[`cargo-semver-checks`](https://github.com/obi1kenobi/cargo-semver-checks). While
the tool isn't perfect, it should help to remind the developer of checking the SemVer
compatibility of its changes. In general there is not any guarantee to downstream
that there isn't a breaking in between and thus a `major` (or `minor` on `<1.0.0`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
that there isn't a breaking in between and thus a `major` (or `minor` on `<1.0.0`)
that there isn't a breaking change in between and thus a `major` (or `minor` on `<1.0.0`)

bump. If possible, it should be prevented, but we also don't want to carry
"dead code" with us for too long. As long as there are clear instructions on how
to integrate the breaking changes it should be fine to break things.

So, the general approach is that developers are required to bump the versions in
all crates that they are changing. If there is a `major` (or `minor` on `<1.0.0`)
crate version bump, it is important to also bump any crate that depends on this
bumped version crate and re-export it. However, if the re-export is done in a
`__private` module (that makes clear that it is internal api) the `major`/`minor`
version bump doesn't ripple and only requires a `minor`/`patch` bump.
Comment on lines +187 to +192
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this paragraph. Can you please explain the re-exporting? What is the __private module (is this just some example name?) Why do we need to bump a crate version if only the internal API changed, is it just because our tooling requires it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a public re-export for macros. This is a known pattern to make people on the outside aware that they should not use this module. However, from a pure SemVer perspective, this would be a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I've never seen that pattern. I grepped and the __private looks like it's only used in substrate. But there is nothing substrate-specific about this doc or this "Crates" section, so it should be clarified. Also, I still don't understand this:

the major/minor version bump doesn't ripple and only requires a minor/patch bump.

But, this paragraph should just be rewritten, and ideally should explain the "why". For sure it is very hard to understand, especially for new contributors.


### Backports

Backports should most of the time not be required. We should only backport critical
bug fixes and then release the fixed crates. There should be no need to backport
anything from a release branch. Bumping the `NODE_VERSION` and `spec_version` of
the test networks should be done before the release process is started on master.
The only other change on release branches, new weights are not that important to
be backported.

When a backport is required for some previous release, it is the job of the
developer (assuming it is some internal person) that has created the initial PR
to create the backports. After the backports are done it is important to ensure
that the crate release is being done. We should backport fixes to the releases
of the last 6 months.

## UI tests

Expand Down
Loading