Fix Substrate features

[ggwpez]

Trying to automatically fix std, runtime-benchmarks and try-runtime features with

zepter lint propagate-feature --feature try-runtime --left-side-feature-missing=ignore --workspace --fix --feature-enables-dep="try-runtime:frame-try-runtime"
zepter lint propagate-feature --feature runtime-benchmarks --left-side-feature-missing=ignore --workspace --fix --feature-enables-dep="runtime-benchmarks:frame-benchmarking"
zepter lint propagate-feature --feature std --left-side-feature-missing=ignore --workspace --fix

The auto-fixer tries to be as non-invasive as possible, but removes empty lines.

Explanation:

• This adds the feature for all dependencies that expose it. It does not create the feature if it is not present, since left-side-feature-missing is passed. To be really correct I think we would have to add it to all crates that do not have it though.
• The feature-enables-dep will enable the given dependency as non-optional when the feature is enabled. Otherwise the features of optional dependencies will also be enabled as optional.

Known blindspot: The --workspace flag currently requires that both sides are in the workspace. This will be extended soon to also allow the right-hand-side dependency to be outside of the worspace. Since currently it does not detect serde/std as missing when the --workspace flag is present.

TODO:

• Double check that it also works fine for dev-dependencies. I am not sure what rules to use here.
• Add CI
• Add other featalign to double-check (will maybe do as follow up so we already have something working).

[shawntabrizi]

Could we use Zepter to a little bit of toml formatting too?

For example organizing dependencies alphabetically?

[ggwpez]

Could we use Zepter to a little bit of toml formatting too?

For example organizing dependencies alphabetically?

Yes we definitely can. Currently the autofixer is written to not re-order anything, but it already has to touch the toml array, so it could be ordered as well.
Comments within the dependency array are lost, but i think that is a fine trade-off in this case.

[koute]

Comments within the dependency array are lost, but i think that is a fine trade-off in this case.

Could we make it so that they're not lost? (I assume there are toml libraries which can do this?)

[ggwpez]

Comments within the dependency array are lost, but i think that is a fine trade-off in this case.

Could we make it so that they're not lost? (I assume there are toml libraries which can do this?)

Oh, the official toml_edit crate actually mentions that it can preserve comment - so I must just be using it wrong. I will look into it, thanks!

[bkchr]

Looks good besides the edge case around enabling of a crate and a feature in one go that I highlighted in the review

[ggwpez]

bot merge

[AurevoirXavier]

cargo-featalign can address many of the situations mentioned in this thread such as preseving comments. Recently, I added support for sorting features alphabetically based on @shawntabrizi's comment.

However, I am unsure about how parity CI functions. I am eager to apply this tool to Substrate. cc @ggwpez

[ggwpez]

@AurevoirXavier you can check out the diff in scripts/ci/gitlab/pipeline/check.yml of this MR.
You can just add featalign alongside the Zepter invocation there into the same job. I am not a CI engie myself, but it should be fine.
The job currently takes 33 sec, it runs as one of the first jobs, but AFAIK it does not block later ones.

[shawntabrizi]

it would be great if these feature fix stuff was somehow integrated into the node-template, cumulus, and similar projects.

Then users would run something like ./scripts/fix-features.sh in their custom project and make sure they are doing everything right.