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

runtimes behind features #2752

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

runtimes behind features #2752

wants to merge 15 commits into from

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Jun 18, 2023 •
edited
Loading

This past year we have added many runtimes. The disk space and compile times of cumulus has increased significantly. This PR puts each runtime behind a feature which will give us the headroom to allow us to continue to add further runtimes. (For bridges I've put them under one bridge-hub-runtimes feature).

A from scratch release build of just selecting asset-hub-kusama-runtime and asset-hub-polkadot-runtime for example takes less than half the time on my desktop. I suspect the time savings are even more on a laptop.

For now all runtimes are still built by default.

(asset_hub.rs has just been split up into three files - one per runtime.)

(A follow up PR will use these same runtime features in the integration tests so that one can run the tests of one runtime without having to build them all.)

TODO: Tidy up unused imports when only a subset of runtimes are built.

@gilescope gilescope added A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the 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 labels Jun 18, 2023
@gilescope gilescope requested review from NachoPal and bkchr June 18, 2023 20:32
@gilescope gilescope added the T0-node This PR/Issue is related to the topic “node”. label Jun 18, 2023
@gilescope gilescope changed the title runtime features runtimes behind features Jun 18, 2023
@gilescope
Copy link
Contributor Author

gilescope commented Jun 19, 2023 •
edited
Loading

Should rococo_parachain_runtime always be included (and not be behind a flag)?

NachoPal
NachoPal reviewed Jun 22, 2023
View reviewed changes
Copy link
Contributor

@NachoPal NachoPal left a comment

Choose a reason for hiding this comment

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

Very needed PR! I would love to see it merged.

polkadot-parachain/Cargo.toml
Comment on lines +129 to +130
glutton-runtime = [ "dep:glutton-runtime", "shell-runtime" ]
penpal-runtime = [ "dep:penpal-runtime", "rococo-parachain-runtime" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why shell-runtime and rococo-parachain-runtime are included here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They use bits of the other. Maybe they should not...

polkadot-parachain/Cargo.toml Show resolved Hide resolved
polkadot-parachain/Cargo.toml Show resolved Hide resolved
polkadot-parachain/src/command.rs Outdated Show resolved Hide resolved
polkadot-parachain/src/command.rs Outdated Show resolved Hide resolved
polkadot-parachain/src/command.rs Outdated Show resolved Hide resolved
polkadot-parachain/src/command.rs Outdated Show resolved Hide resolved
polkadot-parachain/src/command.rs Outdated Show resolved Hide resolved
polkadot-parachain/src/command.rs Outdated Show resolved Hide resolved
polkadot-parachain/src/command.rs Outdated Show resolved Hide resolved
@gilescope gilescope added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jun 30, 2023
@gilescope
Copy link
Contributor Author

Ah #[cfg(feature = "asset-hub-westend-runtime")] isn't a good idea - too many feaures (chain * relay features).
Better: #[cfg(all(feature = "asset-hub-runtime", feature = "westend-runtime"))]

I am thinking though that maybe for brevity we should just have:
#[cfg(all(feature = "asset-hub", feature = "westend"))]
(I can't see us having name clashes with other features)

@NachoPal NachoPal mentioned this pull request Jul 19, 2023
Open
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3336366

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NachoPal NachoPal NachoPal left review comments

@bkchr bkchr Awaiting requested review from bkchr

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the 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 T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gilescope @paritytech-cicd-pr @NachoPal