search

polkadot-fellows / runtimes

The various runtimes which make up the core subsystems of networks for which the Fellowship is represented.
GNU General Public License v3.0
143 stars 97 forks source link

single PR on polkadot-sdk version update #101

Open muharem opened 12 months ago

muharem commented 12 months ago

Problem:

An engineer seeking to integrate a feature from a new polkadot-sdk version must submit a pull request (PR) in which the versions of all crates from polkadot-sdk are updated. A single crate in most cases can not be updated, since they are interdependent. This necessitates the engineer to first address all breaking changes, resulting in a single PR covering various topics. This, in turn, leads to an extended review process for the PR.

I believe it's important to agree on how to handle such updates to ensure that everyone has the right expectations.

Examples: https://github.com/polkadot-fellows/runtimes/pull/87 https://github.com/polkadot-fellows/runtimes/pull/56

Possible Solutions:

There are several approaches that an engineer can take to tackle this:

[updated on 09.01.2024] please see new proposal in the comments - https://github.com/polkadot-fellows/runtimes/issues/101#issuecomment-1825557675, https://github.com/polkadot-fellows/runtimes/issues/101#issuecomment-1830309475 [updated

Option 1: minimal integration / flexible In this approach, an engineer updating to a new polkadot-sdk version will address, at the very least, all breaking changes in a manner that prepares it for production. The extent of additional changes is left to the discretion of the engineer.

Option 2: exhaustive In this scenario, one or more engineers ensure that all anticipated and reasonable features and fixes from the polkadot-sdk are integrated.

Option 3: regressive Here, an engineer tasked with updating all crates within the runtimes repository applies minimal changes with the objective of making the crates compile. New features are disabled, and adjustments to contracts or their items are made in a restrictive manner. The PR is opened and merged to unblock subsequent PRs. Engineers responsible for specific domains or those who have worked on new features then follow up with their own PRs. To support this, restrictive default pallet configs may be introduced on FRAME level.

Any other alternatives?

bkchr commented 12 months ago

Option 3: regressive Here, an engineer tasked with updating all crates within the runtimes repository applies minimal changes with the objective of making the crates compile. New features are disabled, and adjustments to contracts or their items are made in a restrictive manner. The PR is opened and merged to unblock subsequent PRs. Engineers responsible for specific domains or those who have worked on new features then follow up with their own PRs. To support this, restrictive default pallet configs may be introduced on FRAME level.

IMO this almost the same as Option 1 and I would go this road. The process right now is quite tedious. I hope that it improves with prdocs and better changelogs to make it easier for a dev to integrate the changes here. Explicit changes to the configuration should be done in other subsequent prs that are only focused at this one issue.

xlc commented 12 months ago

we really should engineer polkadot-sdk in such way that it doesn't have to be a breaking change on every release and have nightly release

muharem commented 11 months ago

we really should engineer polkadot-sdk in such way that it doesn't have to be a breaking change on every release and have nightly release

@xlc I totally agree. However, there will be breaking upgrades, and as someone who wants to introduce a new feature from a new release, you may not know what steps to take. This uncertainty might discourage contributions, and others might not anticipate such extensive PRs encompassing all changes. Ultimately, this can slow down the process.

@bkchr for example, a situation where I see the difference in these options is when making a change to an existing config's type parameter. Previously, it was Currency, but now it's Consideration trait bound. In Option 1, I would provide an implementation that's ready for production, while in Option 2, I can set it to a unit type with the expectation that it will be configured in subsequent PRs.

I'm not fond of Option 3 due to the risk of inadvertently releasing something disabled that was not intended to be.

muharem commented 11 months ago

I would better put it as follows, A new release can encompass the following:

  1. breaking changes to existing functionality.
  2. breaking changes introducing new functionality.
  3. non-breaking changes to existing functionality.
  4. new functionality.

The first two can be configured or disabled, while the last two can be setup or left unspecified. Where it is configured or setup as ready for production.

muharem commented 11 months ago

I would vote for the approach where,

  1. must be configured as ready for prod
  2. should be disabled
  3. should be left unset
  4. should be left unset

for example in a last such PR, the new weight fee impl should be left unset, and a separate PR should be raised

bkontur commented 10 months ago

There is an upcoming PR with Snowbridge that will require polkadot-sdk 1.6 release. Currently, the fellows' runtimes are aligned with 1.3, so we need to bump to 1.4 and 1.5 before.

I would like to start a PR for the bump to 1.4 and 1.5, similar to what @svyatonik did for the bump from 1.0 to 1.3 here and see how it goes.

Is there a consensus on how to proceed with bumping to 1.4 and 1.5?

muharem commented 10 months ago

@bkchr Since there is no agreement yet, I think you as an author are free to decide which approach to take. I would advertise my proposal from above, curious to see how it will go.

I would vote for the approach where,

  1. must be configured as ready for prod
  2. should be disabled
  3. should be left unset
  4. should be left unset

for example in a last such PR, the new weight fee impl should be left unset, and a separate PR should be raised

gilescope commented 10 months ago

When polkadot-sdk needs to be upgraded, that should be done as a separate PR (along the lines of option 1) and then the additional functionality that wants to be enabled should be done as an additional PR that builds upon that first PR. Having a mishmash of changing things and upgrading polkadot-sdk seems messy (and makes changes harder to rollback). Ideally there should be boring PRs that upgrade polkadot-sdk for each released node version so that the PRs are easy to review.

bkontur commented 10 months ago

very dumb question, actual repo is bumped to polkadot@1.3, but actual latest is polkadot@1.5, so do we need to create at first PR for bumping to polkadot@1.4 + fix compilation + fix/add all features and prepare for release and then do the same for 1.4 to 1.5? Or can we just bump from 1.3 to polkadot@1.5?

@muharem how did you bump to 1.2 here? Manually increased major version by 1 or did you use any script or command?

iiuc, @svyatonik Slava did bump from 1.2 to 1.3 with cargo upgrade --pinned --incompatible here.

So, I did cargo upgrade (the same as Slava) from 1.3 -> 1.5 (latest) here: https://github.com/polkadot-fellows/runtimes/pull/137.

Sorry, maybe I've missed it, is there any guidance/readme for releasing fellows runtimes?

muharem commented 10 months ago

@bkontur I did it just with find and replace, but there is probably a better way. I would just bump it to the latest.

bkontur commented 7 months ago

My two cents for the discussion: points or lessons learned from a practical point of view when bumping to polkadot-sdk@1.5/1.6/1.7

CI for "everything" code-related.

Definitely, we must have reliable CI pipelines for "everything" code-related. Currently, we are missing:

Release checklist

Bump polkadot-sdk

When the fellows' repository is behind several polkadot-sdk releases, we should bump to the latest one and avoid creating semi-bumps (unless there is a reason):

My story with bumping to 1.5 / 1.6 / 1.7

This is what I did for 1.2.0 release, along with suggestions for the next release, 1.3.0:

I can't say if this is Option 1, Option 2, or Option 3, it's probably a combination. However, I would choose the pragmatic approach: compile as soon as possible to unblock others from creating additional PRs. Nonetheless, it can't be merged without approvals, and you don't want to merge code that doesn't compile.

muharem commented 7 months ago

we should bump to the latest one and avoid creating semi-bumps

Do you mean we should upgrade to the final sdk version we plan for release, for 1.2 it would be one PR with sdk 1.7 instead three PRs with 1.5, 1.6, 1.7 upgrades? If yes, I agree with it. Not sure how this matches with your suggestion for dependancy bot that would create PR for every new version of sdk.

(Nice-to-have) CI job for regenerating weights.

For me this is not nice to have, but we should have it. May be not CI job for every PR, but some command that can be manually initiated.

bkontur commented 6 months ago

we should bump to the latest one and avoid creating semi-bumps

Do you mean we should upgrade to the final sdk version we plan for release, for 1.2 it would be one PR with sdk 1.7 instead three PRs with 1.5, 1.6, 1.7 upgrades? If yes, I agree with it.

Yes, I mean, when doing manually, there is no reason to do semi-bumps and waste time, so just bumping directly to the actual latest polkadot-sdk release is enough.

Not sure how this matches with your suggestion for dependancy bot that would create PR for every new version of sdk.

But, if we want to do it automatically, we can use dependabot (or other tool).

How's the situation today? We've released fellows 1.2.0 with polkadot-sdk 1.7.0 (plus several patches). In the meantime, we've also released polkadot-sdk 1.8, 1.9, 1.10, and this week 1.11 is coming. So, who's working on bumping the fellows repo? Well, unfortunately, nobody. And why?

For instance, if we set up dependabot (or another tool), it could at least create a branch with bumped dependencies for 1.8, 1.9, and/or 1.10. There's a higher chance that somebody could pick it up if they see that some easy stuff is not compiling or whatever (at least a few commits could help). It's a better starting point than having no branch at all, because I guess that people don't even know how to start this bumping manually.

And maybe one day (in an ideal world and with the right alignment of stars and moons), we could have a polkadot-sdk release that makes no breaking changes (yes, we are working on that with DefaultConfigs, semver and other stuff, which is cool). Then, we would be able to merge directly the bumped branch from dependabot, similar to how we do it for other dependencies, like Bump anyhow from 1.0.75 to 1.0.81.

xlc commented 6 months ago

I will suggest a regular release pipeline. e.g. we try to make a Polkadot runtime regular release every month. We can have a bot or something to create a release branch and everything not yet merged will require extra justification for it to be included. And then the same bot also can create a PR to bump polkadot-sdk and ready for someone to pick it up.

muharem commented 6 months ago

@bkontur I think dependabot might result a very similar experience with what you had with 1.5/1.6/1.7 PRs. But I am not against trying. For now I was thinking that we could first choose a final sdk version for the next runtime release. And after we have that agreement we open one PR to upgrade runtimes to a chosen sdk version.

ggwpez commented 3 months ago

I went for the exhaustive single-MR approach and it worked fine for 1.7->1.13 and 1.13->1.14. The trick is to ping every developer and delegate the changes in their submodule, since i have no clue how some things should be configured