chore: remove unused deps and add missing feats for all tomls #1223

wischli · 2023-03-02T20:11:37Z

Description

Result of fixing try-runtime in #1198 required for migration of pallet-block-rewards.

Removes unused deps with help of cargo-machete (though this tools creates many false positives, especially for optional deps, codec and scale-info)
Adds missing features std, runtime-benchmarks and try-runtime for all crates
- Enables isolated compiling of any of our crates.
Adds try-runtime to development-runtime required in feat: impl block rewards #1198
Fixes try-runtime which wasn't working since try-runtime was made much stricter sometime between Polkadot v0.9.28 and v0.9.32. Basically, each of our pallets requires at least having

try-runtime = [ "frame-support/try-runtime" ]

See Shawns comment for details.
Normalizes apostrophes used in tomls to " instead of mixed '/"
Adds patch to PureStake/Frontier, thanks @NunoAlexandre !

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

# Find all child directories containing Cargo.toml files
dirs=$(find . -name Cargo.toml -print0 | xargs -0 -n1 dirname | sort -u)

# Execute the command "subalfred check" on each directory
for dir in $dirs; do
    subalfred check features $dir
done

# Check unused deps
# NOTE: creates false positives
cargo-machete
# cargo-machete --fix

cargo run --release --features=try-runtime try-runtime --chain=development --no-spec-check-panic --execution=native on-runtime-upgrade live --uri=wss://fullnode.parachain.centrifuge.io:443

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I rebased on the latest main branch

lemunozm

LGTM! Awesome work @wischli! 🙌🏻

mustermeiszer

Great clean up, love it!!

Just one minor question regarding a change in the lock.

The other comments are just nit picks that can also be cleaned up on the same run.

But see no blockers.

mustermeiszer · 2023-03-03T09:00:58Z

pallets/investments/Cargo.toml

+authors = ["Substrate DevHub <https://github.com/substrate-developer-hub>"]
+description = "FRAME pallet template for defining custom runtime logic."
+edition = "2018"
+homepage = "https://substrate.dev"
+license = "Unlicense"
+name = "pallet-investments"
+readme = "README.md"
+repository = "https://github.com/substrate-developer-hub/substrate-node-template/"


We should tackle this in a follow up and actually insert the right info. My technical debt, sorry.

mustermeiszer · 2023-03-03T09:04:03Z

runtime/development/Cargo.toml

  "pallet-preimage/try-runtime",
  "pallet-proxy/try-runtime",
  "pallet-randomness-collective-flip/try-runtime",
  "pallet-restricted-tokens/try-runtime",
  "pallet-scheduler/try-runtime",
  "pallet-session/try-runtime",
-  "pallet-society/try-runtime",


Why do we even have pallet-staking as a deps?

Good point. I will run cargo-machete an all crates and remove unused deps!

mustermeiszer · 2023-03-03T09:04:33Z

taplo.toml

@@ -22,7 +22,7 @@ column_width = 160
 # Indent based on tables and arrays of tables and their subtables, subtables out of order are not indented.
 indent_tables = false
 # The substring that is used for indentation, should be tabs or spaces (but technically can be anything).
-indent_string = '  '
+indent_string = "  "


Unfortunately, this does not change anything. There is no taplo enforcement on the type of apostrophe used 😓

mustermeiszer · 2023-03-03T09:11:22Z

runtime/centrifuge/Cargo.toml

+  "orml-asset-registry/runtime-benchmarks",
+  "orml-tokens/runtime-benchmarks",
+  "orml-xtokens/runtime-benchmarks",
+  "pallet-babe/runtime-benchmarks",


Babe and grandpa can also be removerd

pallets/connectors/Cargo.toml

mustermeiszer · 2023-03-03T09:15:34Z

Cargo.lock

@@ -13132,7 +13134,7 @@ version = "1.6.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "97fee6b57c6a41524a810daee9286c02d7752c4253064d0b05472833a438f675"
 dependencies = [
- "cfg-if 1.0.0",
+ "cfg-if 0.1.10",


Do you know what caused the downgrade of the version?

Very likely due to the patching for PureStake/frontier. Should be reverted once I remove the patch.

Resolved after removing the patch as expected.

mustermeiszer

Thanks for changing Investments cargo already ❤️

Reverting patch looks good. Good to go from my side

NunoAlexandre

Awesome sauce 🌶️

NunoAlexandre

Re-approving

mustermeiszer · 2023-03-03T13:51:04Z

Re-approve

wischli added 5 commits March 2, 2023 14:36

fix: add missing try-runtime feat to all tomls

0ec9bbb

fix: add missing std, bench feats to all tomls

d787ebf

fix: patch PureStake/Frontier

f9165a2

chore: bump nix

8084efd

refactor: normalize apostrophes in toml

5fedbe7

wischli requested review from mustermeiszer, NunoAlexandre, branan, mikiquantum, hieronx, cdamian and lemunozm as code owners March 2, 2023 20:11

wischli added 3 commits March 2, 2023 21:13

Merge branch 'main' into chore/add-missing-toml-feats

f25b37b

fmt: taplo

53336bc

fix: typo

53e41a2

wischli added D0-ready Pull request can be merged without special precaution and notification. I6-refactoring Code needs refactoring. labels Mar 2, 2023

lemunozm previously approved these changes Mar 3, 2023

View reviewed changes

This was referenced Mar 3, 2023

Add mock-builder to the repo with pools & permissions instances #1220

Merged

feat: impl block rewards #1198

Merged

mustermeiszer previously approved these changes Mar 3, 2023

View reviewed changes

wischli added 3 commits March 3, 2023 10:31

revert: frontier patch

7c40fb2

refactor: remove unused deps

d554430

fix: investment toml boilerplate

9761390

wischli dismissed stale reviews from mustermeiszer and lemunozm via 9761390 March 3, 2023 12:59

wischli requested review from lemunozm and mustermeiszer March 3, 2023 13:01

wischli changed the title ~~chore: add missing features in all tomls~~ chore: remove unused deps and add missing feats for all tomls Mar 3, 2023

refactor: fix uncaught stuff by taplo

df24c90

mustermeiszer previously approved these changes Mar 3, 2023

View reviewed changes

wischli added 2 commits March 3, 2023 14:31

Merge remote-tracking branch 'origin' into chore/add-missing-toml-feats

3dfcee5

chore: bump nix after merge

887de3f

NunoAlexandre previously approved these changes Mar 3, 2023

View reviewed changes

wischli dismissed stale reviews from NunoAlexandre and mustermeiszer via 887de3f March 3, 2023 13:41

NunoAlexandre approved these changes Mar 3, 2023

View reviewed changes

mustermeiszer approved these changes Mar 3, 2023

View reviewed changes

cdamian approved these changes Mar 3, 2023

View reviewed changes

lemunozm approved these changes Mar 3, 2023

View reviewed changes

hieronx approved these changes Mar 3, 2023

View reviewed changes

mikiquantum approved these changes Mar 3, 2023

View reviewed changes

mikiquantum merged commit c291287 into main Mar 3, 2023

mikiquantum deleted the chore/add-missing-toml-feats branch March 3, 2023 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove unused deps and add missing feats for all tomls #1223

chore: remove unused deps and add missing feats for all tomls #1223

wischli commented Mar 2, 2023 •

edited

Loading

lemunozm left a comment

mustermeiszer left a comment

mustermeiszer Mar 3, 2023

wischli Mar 3, 2023

mustermeiszer Mar 3, 2023

wischli Mar 3, 2023

wischli Mar 3, 2023

mustermeiszer Mar 3, 2023

wischli Mar 3, 2023

mustermeiszer Mar 3, 2023

wischli Mar 3, 2023

mustermeiszer Mar 3, 2023

wischli Mar 3, 2023

wischli Mar 3, 2023

mustermeiszer left a comment

NunoAlexandre left a comment

NunoAlexandre left a comment

mustermeiszer commented Mar 3, 2023

chore: remove unused deps and add missing feats for all tomls #1223

chore: remove unused deps and add missing feats for all tomls #1223

Conversation

wischli commented Mar 2, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

lemunozm left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

mustermeiszer commented Mar 3, 2023

wischli commented Mar 2, 2023 •

edited

Loading