Fix lint errors for src/tools/rustfmt/ci/integration.sh
#143512
Conversation
rustbot
added
the
S-waiting-on-review
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
|
I am not at all well-versed at bash and thus I am not very confident in reviewing such changes :( I would be just piping the PR diff into ChatGPT to even understand it, but that's not a very good reviewing strategy for such potentially subtle changes. |
|
In this case, I can walk you through them. :) |
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| set -ex | |||
|
|
|||
| : ${INTEGRATION?"The INTEGRATION environment variable must be set."} | |||
| : "${INTEGRATION?'The INTEGRATION environment variable must be set.'}" | |||
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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.
| if [[ "$build" =~ "build failed" ]] || [[ "$build" =~ "test result: FAILED." ]]; then | ||
| local build | ||
| build=$(cargo test "$test_args" 2>&1) | ||
| if [[ "$build" =~ "build failed" ]] || [[ "$build" =~ test\ result\:\ FAILED\. ]]; then |
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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). ^^'
| ! cat rustfmt_output | grep -q "internal error" | ||
| if [[ $? != 0 ]]; then | ||
| grep -q "internal error" < rustfmt_output | ||
| cmd_ret=$? |
There was a problem hiding this comment.
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.
|
And with these comments, I covered all lint cases. :) If you still don't think you're the right person to review it, don't hesitate to assign someone else. |
|
@GuillaumeGomez can you close this and open a PR directly in https://github.com/rust-lang/rustfmt |
|
Sure. |
rustbot
removed
the
S-waiting-on-review
I think I'm going through shell lints errors one file at a time. Will make review much easier and hopefully will prevent me from adding some unwanted side effets.
r? @Kobzol