fix: features #353
fix: features #353
Conversation
wischli
left a comment
wischli
left a comment
There was a problem hiding this comment.
LGTM! However, I cannot execute the fish script.
scripts/all.fish
Outdated
| @@ -0,0 +1,11 @@ | |||
| for features in "--all-features" "--features runtime-benchmarks" "--features try-runtime" | |||
There was a problem hiding this comment.
When I try to execute this, I receive an error: no such subcommand: workspaces`.
There was a problem hiding this comment.
You have to install cargo-workspaces. I will add a comment for that.
ntn-x2
left a comment
ntn-x2
left a comment
There was a problem hiding this comment.
Just two small questions, but otherwise LGTM!
| fast-gov = [] | ||
| runtime-benchmarks = [ | ||
| "attestation/runtime-benchmarks", | ||
| "frame-support/runtime-benchmarks", |
There was a problem hiding this comment.
Why does runtime common enable these features upon runtime benchmarks? I'm talking specifically about attestation and parachain-staking.
There was a problem hiding this comment.
we need to pass this feature down to all dependencies. If runtime-benchmarks is enabled, some method in some trait that is guarded by this feature might be enabled too. If attestation and frame-support implement that trait and we don't enable the feature, this method from the trait would be missing and compilation would fail. So we always need to pass down all features that we use to all the dependencies. Either runtime-benchmarks is active for all dependencies or for none. Mixing this would create terrible errors.
scripts/all.fish
Outdated
| @@ -0,0 +1,11 @@ | |||
| for features in "--all-features" "--features runtime-benchmarks" "--features try-runtime" | |||
There was a problem hiding this comment.
Would it also make sense to test with no (default) features, and with the mock feature, where specified? Anyway very good solution to the problem!
There was a problem hiding this comment.
yes that's a good idea
There was a problem hiding this comment.
default features are a good idea. But mock doesn'T work since not all of the crates have that feature
(cherry picked from commit a0b9f86)
* Adds two more relaychain bootnodes for staging environment (#334) * chore: reset peregrine stg (#335) * ci: use custom ci image (#336) * Optimizes docker layer (#337) * fix: add did lookup pallet to DID authorization logic + reverse lookup index (#343) * chore: update toolchain version to nightly 1.59 (#339) * feat: proxy type for disableling deposit claiming (#341) * fix: rococo protocol id (#369) * feat: generic access control (#316) * Updates toolchain version (#345) * refactor: enforce no runtime in pallet (#349) * fix: features (#353) * feat: add tips pallet (#352) * feat: upgrade to Polkadot v0.9.19 (#357) * chore: upgrade and clean up (#360) * Adds the new rococo chainspec (#363) * feat: add launch pallet removal migration (#359) * refactor: update rilt para id from 2015 to 2108 (#364) * fix: rilt para id (#365) * feat: upgrade to Polkadot v0.9.23 (#366) * use ci-linx:production base image (#368) * feat: upgrade to Polkadot v0.9.24 (#370) * fix: fix CI builders compilation errors and pin to a specific hash (#372)
Checklist:
array[3]useget(3), ...)