Check Rust feature configs #964
Conversation
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]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #964 +/- ##
=======================================
Coverage 85.32% 85.32%
=======================================
Files 88 88
Lines 10476 10476
=======================================
Hits 8939 8939
Misses 1537 1537 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
Sure, thank you. However, I am currently waiting for my new laptop, so I will be unavailable for a brief period. |
Okay nice. I am waiting for @xlc for some feedback on the approach. |
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Makefile
Outdated
| @@ -34,9 +34,11 @@ Cargo.toml: Cargo.dev.toml | |||
|
|
|||
| dev-format: Cargo.toml | |||
| cargo fmt --all | |||
| zepter format features --fix | |||
There was a problem hiding this comment.
maybe have them on a separate target otherwise we need to indicate on README to tell user to install zepter first
but a good amount of time people won't need to use it so I want to make it optional
There was a problem hiding this comment.
Okay I put it into another target. LMK if i overdid it 😅
.github/workflows/test.yml
Outdated
| @@ -29,6 +29,8 @@ jobs: | |||
| uses: dtolnay/rust-toolchain@nightly | |||
| - name: Install Wasm toolchain | |||
| run: rustup target add wasm32-unknown-unknown | |||
| - name: Install Zepter | |||
| run: cargo install zepter --version 0.14.0 --locked -q -f --no-default-features && zepter --version | |||
There was a problem hiding this comment.
doesn't need to happen in this PR but at some stage I want to use https://github.com/taiki-e/install-action (powered by https://github.com/cargo-bins/cargo-binstall) to install tools. I don't know if zepter is compatible with it but we could try and see
There was a problem hiding this comment.
Yea i wanted to use cargo-install as well. I think it just pulls binaries from the the GH release page. Will explore it and then open another MR when its done.
.github/workflows/test.yml
Outdated
| @@ -41,3 +43,5 @@ jobs: | |||
| run: make dev-check | |||
| - name: Run tests | |||
| run: make dev-test | |||
| - name: Check Rust features | |||
| run: zepter run check | |||
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
| "frame-support/try-runtime", | ||
| "frame-system/try-runtime", | ||
| "orml-tokens/try-runtime", |
There was a problem hiding this comment.
this is causing issues when I try to publish this to crates.io using cargo release. orml-tokens is only a dev-dependencies so I think there is no reason to enable features for it?
There was a problem hiding this comment.
Hm, maybe it should be an optional feature enable then like orml-tokens?/try-runtime. Since when the feature is not propagated some of the tests could error i think.
I will try to debug it.
There was a problem hiding this comment.
No the optional is not possible to compile then. I think in the past, Parity stripped dev-dependencies before publishing from the Cargo.toml files. Not sure if this still happens.
PS: Yea i think it still re-writes them https://github.com/paritytech/parity-publish/blob/1db5cd6056f76ebc8bb202931589397ce5b2cd36/src/apply.rs#L86
Maybe i can make a Zepter command to quickly strip all the dev stuff out.
There was a problem hiding this comment.
I tried and not working. I believe it is cargo release removes dev-dependencies before publish and all those features will not be valid.
Besides, we don't really need to enable them for dev-dependencies anyway. If the tests requires the feature, we simply import them with feature enabled in dev-dependencies directly.
There was a problem hiding this comment.
There is a first version of this here ggwpez/zepter#56 for zepter transpose features strip-dev-only.
I will write some more tests tomorrow and fix a few edge-cases but it seems to work when i tested it with cargo publish --dry-run on the crates here.
|
Currently, cargo-featalign requires a |
Adds the Zepter CLI to the CI to verify feature integrity of all Rust crates. It uses (nearly) the same config file as the Polkadot-SDK.
cc @AurevoirXavier maybe you also want to add your tool?
What it does:
try-runtime,runtime-benchmarksandstdis forwarded to dependencies.Bhas a feature that crateAalso must have that feature (we skipped this in the SDK, but could be enabled).