Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[Companion] BEEFY crates renaming #6799

Merged
merged 5 commits into from
Feb 28, 2023
Merged

[Companion] BEEFY crates renaming #6799

merged 5 commits into from
Feb 28, 2023

Conversation

davxy
Copy link
Member

@davxy davxy commented Feb 28, 2023

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

You shouldn't change Cargo.tomls (other than renaming the beefy crates) - aka it should still point to git = "https://github.com/paritytech/substrate", branch = "master"

The companion system will do the rest of the magic:

  1. it will cargo patch substrate to the right PR branch for the CI
  2. once the PR is merged, it will update substrate references here to the right commit hash in paritytech/substrate/master

Cargo.lock Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@davxy
Copy link
Member Author

davxy commented Feb 28, 2023

@acatangiu I know. Indeed is marked as DO NOT MERGE.

This is a trick to allow to have a successful pipeline when you rename some crates already in the lock file of polkadot.
Without this trick you can't patch polkadot using Diener in the Substrate job.

This will be reverted just before merge (I already used this trick for GRANDPA crates renaming in a prev PR)

In short the steps that I've used is:

  1. let polkadot companion point to your patched branch directly
  2. let the substrate polkadot companion job pass
  3. merge substrate PR to master
  4. revert polkadot companion to point to paritytech
  5. merge polkadot

AFAIK this currently is the only way to do this since diener doesn't support this (yet)

@acatangiu acatangiu dismissed their stale review February 28, 2023 13:06

as intended, temporary version

@davxy davxy added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. A0-pleasereview labels Feb 28, 2023
@davxy davxy self-assigned this Feb 28, 2023
@davxy davxy changed the title [Companion] BEEFY crates renaming [DO NOT MERGE] [Companion] BEEFY crates renaming Feb 28, 2023
@davxy davxy requested review from acatangiu and a team February 28, 2023 15:15
@michalkucharczyk michalkucharczyk requested a review from a team February 28, 2023 15:17
node/service/Cargo.toml Outdated Show resolved Hide resolved
@davxy davxy enabled auto-merge (squash) February 28, 2023 15:37
@@ -35,6 +34,7 @@ telemetry = { package = "sc-telemetry", git = "https://github.com/paritytech/sub
# Substrate Primitives
sp-authority-discovery = { git = "https://github.com/paritytech/substrate", branch = "master" }
consensus_common = { package = "sp-consensus", git = "https://github.com/paritytech/substrate", branch = "master" }
beefy-primitives = { package = "sp-consensus-beefy", git = "https://github.com/paritytech/substrate", branch = "master" }
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
beefy-primitives = { package = "sp-consensus-beefy", git = "https://github.com/paritytech/substrate", branch = "master" }
sp-consensus-beefy = { package = "sp-consensus-beefy", git = "https://github.com/paritytech/substrate", branch = "master" }

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. you are absolutely right! And I really would :-)
But then if I follow my instinct I have to rename all this stuff as well:

  • beefy-primitives -> sp-consensus-beefy
  • grandpa-primitives -> sp-consensus-grandpa
  • beefy -> sc-consensus-beefy
  • grandpa -> sc-consensus-grandpa
  • babe -> sc-consensus-babe
    (for the coherence reason 😃)

So probably I would merge this first, just to let polkadot to compile with current substrate master.
Then I can do that in a follow up PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants