Skip to content

umbrella create Tweaks#5516

Closed
pgherveou wants to merge 2 commits intomasterfrom
pg/umbrella-tweak
Closed

umbrella create Tweaks#5516
pgherveou wants to merge 2 commits intomasterfrom
pg/umbrella-tweak

Conversation

@pgherveou
Copy link
Copy Markdown
Contributor

No description provided.

@pgherveou pgherveou requested a review from a team as a code owner August 29, 2024 09:28
Comment on lines -541 to -553
"frame-benchmarking",
"frame-benchmarking-pallet-pov",
"frame-election-provider-solution-type",
"frame-election-provider-support",
"frame-executive",
"frame-metadata-hash-extension",
"frame-support",
"frame-support-procedural",
"frame-support-procedural-tools-derive",
"frame-system",
"frame-system-benchmarking",
"frame-system-rpc-runtime-api",
"frame-try-runtime",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kianenigma @ggwpe a few tweaks: these crates don't need to be in the runtime features as well, as they are already included in polkadot-sdk-frame, they were also causing some annyoning warning when compiling

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

features names are not global. If the frame-benchmarking feature is not activated in this Cargo.toml then even if polkadot-sdk-frame has a feature with the same name, then the feature won't be activated. This means when compiled with feature runtime, the umbrella will not reexport frame-benchmarking. frame-benchmarking will only be available from polkadot-sdk-frame. Is it what we want?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

frame-benchmarking. frame-benchmarking will only be available from polkadot-sdk-frame. Is it what we want?

Yeah I think that is fine to access all frames thing through polkadot-sdk-frame

@paritytech-cicd-pr
Copy link
Copy Markdown

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

@paritytech-cicd-pr
Copy link
Copy Markdown

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

"frame-support/try-runtime",
"frame-system/try-runtime",
"sp-runtime/try-runtime"
]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need this, for tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that's added by zepter, automatically, I explicity added try-runtime feature in the cargo as it complains in some situation with

warning: unexpected `cfg` condition value: `try-runtime`
   --> substrate/frame/support/procedural/src/lib.rs:323:31

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in the past it has been avoided see this comment: #5099 (comment)

But in this exact case, it doesn't harm. But if it then get used for different usecase, then I don't know.

Note that we could fix it like this: https://github.com/paritytech/polkadot-sdk/compare/pg/umbrella-tweak...gui1117:gui-fix-try-runtime-usage-in-frame-support?expand=1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that we could fix it like this

Yea that looks like the proper fix 🙏


runtime_crates = [crate for crate in nostd_crates if 'frame' in crate[0].name or crate[0].name.startswith('sp-')]
frame_crates_names = [ "polkadot-sdk-frame", "frame-support", "frame-system", ]
runtime_crates = [crate for crate in nostd_crates if crate[0].name.startswith('sp-') or crate[0].name in frame_crates_names]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should then probably also have a runtime-full feature or something like that.

@pgherveou pgherveou closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants