This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Fix Rust features #11976
Merged
Fix Rust features #11976
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
ggwpez
added
A0-please_review
ggwpez
added
A3-in_progress
Member
|
@ggwpez you made a tool? share the link! |
shawntabrizi
approved these changes
Aug 4, 2022
bkchr
reviewed
Aug 4, 2022
Member
|
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
ggwpez
changed the title
std features to Cargo filesstd features to Cargo files
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Contributor
|
fyi there is progress being made on cargo fmt Cargo.toml. Very much still top of my xmas list this year: rust-lang/rustfmt#5240 |
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
|
The CI pipeline was cancelled due to failure one of the required jobs. |
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
kianenigma
approved these changes
Aug 18, 2022
Contributor
kianenigma
left a comment
kianenigma
left a comment
There was a problem hiding this comment.
Didn't look closely, but based on the description it all looks very good.
I have a similar request as well. If I create a new pallet now, and it has try-runtime feature, if I add it to a runtime and forgot to enable try-runtime, no normal compilation will work, but the try-runtime build will fail. Can you check for this as well? You can see a few instances of it in the diff of this PR: https://github.com/paritytech/substrate/pull/10174/files
athei
approved these changes
Aug 18, 2022
ggwpez
added
A0-please_review
gilescope
approved these changes
Aug 25, 2022
bkchr
approved these changes
Aug 25, 2022
| default = ["std"] | ||
| with-tracing = ["frame-executive/with-tracing"] | ||
| std = [ | ||
| "sp-sandbox/std", |
Member
There was a problem hiding this comment.
You can just remove the unused depedency, then we don't need to add it here :P
Member
Author
There was a problem hiding this comment.
There also is cargo udeps which claims to find unused crates.
I will give it a try later.
Something makes the bench regression guard fail, maybe this? Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
This was referenced Sep 8, 2022
acatangiu
approved these changes
Sep 12, 2022
This reverts commit f2cddfe. Was already fixed, only needed a CI retry. Signed-off-by: Oliver Tale-Yazdi <[email protected]>
ggwpez
added
A8-mergeoncegreen
and removed
A0-please_review
Member
Author
|
bot merge |
ark0f
pushed a commit
to gear-tech/substrate
that referenced
this pull request
Feb 27, 2023
* Add std feature Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix features Signed-off-by: Oliver Tale-Yazdi <[email protected]> * WIP Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix features Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fmt Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix features Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Cleanup Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Impl function also in tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Make compile Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix sp-trie feature Something makes the bench regression guard fail, maybe this? Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add runtime-benchmarks feature to sc-service Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Revert "Fix sp-trie feature" This reverts commit f2cddfe. Was already fixed, only needed a CI retry. Signed-off-by: Oliver Tale-Yazdi <[email protected]> Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problems & Fixes
Some crates do not compile on their own
Try
cargo c -p pallet-alliance --features runtime-benchmarks, which does not work on Substrate master.The fix is to always enable the std features of all non-optional dependencies that have a std feature.
There is no regression script provided by me but it could be done. Currently I am using scripts
to compile each crate on its own to see that they work with different feature sets.
runtime-benchmarksis enabled per defaultTry
cargo tree -e features | grep runtime-benchmarksto see which crates have the feature enabledalthough
--features runtime-benchmarkswas never provided.This comes from crates either: carelessly enabling it, or crates enabling it per default.
Fixed by checking the deps tree ensuring that it is never enabled per default.
I wrote a regression script which wraps that
cargo treecommand and can be in-cooperated into theCI in a future MR.
pallet-bags-list-fuzzer requires
runtime-benchmarksFixed by creating a dedicated
fuzzfeature which gates the required functions.Polkadot companion: paritytech/polkadot#5983
Cumulus companion: paritytech/cumulus#1607