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

Stylecheck script fails on MacOS #13492

Open
reubenr0d opened this issue Sep 6, 2022 · 3 comments
Open

Stylecheck script fails on MacOS #13492

reubenr0d opened this issue Sep 6, 2022 · 3 comments
Labels
bug 🐛 good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. testing 🔨

Comments

@reubenr0d
Copy link

scripts/check_style.sh does not work on MacOS:

$ scripts/check_style.sh 
grep: repetition-operator operand invalid

$ sysctl kern.version
kern.version: Darwin Kernel Version 21.2.0: Sun Nov 28 20:28:41 PST 2021; root:xnu-8019.61.5~1/RELEASE_ARM64_T6000
$ grep -V
grep (BSD grep, GNU compatible) 2.6.0-FreeBSD
@nikola-matic
Copy link
Collaborator

Eh, so it looks like this has been broken on OSX for a long time now; OSX uses FreeBSD grep, whereas Linux used GNU grep, and even though they claim GNU compatible, it's really not.

The easiest solution would be to require a brew install grep prerequisite on Macs, which will install GNU grep, but under the ggrep command, and then altering the check_style.sh script to check whether it's on Darwin, and use ggrep instead of grep.

Or, we can simply let it sit broken until we move to clang-format which ought to make the grep-ing approach redundant, but that's not likely to happen any time soon.

@nikola-matic nikola-matic added bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. labels Sep 7, 2022
@cameel cameel added the nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. label Sep 7, 2022
@reubenr0d
Copy link
Author

reubenr0d commented Sep 7, 2022

I have something that I think works, but still need to test it:

diff --git a/scripts/check_style.sh b/scripts/check_style.sh
index d1ad6bb9e..dbfbecd85 100755
--- a/scripts/check_style.sh
+++ b/scripts/check_style.sh
@@ -4,6 +4,12 @@ set -eu
 
 ERROR_LOG="$(mktemp -t check_style_XXXXXX.log)"
 
+if [ "$(uname)" == "Darwin" ]; then
+    grepCommand=ggrep
+else
+    grepCommand=grep
+fi
+
 EXCLUDE_FILES=(
     "libsolutil/picosha2.h"
     "test/cmdlineTests/strict_asm_only_cr/input.yul"
@@ -24,7 +30,7 @@ REPO_ROOT="$(dirname "$0")"/..
 cd "$REPO_ROOT" || exit 1
 
 WHITESPACE=$(git grep -n -I -E "^.*[[:space:]]+$" |
-    grep -v "test/libsolidity/ASTJSON\|test/libsolidity/ASTRecoveryTests\|test/compilationTests/zeppelin/LICENSE\|${EXCLUDE_FILES_JOINED}" || true
+    ${grepCommand} -v "test/libsolidity/ASTJSON\|test/libsolidity/ASTRecoveryTests\|test/compilationTests/zeppelin/LICENSE\|${EXCLUDE_FILES_JOINED}" || true
 )
 
 if [[ "$WHITESPACE" != "" ]]
@@ -37,13 +43,13 @@ fi
 
 function preparedGrep
 {
-    git grep -nIE "$1" -- '*.h' '*.cpp' | grep -v "${EXCLUDE_FILES_JOINED}"
+    git grep -nIE "$1" -- '*.h' '*.cpp' | ${grepCommand} -v "${EXCLUDE_FILES_JOINED}"
     return $?
 }
 
 FORMATERROR=$(
 (
-    preparedGrep "#include \"" | grep -E -v -e "license.h" -e "BuildInfo.h"  # Use include with <> characters
+    preparedGrep "#include \"" | ${grepCommand} -E -v -e "license.h" -e "BuildInfo.h"  # Use include with <> characters
     preparedGrep "\<(if|for|while|switch)\(" # no space after "if", "for", "while" or "switch"
     preparedGrep "\<for\>\s*\([^=]*\>\s:\s.*\)" # no space before range based for-loop
     preparedGrep "\<if\>\s*\(.*\)\s*\{\s*$" # "{\n" on same line as "if"
@@ -51,12 +57,12 @@ FORMATERROR=$(
     preparedGrep "[,\(<]\s*const " # const on left side of type
     preparedGrep "^\s*(static)?\s*const " # const on left side of type (beginning of line)
     preparedGrep "^ [^*]|[^*] 	|	 [^*]" # uses spaces for indentation or mixes spaces and tabs
-    preparedGrep "[a-zA-Z0-9_]\s*[&][a-zA-Z_]" | grep -E -v "return [&]" # right-aligned reference ampersand (needs to exclude return)
+    preparedGrep "[a-zA-Z0-9_]\s*[&][a-zA-Z_]" | ${grepCommand} -E -v "return [&]" # right-aligned reference ampersand (needs to exclude return)
     # right-aligned reference pointer star (needs to exclude return and comments)
-    preparedGrep "[a-zA-Z0-9_]\s*[*][a-zA-Z_]" | grep -E -v -e "return [*]" -e "^* [*]" -e "^*//.*"
+    preparedGrep "[a-zA-Z0-9_]\s*[*][a-zA-Z_]" | ${grepCommand} -E -v -e "return [*]" -e "^* [*]" -e "^*//.*"
     # unqualified move check, i.e. make sure that std::move() is used instead of move()
-    preparedGrep "move\(.+\)" | grep -v "std::move" | grep -E "[^a-z]move"
-) | grep -E -v -e "^[a-zA-Z\./]*:[0-9]*:\s*\/(\/|\*)" -e "^test/" || true
+    preparedGrep "move\(.+\)" | ${grepCommand} -v "std::move" | ${grepCommand} -E "[^a-z]move"
+) | ${grepCommand} -E -v -e "^[a-zA-Z\./]*:[0-9]*:\s*\/(\/|\*)" -e "^test/" || true
 )
 
 if [[ "$FORMATERROR" != "" ]]

Although I get some warnings when I try to run it now, need to check if they're relevant:

$scripts/check_style.sh         
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression

@NunoFilipeSantos NunoFilipeSantos added the good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. label Dec 5, 2022
hiroshitashir added a commit to hiroshitashir/solidity that referenced this issue Jun 22, 2023
- Install GNU grep on macOS with `brew install grep`
- Use GNU grep with -E option
- The following errors fixed.
```
$scripts/check_style.sh
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression
```
hiroshitashir added a commit to hiroshitashir/solidity that referenced this issue Jun 22, 2023
- Install GNU grep on macOS with `brew install grep`
- Use GNU grep with -E option
- The following errors fixed.
```
$scripts/check_style.sh
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression
```
hiroshitashir added a commit to hiroshitashir/solidity that referenced this issue Jul 14, 2023
- Install GNU grep on macOS with brew install grep
- Use GNU grep with -E option
- The following errors fixed.

```
$scripts/check_style.sh
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression
```

Fixes [Stylecheck script fails on MacOS ethereum#13492](ethereum#13492)
hiroshitashir added a commit to hiroshitashir/solidity that referenced this issue Jul 14, 2023
- Install GNU grep on macOS with brew install grep
- Use GNU grep with -E option
- The following errors fixed.

```
$scripts/check_style.sh
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression
```

Fixes [Stylecheck script fails on MacOS ethereum#13492](ethereum#13492)
@hiroshitashir
Copy link

PR at #14348

cameel pushed a commit to hiroshitashir/solidity that referenced this issue Aug 16, 2023
- Install GNU grep on macOS with brew install grep
- Use GNU grep with -E option
- The following errors fixed.

```
$scripts/check_style.sh
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression
```

Fixes [Stylecheck script fails on MacOS ethereum#13492](ethereum#13492)
hiroshitashir added a commit to hiroshitashir/solidity that referenced this issue Jan 11, 2024
- Install GNU grep on macOS with brew install grep
- Use GNU grep with -E option
- The following errors fixed.

```
$scripts/check_style.sh
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression
```

Fixes [Stylecheck script fails on MacOS ethereum#13492](ethereum#13492)
hiroshitashir added a commit to hiroshitashir/solidity that referenced this issue Jan 11, 2024
- Install GNU grep on macOS with brew install grep
- Use GNU grep with -E option
- The following errors fixed.

```
$scripts/check_style.sh
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression
```

Fixes [Stylecheck script fails on MacOS ethereum#13492](ethereum#13492)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. testing 🔨
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants