Skip to content

Fix lint errors for src/tools/rustfmt/ci/integration.sh #143512

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix lint errors for src/tools/rustfmt/ci/integration.sh #143512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 29 additions & 23 deletions src/tools/rustfmt/ci/integration.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

set -ex

: ${INTEGRATION?"The INTEGRATION environment variable must be set."}
: "${INTEGRATION?'The INTEGRATION environment variable must be set.'}"
Copy link
Member Author

Choose a reason for hiding this comment

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

In shell, simple quote strings are literals, whereas double quote strings are an equivalent of format!.

So now, variables (${}) are interpreted as strings, so if they contain whitespace characters, it might break the script. Hence why we're putting them in double quote strings so they are treated as one string.


# FIXME: this means we can get a stale cargo-fmt from a previous run.
#
Expand Down Expand Up @@ -42,8 +42,9 @@ function check_fmt_with_lib_tests {

function check_fmt_base {
local test_args="$1"
local build=$(cargo test $test_args 2>&1)
if [[ "$build" =~ "build failed" ]] || [[ "$build" =~ "test result: FAILED." ]]; then
local build
Copy link
Member Author

Choose a reason for hiding this comment

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

The lint says that we should not run a command while declaring a variable, so instead I declared the variable then assign it a value in the next line.

build=$(cargo test "$test_args" 2>&1)
if [[ "$build" =~ "build failed" ]] || [[ "$build" =~ test\ result\:\ FAILED\. ]]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

Two lints here: variables in double quote strings and the most important one is:

=~ test\ result\:\ FAILED\.

=~ means it's a regex check and the lint prefers to not have strings to ensure it doesn't have variables which could contain characters that might be interpreted as regex special characters (like the . here). So instead, we remove the quotes and escape the whitespace characters to keep it as one string and also escape characters that could be considered as regex special characters.

return 0
fi
touch rustfmt.toml
Expand All @@ -53,67 +54,72 @@ function check_fmt_base {
return 1
fi
cat rustfmt_output
! cat rustfmt_output | grep -q "internal error"
if [[ $? != 0 ]]; then
grep -q "internal error" < rustfmt_output
Copy link
Member Author

Choose a reason for hiding this comment

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

The lint was complaining about the cat redirection which could be replaced by < rustfmt_output. And then I removed the ! operator and reverted the checked value (from != 0 to == 0). Less tricky to understand (if the command didn't fail, ie grep found the string, then not good). ^^'

cmd_ret=$?
Copy link
Member Author

Choose a reason for hiding this comment

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

Lint was complaining that we were using $? and that it could be problematic since the last run command might not be the one expected. So instead, we store the value in a variable and then use this variable.

if [[ $cmd_ret == 0 ]]; then
return 1
fi
! cat rustfmt_output | grep -q "warning"
if [[ $? != 0 ]]; then
grep -q "warning" < rustfmt_output
cmd_ret=$?
if [[ $cmd_ret == 0 ]]; then
return 1
fi
! cat rustfmt_output | grep -q "Warning"
if [[ $? != 0 ]]; then
grep -q "Warning" < rustfmt_output
cmd_ret=$?
if [[ $cmd_ret == 0 ]]; then
return 1
fi
cargo fmt --all -- --check |& tee rustfmt_check_output
if [[ ${PIPESTATUS[0]} != 0 ]]; then
cat rustfmt_check_output
return 1
fi
cargo test $test_args
if [[ $? != 0 ]]; then
return $?
cargo test "$test_args"
cargo_ret=$?
if [[ $cargo_ret != 0 ]]; then
return $cargo_ret
fi
}

function show_head {
local head=$(git rev-parse HEAD)
local head
head=$(git rev-parse HEAD)
echo "Head commit of ${INTEGRATION}: $head"
}

case ${INTEGRATION} in
cargo)
git clone --depth=1 https://github.com/rust-lang/${INTEGRATION}.git
cd ${INTEGRATION}
git clone --depth=1 "https://github.com/rust-lang/${INTEGRATION}.git"
cd "${INTEGRATION}"
show_head
export CFG_DISABLE_CROSS_TESTS=1
check_fmt_with_all_tests
cd -
;;
crater)
git clone --depth=1 https://github.com/rust-lang/${INTEGRATION}.git
cd ${INTEGRATION}
git clone --depth=1 "https://github.com/rust-lang/${INTEGRATION}.git"
cd "${INTEGRATION}"
show_head
check_fmt_with_lib_tests
cd -
;;
bitflags)
git clone --depth=1 https://github.com/bitflags/${INTEGRATION}.git
cd ${INTEGRATION}
git clone --depth=1 "https://github.com/bitflags/${INTEGRATION}.git"
cd "${INTEGRATION}"
show_head
check_fmt_with_all_tests
cd -
;;
tempdir)
git clone --depth=1 https://github.com/rust-lang-deprecated/${INTEGRATION}.git
cd ${INTEGRATION}
git clone --depth=1 "https://github.com/rust-lang-deprecated/${INTEGRATION}.git"
cd "${INTEGRATION}"
show_head
check_fmt_with_all_tests
cd -
;;
*)
git clone --depth=1 https://github.com/rust-lang/${INTEGRATION}.git
cd ${INTEGRATION}
git clone --depth=1 "https://github.com/rust-lang/${INTEGRATION}.git"
cd "${INTEGRATION}"
show_head
check_fmt_with_all_tests
cd -
Expand Down
Loading