Fixes for shellcheck warnings, part 1 #10586

cameel · 2020-12-12T06:37:35Z

~~Depends on #10877.~~ It's on develop now.

This PR fixes most of shellcheck warnings in our scripts. Those are only the ones that were not actual errors (those are covered by #10584, #10583) or just missing quotes (#10585, #10877).

These are not all fixes, just the ones that I could easily put in an independent branch. I have a few more but they depend on all the other branches so I'd like to get these here merged first. When we're done with them I'll submit the rest and another PR that removes all the scripts (now warning-free) from shellcheck ignore list. ~~EDIT: I decided to just put it on top of the other PRs and just get them all through review one by one. So this will remain a draft until all the PRs below are merged.~~ EDIT: Ready for review.

cameel · 2020-12-12T07:10:23Z

.circleci/soltest_all.sh

+        RUN_STEPS=$(seq 2 "$STEPS" | circleci tests split | xargs)
    fi
 else
-    RUN_STEPS=$(seq "$STEPS")
+    RUN_STEPS=$(seq "$STEPS" | xargs)
 fi

-# turn newlines into spaces
-RUN_STEPS=$(echo $RUN_STEPS)
-


So we ended up with #9421 (comment) anyway...

scripts/wasm-rebuild/docker-scripts/rebuild_current.sh

leonardoalt · 2021-01-19T09:03:48Z

@cameel why is it a draft?

leonardoalt · 2021-01-19T09:05:38Z

scripts/check_style.sh

-	preparedGrep "[a-zA-Z0-9_]\s*[*][a-zA-Z_]" | egrep -v -e "return [*]" -e "^* [*]" -e "^*//.*"
-) | egrep -v -e "^[a-zA-Z\./]*:[0-9]*:\s*\/(\/|\*)" -e "^test/"
+	preparedGrep "[a-zA-Z0-9_]\s*[*][a-zA-Z_]" | grep -E -v -e "return [*]" -e "^* [*]" -e "^*//.*"
+) | grep -E -v -e "^[a-zA-Z\./]*:[0-9]*:\s*\/(\/|\*)" -e "^test/"


Does this work on MacOS as well? I think we had some problems in the past because of grep -E, but might be imagining that

I haven't checked that yet. This is the reason why this and #10585 are still drafts - I need to test this stuff a bit more because we have very little coverage for shell scripts but I want to finish the bytecode compare PRs first.

This should be fine on macOS. -E is one of the options that work the same way between Linux and macOS. AFAIK -P (--perl-regexp) that we're using in get_version.sh is the problematic one.

leonardoalt · 2021-01-19T09:07:01Z

scripts/endToEndExtraction/create_traces.sh


 mkdir -p build
-cd build || exit


Why were these removed?

Because I added set -eu at the top of the script so it will fail automatically if cd fails.

cameel · 2021-01-19T10:51:51Z

@cameel why is it a draft?

Basically, I'm not yet 100% confident that this does not break anything and I want to test it manually a bit more but I'm stuck in the bytecode compare PRs at the moment and I want to finish them first.

axic · 2021-01-19T15:51:08Z

I think you could definitely merge without hesitation the /usr/bin/env and set -e changes separately.

axic · 2021-01-19T15:52:01Z

scripts/ASTImportTest.sh

                echo "Obtained:"
-                echo "$(cat ./obtained.json)"
+                cat ./obtained.json


Also this is pretty clearly not breaking anything?

scripts/check_style.sh

cameel · 2021-01-21T17:11:22Z

@axic Yeah, that sounds like a good idea. I'm going to split this PR into smaller ones and get it merged in smaller pieces to make it more manageable. The changes you mentioned will go in first.

leonardoalt · 2021-03-04T11:04:51Z

scripts/wasm-rebuild/docker-scripts/genbytecode.sh

@@ -93,6 +93,6 @@ for (var optimize of [false, true])
 }
 EOF
    chmod +x solc
-    ./solc *.sol > /tmp/report.txt
+    ./solc -- *.sol > /tmp/report.txt


If you have a file that's named like a command line option, * will match it. For example if there's a file called -rf and you do rm *, you're actually doing rm -rf * :) To avoid this the convention in many programs is that stuff after -- is never interpreted as an option. So rm -- * is safe even if you have a file called -rf.

Another way to avoid that is to use ./* instead of * because for most programs the difference between x.txt and ./x.txt does not matter. Unfortunately for solc it does (though it might change soon: #4702). I used ./* in most places that shellcheck pointed out becuase that's more universal but I could not do it here. Fortunately boost::program_options does support -- so that's the workaround I used.

scripts/wasm-rebuild/docker-scripts/rebuild_current.sh

…r do not have a shebang at all

…alents

…ere possible

…y items

… like command-line options

…leFull - This is a global variable and would be better to just make it a parameter. At least this makes shellcheck shut up

leonardoalt · 2021-03-04T16:06:20Z

Looks good, can you squash? I'll approve then

cameel · 2021-03-04T16:10:37Z

Why squash though? It's not a pile of random fixup commits :) Each commit deals with a different warning and the description says what is being fixed. It's just that there were quite a few different things to fix. It would be a jumbled mess if I squashed that so I'd rather not.

leonardoalt · 2021-03-04T16:13:35Z

Hmmm ok, it's quite a lot of commits though. I'll approve it anyway, if anyone doesn't like it blame @cameel !

cameel · 2021-03-04T16:33:08Z

Yeah, I know. But do we have a rule against that? No one is being paid by the commit anyway :) I thought that it's more about whether it appropriate for the situation.

I do see that there's a tendency towards bigger commits in Solidity so I mostly try to do that too and only have a few per PR even though personally I prefer smaller ones. Especially in PRs like this one where the fix consists of many independent parts that are better off viewed separately, I think smaller ones make more sense. There are bigger and smaller changes here and I think that without separating them the smaller ones can easily get lost in the noise.

leonardoalt · 2021-03-04T16:42:30Z

That's fine with me

cameel self-assigned this Dec 12, 2020

cameel force-pushed the shellcheck-fixes-part1 branch from de00b7e to 2d99157 Compare December 12, 2020 07:08

cameel changed the base branch from develop to fix-doc-pragma-checks December 12, 2020 07:10

cameel force-pushed the shellcheck-fixes-part1 branch from 2d99157 to d54e4f0 Compare December 12, 2020 07:15

cameel commented Dec 12, 2020

View reviewed changes

cameel marked this pull request as draft December 12, 2020 07:17

Base automatically changed from fix-doc-pragma-checks to develop December 14, 2020 10:30

axic reviewed Dec 14, 2020

View reviewed changes

scripts/wasm-rebuild/docker-scripts/rebuild_current.sh Outdated Show resolved Hide resolved

cameel mentioned this pull request Jan 15, 2021

Refactor and shellcheck cleanup in report_errors.sh and check_style.sh #10797

Merged

leonardoalt reviewed Jan 19, 2021

View reviewed changes

axic reviewed Jan 19, 2021

View reviewed changes

scripts/check_style.sh Outdated Show resolved Hide resolved

cameel mentioned this pull request Feb 2, 2021

Fix quoting in shell scripts #10585

Merged

cameel force-pushed the shellcheck-fixes-part1 branch from d54e4f0 to 5cda31d Compare February 2, 2021 13:41

cameel changed the base branch from develop to fix-quoting-and-whitespace-in-shell-scripts February 2, 2021 13:43

cameel force-pushed the shellcheck-fixes-part1 branch 3 times, most recently from c61aefa to d075ffd Compare February 2, 2021 14:10

cameel changed the base branch from fix-quoting-and-whitespace-in-shell-scripts to bash-arrays-for-arg-variables February 2, 2021 14:14

cameel force-pushed the shellcheck-fixes-part1 branch from d075ffd to cde45fc Compare February 2, 2021 14:20

cameel mentioned this pull request Feb 2, 2021

Silence remaining shellcheck warnings and remove ignore list #10879

Merged

cameel force-pushed the bash-arrays-for-arg-variables branch from 37b0f80 to cba6e68 Compare February 2, 2021 14:47

cameel force-pushed the shellcheck-fixes-part1 branch 2 times, most recently from 1585d01 to 1e5f0e5 Compare February 2, 2021 14:56

Base automatically changed from bash-arrays-for-arg-variables to develop February 8, 2021 12:31

leonardoalt reviewed Mar 4, 2021

View reviewed changes

scripts/wasm-rebuild/docker-scripts/rebuild_current.sh Outdated Show resolved Hide resolved

cameel added 10 commits March 4, 2021 16:53

Explicitly use Bash in scripts that actually use non-POSIX features o…

8c57c7c

…r do not have a shebang at all

Replace some older shell syntax with newer and/or more reliable equiv…

4c18632

…alents

Use "$@" instead of $*

1c7cc37

Declare variables separately when assigning the result of $()

75b87d1

Make scripts fail on non-zero return codes and undefined variables wh…

d14c428

…ere possible

Use read with the -r option

2784135

Don't use $ on variables inside (())

a05f3a6

Use mapfile instead of () for reading multiple lines as separate arra…

dbfd3db

…y items

Don't use negative exit codes

4e3ebda

Fix warnings about constructions that have no effect

62bf467

cameel force-pushed the shellcheck-fixes-part1 branch from 2806eb8 to 09c9591 Compare March 4, 2021 15:56

cameel added 6 commits March 4, 2021 16:57

Don't use globs in command names

7d845c0

Use glob match instead of regex match when not actually using a regex

04302d4

Prefix globs with ./ or use -- to make commands safe with files named…

e8a70e8

… like command-line options

docs_version_pragma_check.sh: Set SOLC only for the duration of compi…

87f9eba

…leFull - This is a global variable and would be better to just make it a parameter. At least this makes shellcheck shut up

Avoid using $? in conditions and do && exit 1 instead if !

c8fc241

Remove scripts that now have no warnings from shellcheck ignore list

cf94c3f

cameel force-pushed the shellcheck-fixes-part1 branch from 09c9591 to cf94c3f Compare March 4, 2021 15:58

cameel requested review from leonardoalt and axic March 4, 2021 15:58

leonardoalt approved these changes Mar 4, 2021

View reviewed changes

cameel merged commit 093ea46 into develop Mar 5, 2021

cameel deleted the shellcheck-fixes-part1 branch March 5, 2021 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for shellcheck warnings, part 1 #10586

Fixes for shellcheck warnings, part 1 #10586

cameel commented Dec 12, 2020 •

edited

Loading

cameel Dec 12, 2020

leonardoalt commented Jan 19, 2021

leonardoalt Jan 19, 2021

cameel Jan 19, 2021

cameel Feb 19, 2021

leonardoalt Jan 19, 2021

cameel Jan 19, 2021

cameel commented Jan 19, 2021 •

edited

Loading

axic commented Jan 19, 2021

axic Jan 19, 2021

cameel commented Jan 21, 2021

leonardoalt Mar 4, 2021

cameel Mar 4, 2021

leonardoalt Mar 4, 2021

leonardoalt commented Mar 4, 2021

cameel commented Mar 4, 2021

leonardoalt commented Mar 4, 2021

cameel commented Mar 4, 2021

leonardoalt commented Mar 4, 2021

Fixes for shellcheck warnings, part 1 #10586

Fixes for shellcheck warnings, part 1 #10586

Conversation

cameel commented Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

leonardoalt commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Jan 19, 2021 • edited Loading

axic commented Jan 19, 2021

Choose a reason for hiding this comment

cameel commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoalt commented Mar 4, 2021

cameel commented Mar 4, 2021

leonardoalt commented Mar 4, 2021

cameel commented Mar 4, 2021

leonardoalt commented Mar 4, 2021

cameel commented Dec 12, 2020 •

edited

Loading

cameel commented Jan 19, 2021 •

edited

Loading