Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent Rust feature bleed #81

Merged
merged 7 commits into from
Oct 12, 2022
Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Oct 6, 2022

See paritytech/substrate#12341 for a description of the new script.

Currently working in Substrate: success case and error case.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
rust-features.sh Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Oct 7, 2022

@mordamax you are fine with maintaining this? My eyes bleed again a little bit after seeing this bash script.

@bkchr
Copy link
Member

bkchr commented Oct 7, 2022

BTW, there also exist this pr: paritytech/diener#19

@mordamax
Copy link
Contributor

mordamax commented Oct 7, 2022

@mordamax you are fine with maintaining this? My eyes bleed again a little bit after seeing this bash script.

I guess it's @paritytech-ci who maintain it in this case.
This repo is mainly for reusable CI scripts, to avoid dupe

but my opinion is that it's okay as long as it's relevantly simple, <=~100 lines, otherwise it should be some more maintainable/readable language like ts/python/rust

the existing stuff like check-dependent should be killed 100%
tryruntime and bench most probably will be moved & rewritten to command bot

Co-authored-by: Bastian Köcher <info@kchr.de>
@ggwpez
Copy link
Member Author

ggwpez commented Oct 7, 2022

BTW, there also exist this pr: paritytech/diener#19

Interesting... I dont think parsing Cargo.toml files works for external dependencies from crates.io, or? (In case we ever release Substrate there 🤣) But in any case, that check is additive to this one.

I am not happy with this as long-term solution, but for now it should prevent obvious blunders which can take a while to find otherwise. Recently I saw https://github.com/guppy-rs/guppy which is used by Diem and think could be a fit.

@bkchr
Copy link
Member

bkchr commented Oct 7, 2022

I dont think parsing Cargo.toml files works for external dependencies from crates.io, or

You download the deps to the local cargo registry. So, this should work.

Recently I saw https://github.com/guppy-rs/guppy which is used by Diem and think could be a fit.

I think I read about this somewhere before, but if that could be a fit, it would be nice.

@ggwpez
Copy link
Member Author

ggwpez commented Oct 7, 2022

Okay then lets use this to get rid of some obvious errors and then make a long-term strategy?
Like set out a scope of rules that we want to be checked and search for existing tools if they can do it or self implement.

rust-features.sh Outdated Show resolved Hide resolved
rust-features.sh Outdated Show resolved Hide resolved
rust-features.sh Outdated Show resolved Hide resolved
rust-features.sh Outdated Show resolved Hide resolved
rust-features.sh Outdated Show resolved Hide resolved
rust-features.sh Outdated Show resolved Hide resolved
rust-features.sh Outdated Show resolved Hide resolved
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
# This case just means that the pallet does not support the
# requested feature which is fine.
PASSED=$((PASSED+1))
elif echo "$OUTPUT" | grep -qF "feature \"$STAYS_DISABLED\""; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see three different approaches to checking return code here.
This one, inside if, then RET=0; command || RET=$?, and IS_NOT_SUPPORTED=$(command || echo $?).
Maybe put all of them into if's?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of wanted to keep the lines short, but I guess I can break it into multiple like this as well 👍

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
rust-features.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@mutantcornholio mutantcornholio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyway, much more readable and maintainable now, thank you!

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@mutantcornholio mutantcornholio merged commit 7e40257 into master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants