-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Bump pallet-staking-reward-fn #10905
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.
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
Conversation
9eeb7da to
322c618
Compare
|
Should we first try this in the PR where we see the issue, and see if it fixes the CI problem there? I don't quite understand why #10682 could cause this though; the patch bump is correct and I am sure it is the first time there ever was a patch bump 🙈 |
We have seen the issue in all PRs past #10682 to be honest, so I thought a dummy PR to test wouldn't hurt 😅 |
|
maybe @pgherveou, as author of #10682 , do you have suggestions / comments here? |
|
https://github.com/paritytech/polkadot-sdk/actions/runs/21364999186/job/61494755114?pr=10905 It actually works with the proposed change. This forces pallet-staking-reward-fn to version 24.0.1 (doesn't exist on crates.io), so it keeps its local path and uses the local sp-arithmetic 28.0.1 instead of pulling the registry version. |
|
So in the parity-publish CI job example we have:
What is a bit annoying is that check-publish-compile tries to defend against PRs which aren't updating all the needed dependencies, but it is not enough - this PR is a valid case where a previous PR rightfully bumps a certain crate that should've been also bumped with this PR, and one bump in an unrelease PR is enough for all the following PRs that would have needed to bump the same crate because their changes require it. If at some point we release that initial PR with the crate bump without the rest of the PRs that depend on that bump, check-publish-compile will start failing after that point (since we move all the released PRs in a stable* directory, so they aren't under the scope of check-publish-compile). We end up in such a situation frequently enough, and for some reason the release team is not impacted by this when compiling and publishing (and I don't know why they are not 😅 - most probably my mental model is wrong). |
|
Thanks @iulianbarbu !
@EgorPopelyaev can you comment on this part ? About this PR: do we want to keep as it is then, assuming that e.g. #10682 , #10582 and current #10905 will all end up in the next release (2603?) branch - which will probably be the case since we are going to release 2512 as next release and we don't plan to backport any of them AFAIK? |
|
@sigurpol tbh, in this particular case, I don't really get🙈, how in general did it happen, that the crate ( |
My (poor) understanding is that the CI job is simulating what would happen if we released all unreleased PRs from master and then did a patch release with this PR.
Dependency: polkadot-runtime-common depends on both sp-arithmetic and pallet-staking-reward-fn. pallet-staking-reward-fn depends on sp-arithmetic. CI in the parity-publish check-compile job applies all master unreleased prdocs: parity-publish plan --prdoc prdoc/ # includes #10682 and #10582 prdocs
parity-publish apply --registryThis bumps:
When rewriting deps with For // edit.rs logic
if sp_arithmetic_version_exists_on_crates_io(28.0.1) {
remove_path() // use registry
} else {
keep_path() // use local
}
For // edit.rs logic
if pallet_staking_reward_fn_version_exists_on_crates_io(24.0.0) {
remove_path() // use registry
} else {
keep_path() // use local
}
The mismatch:
Why it compiles with #10905: the prdoc bumps
|
Exactly. I think you could test how a release might be impacted by "simulating" a release for |
|
@iulianbarbu , @EgorPopelyaev should we merge this PR or do we want to wait for Egor to simulate a release with current master - check that it fails (or so we think) - then repeat the exercise with this PR in - check that it works? |
|
We can merge this, sorry for the radio silence. Also opened: #10943. |
Bump pallet-staking-reward-fn to patch. This forces it to use local path and the local sp-arithmetic instead of registry while running
parity-publish.This aims to fix the failure in
Check publish buildjob (e.g. https://github.com/paritytech/polkadot-sdk/actions/runs/21359458014/job/61474963706?pr=10903#step:12:546) which started to appear since #10682 got merged.When parity-publish is used with --registry:
The issue is that --registry creates a hybrid state where some deps use registry and some use local paths, causing version conflicts in case e.g of missing trait impl in on the two.
Extended explanation, courtesy of @iulianbarbu :
In the parity-publish CI job example we have:
polkadot-runtime-common fails to compile due to a dependency graph using two versions of same type of crate B. What is an issue though is that pallet-staking-reward-fn uses the previous crate B version (from the registry), which misses a certain trait impl that is required by polkadot-runtime-common. If polkadot-runtime-common usage of pallet-staking-reward-fn expects the new trait impl, it means that it is not enough to just bump pallet-staking-reward-fn, but also polkadot-runtime-common, in this PR (to update it to depend on the new version of pallet-staking-reward-fn). Everything compiles fine rn because polkadot-runtime-common is already bumped in a previous PR (e.g #10582 and maybe others), which did not make its way to a stable release yet.