Skip to content
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

Use umbrella crate for minimal template #5155

Merged
merged 59 commits into from
Aug 28, 2024

Conversation

pgherveou
Copy link
Contributor

No description provided.

@pgherveou pgherveou added R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. labels Jul 26, 2024
@pgherveou pgherveou marked this pull request as draft July 26, 2024 14:09
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Beautiful

templates/minimal/node/Cargo.toml Outdated Show resolved Hide resolved
@ggwpez
Copy link
Member

ggwpez commented Jul 26, 2024

Some of the clippy warnings seem to come from the experimental feature...

@pgherveou
Copy link
Contributor Author

pgherveou commented Jul 26, 2024

Some of the clippy warnings seem to come from the experimental feature...

Yeah this is weird as the experimental feature seems to be propagated correctly, clippy works fine locally too 🤔

@pgherveou pgherveou marked this pull request as ready for review July 28, 2024 09:36
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I think we are waiting for a new release before we can merge this?

also would like to see https://github.com/paritytech/polkadot-sdk/pull/5155/files#r1703858118 addressed.

@kianenigma kianenigma added the T17-Templates This PR/Issue is related to templates label Aug 5, 2024
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Thanks for the demo!

@kianenigma kianenigma requested a review from a team as a code owner August 13, 2024 14:17
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Made a few fixes me self now, and now it is all good!

@paritytech-review-bot paritytech-review-bot bot requested a review from a team August 26, 2024 21:28
impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> {
impl<'a, E: Ext + 'a, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> {
Copy link
Member

Choose a reason for hiding this comment

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

Merge error? This shouldn't be necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Curious. I guess due to the difference in toolchain version then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so it does now when we build

SUBSTRATE_RUNTIME_TARGET=riscv cargo check -p minimal-template-runtime

because it pulls the umbrella crate that tries to build all the crates deps including pallet-revive to riscv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kianenigma that explains why we are seeing all these compilations errors here and not in other PRs.
We could pick only the features we need in the Cargo.toml if we want to avoid that ...

Copy link
Member

Choose a reason for hiding this comment

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

Hm, yea only selecting the needed dependencies should work.
It's a bit annoying though, since the umbrella crate is supposed to make things easier. I guess it is still easier than selecting each dependency version manually.

Would it be possible to feature-gate the RISCV compilation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gated the pallets that were causing issue, the long term solution is probably to generate the umbrella trait with feature flags that declare all their dependencies, so that you can hand pick the one you need and optimize your build time

Copy link
Member

Choose a reason for hiding this comment

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

yea only selecting the needed dependencies should work.
It's a bit annoying though, since the umbrella crate is supposed to make things easier.

Couldn't we encode the dependency graph into the features in the script? Like you only activate the pallets you want and then they automatically activate the other pallets they depend on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not support making the usage of umbrella crate in any of the templates sub-optimal, for the sake of SUBSTRATE_RUNTIME_TARGET=riscv which is more of an experimental thing.

All people out there will not face the same compilation issue, and people do use these templates as a source of truth, so it should be in the simplest form possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to self: maybe this can be rolled back as well

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

I think with this we kill two birds with one stone. Cut down on compile times for our users and fix our CI.

@kianenigma kianenigma added this pull request to the merge queue Aug 28, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2024
Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Merged via the queue into master with commit 9cf5e81 Aug 28, 2024
187 of 189 checks passed
@kianenigma kianenigma deleted the pg/use-umbrella-crate-for-minimal-template branch August 28, 2024 14:32
ordian added a commit that referenced this pull request Aug 29, 2024
* master: (39 commits)
  short-term fix for para inherent weight overestimation (#5082)
  CI: Add backporting bot (#4795)
  Fix benchmark failures when using `insecure_zero_ed` flag (#5354)
  Command bot GHA v2 - /cmd <cmd> (#5457)
  Remove pallet::getter usage from treasury (#4962)
  Bump blake2b_simd from 1.0.1 to 1.0.2 (#5404)
  Bump rustversion from 1.0.14 to 1.0.17 (#5405)
  Bridge zombienet tests: remove old command (#5434)
  polkadot-parachain: Add omni-node variant with u64 block number (#5269)
  Refactor verbose test (#5506)
  Use umbrella crate for minimal template (#5155)
  IBP Coretime Polkadot bootnodes (#5499)
  rpc server: listen to `ipv6 socket` if available and `--experimental-rpc-endpoint` CLI option (#4792)
  Update approval-voting-regression-bench (#5504)
  change try-runtime rpc domains (#5443)
  polkadot-parachain-bin: Remove contracts parachain (#5471)
  Add feature to allow Aura collator to use full PoV size (#5393)
  Adding stkd bootnodes (#5470)
  Make `PendingConfigs` storage item public (#5467)
  frame-omni-bencher maintenance (#5466)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. T17-Templates This PR/Issue is related to templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants