-
Notifications
You must be signed in to change notification settings - Fork 3
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
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f443985
Add rust-features.sh
ggwpez 6aee520
Use `env bash`
ggwpez 3e61c10
Remove //
ggwpez 277acd7
Simplify script
ggwpez 6055941
Use main function
ggwpez e5adbdb
Add newline at EOF
ggwpez 7409082
Put command directly in 'if'
ggwpez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
#!/usr/bin/env bash | ||
|
||
############################################################################## | ||
# | ||
# This script checks that crates to not carelessly enable features that | ||
# should stay disabled. It's important to check that since features | ||
# are used to gate specific functionality which should only be enabled | ||
# when the feature is explicitly enabled. | ||
# | ||
# Invocation scheme: | ||
# ./rust-features.sh <CARGO-ROOT-PATH> | ||
# | ||
# Example: | ||
# ./rust-features.sh path/to/substrate | ||
# | ||
# The steps of this script: | ||
# 1. Check that all required dependencies are installed. | ||
# 2. Check that all rules are fullfilled for the whole workspace. If not: | ||
# 4. Check all crates to find the offending ones. | ||
# 5. Print all offending crates and exit with code 1. | ||
# | ||
############################################################################## | ||
|
||
set -eu | ||
|
||
# Check that cargo and grep are installed - otherwise abort. | ||
command -v cargo >/dev/null 2>&1 || { echo >&2 "cargo is required but not installed. Aborting."; exit 1; } | ||
command -v grep >/dev/null 2>&1 || { echo >&2 "grep is required but not installed. Aborting."; exit 1; } | ||
|
||
# Enter the workspace root folder. | ||
cd "$1" | ||
echo "Workspace root is $PWD" | ||
|
||
function main() { | ||
feature_does_not_imply 'default' 'runtime-benchmarks' | ||
feature_does_not_imply 'std' 'runtime-benchmarks' | ||
feature_does_not_imply 'default' 'try-runtime' | ||
feature_does_not_imply 'std' 'try-runtime' | ||
} | ||
|
||
# Accepts two feature names as arguments. | ||
# Checks that the first feature does not imply the second one. | ||
function feature_does_not_imply() { | ||
ENABLED=$1 | ||
STAYS_DISABLED=$2 | ||
echo "📏 Checking that $ENABLED does not imply $STAYS_DISABLED ..." | ||
|
||
# Check if the forbidden feature is enabled anywhere in the workspace. | ||
if cargo tree --no-default-features --locked --workspace -e features --features "$ENABLED" | grep -qF "feature \"$STAYS_DISABLED\""; then | ||
echo "❌ $ENABLED implies $STAYS_DISABLED in the workspace" | ||
else | ||
echo "✅ $ENABLED does not imply $STAYS_DISABLED in the workspace" | ||
return | ||
fi | ||
|
||
# Find all Cargo.toml files but exclude the root one since we already know that it is broken. | ||
CARGOS=`find . -name Cargo.toml -not -path ./Cargo.toml` | ||
NUM_CRATES=`echo "$CARGOS" | wc -l` | ||
FAILED=0 | ||
PASSED=0 | ||
echo "🔍 Checking all $NUM_CRATES crates - this takes some time." | ||
|
||
for CARGO in $CARGOS; do | ||
OUTPUT=$(cargo tree --no-default-features --locked --offline -e features --features $ENABLED --manifest-path $CARGO 2>&1 || true) | ||
|
||
if echo "$OUTPUT" | grep -qF "not supported for packages in this workspace"; then | ||
# 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 | ||
echo "❌ Violation in $CARGO by dependency:" | ||
# Best effort hint for which dependency needs to be fixed. | ||
echo "$OUTPUT" | grep -wF "feature \"$STAYS_DISABLED\"" | head -n 1 | ||
FAILED=$((FAILED+1)) | ||
else | ||
PASSED=$((PASSED+1)) | ||
fi | ||
done | ||
|
||
echo "Checked $NUM_CRATES crates in total of which $FAILED failed and $PASSED passed." | ||
echo "Exiting with code 1" | ||
exit 1 | ||
} | ||
|
||
main "$@" |
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.
There was a problem hiding this comment.
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
, thenRET=0; command || RET=$?
, andIS_NOT_SUPPORTED=$(command || echo $?)
.Maybe put all of them into
if
's?There was a problem hiding this comment.
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 👍