View Issue Details

IDProjectCategoryView StatusLast Update
0002983OpenFOAMPatchpublic2018-06-17 20:45
Reporterwyldckat Assigned Tohenry  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformGNU/LinuxOSUbuntuOS Version16.04
Product Versiondev 
Fixed in Versiondev 
Summary0002983: Patching a couple of bugs introduced with shellcheck in commit 0cca22576295
DescriptionThe bugs introduced in commit 0cca22576295ee1b1aa55e10dbdf42cf084025ca are as follows:

 1- `wmake -j all` does not work in OpenFOAM-dev, because the parsing code '(($2 + 1))' assumes that "all1" is valid, when it's not, since it should be checking if it's a number or not.

 2. Wrapping with quotes all of the calls to 'find "$objectsDir"...' in 'wmake/wrmdep' will fail when running in the files-mode with the option '-all', because it replaces the current platform name for an asterisk, which results in not expanding the asterisk when using it in quotes with the 'find' application.

 3. Last but not least, spellcheck did not catch the C-like if-statements for how "$WM_NCOMPPROCS" and "$MAKEFLAGS" were used.

   - For example, `[ "$WM_NCOMPPROCS" ]` will also evaluate statements defined in the environment variable, which could be considered an attack vector or at least some fairly weird error messages.
     Try exporting 'WM_NCOMPPROCS=-z' and you'll get an error message that '-z' was missing an argument.



The attach 'proposition_v1' (tarball and patch) provides the following files and respective fixes:

 - wmake/wmake

     - Go back to the old checking method for the number of cores next to '-j'.

     - Properly check contents of the "$WM_NCOMPPROCS" and "$MAKEFLAGS" variables.


 - wmake/wrmdep

     - Properly handle searches for .dep files for either the current platform and all platforms, while still abiding to the quoted variables that spellcheck proposed.

     Note: the change is rather cumbersome, but it's the cleanest way I could think of to do this. The other solution would be to revert to the old mechanism, but that would result in not safely handling the then unquoted variable (e.g. to support paths with spaces and non-conventional characters).
TagsNo tags attached.

Activities

wyldckat

2018-06-17 17:09

updater  

proposition_v1.tar.gz (5,881 bytes)

wyldckat

2018-06-17 17:09

updater  

proposition_v1.patch (2,831 bytes)   
diff --git a/wmake/wmake b/wmake/wmake
index 33c9de9..1ed18b7 100755
--- a/wmake/wmake
+++ b/wmake/wmake
@@ -153,7 +153,7 @@ do
         # Parallel compilation on all cores of local machine
         -j)
             useAllCores
-            test $# -ge 2 && (($2 + 1)) > /dev/null 2>&1 \
+            test $# -ge 2 && expr $2 + 1 > /dev/null 2>&1 \
                 && shift && export WM_NCOMPPROCS=$1
             echo "Compiling enabled on $WM_NCOMPPROCS cores"
             ;;
@@ -222,11 +222,11 @@ then
     WM_NCOMPPROCS=$(wmakeScheduler -count) || unset WM_NCOMPPROCS
 fi
 
-if [ "$WM_NCOMPPROCS" ]
+if [ -n "$WM_NCOMPPROCS" ]
 then
     parOpt="-j $WM_NCOMPPROCS"
 
-    if [ "$WM_NCOMPPROCS" -gt 1 ] && [ ! "$MAKEFLAGS" ]
+    if [ "$WM_NCOMPPROCS" -gt 1 ] && [ -z "$MAKEFLAGS" ]
     then
         lockDir=$HOME/.$WM_PROJECT/.wmake
 
diff --git a/wmake/wrmdep b/wmake/wrmdep
index 644fab0..70154f5 100755
--- a/wmake/wrmdep
+++ b/wmake/wrmdep
@@ -92,6 +92,18 @@ error() {
     exit 1
 }
 
+removeDepFiles()
+{
+    if [ "$#" -eq 1 ]
+    then
+        find "$1" -name '*.dep' -print0 | xargs -0t rm 2>/dev/null
+    elif [ "$#" -eq 2 ]
+    then
+        find "$1" -name '*.dep' -exec grep -q "$2" '{}' \; \
+             -exec rm '{}' \; -print
+    fi
+}
+
 #------------------------------------------------------------------------------
 # Parse arguments and options
 #------------------------------------------------------------------------------
@@ -144,20 +156,37 @@ files)
 
     findObjectDir .
 
-    # With the -a/-all option replace the current platform with a wildcard
-    if [ "$all" = "all" ]
-    then
-        objectsDir=$(echo "$objectsDir" | sed s%"$WM_OPTIONS"%*% )
-    fi
-
+    # First give the verbose information for the current searching method
     if [ "$#" -eq 0 ]
     then
         echo "removing all .dep files ..."
-        find "$objectsDir" -name '*.dep' -print0 | xargs -0t rm 2>/dev/null
     else
         echo "removing .dep files referring to $1 ..."
-        find "$objectsDir" -name '*.dep' -exec grep -q "$1" '{}' \; \
-             -exec rm '{}' \; -print
+    fi
+
+    # With the -a/-all option replace the current platform with a wildcard
+    if [ "$all" = "all" ]
+    then
+
+        # Handle the removal on all platforms for consistency
+        mainPath=${objectsDir%%/$WM_OPTIONS*}
+        objSubPath=${objectsDir##*$WM_OPTIONS/}
+
+        for pathEntry in "$mainPath"/*
+        do
+            if [ -e "$pathEntry/$objSubPath" ]
+            then
+                # Note:
+                # If $1 is empty, it should still work for the default method
+                removeDepFiles "$pathEntry/$objSubPath" $1
+            fi
+        done
+
+    else
+
+        # Handle only the removal for the current WM_OPTIONS
+        removeDepFiles $@
+
     fi
 
     ;;
proposition_v1.patch (2,831 bytes)   

henry

2018-06-17 17:20

manager   ~0009780

> The other solution would be to revert to the old mechanism, but that would result in not safely handling the then unquoted variable (e.g. to support paths with spaces and non-conventional characters).

If this would be simpler and easier to maintain I do not have a problem with it as spaces in paths is not supported by OpenFOAM anyway.

I found that using shellcheck on the wmake scripts caused a lot of problems for little benefit and we could revert these changes altogether if the old scripts are easier to maintain.

wyldckat

2018-06-17 17:41

updater   ~0009781

Most of the changes proposed by spellcheck are part of good shell-scripting practices... but after reviewing the commit once again, there are only some 3 or 4 lines of changes that I would keep.

...and the bash'isms still look weird to me... I've gotten too familiar with using the simpler shell evaluation convention, rather than using multiple brackets and parenthesis...

henry

2018-06-17 17:59

manager   ~0009782

Right now I don't have time to re-do the wmake update from scratch keeping only some of the changes and putting all the messy switches in to stop shellcheck complaining about the others, it took me a whole day last time. So either I revert all the changes, apply your patch as-is or if you supply a simpler patch which "The other solution would be to revert to the old mechanism" where appropriate I could apply that. I think the last option would be best right now if you have time, otherwise I can apply the current patch.

wyldckat

2018-06-17 18:20

updater   ~0009783

Then please apply the attached patch as-is.

Given the effort already put into this, there are several safety measures that the spellcheck helped put in which would best to be kept. The old way may look easier to maintain, but e.g. leaving the variables unquoted is always a risk, even if it doesn't look like it.
Furthermore, this way spellcheck can be re-used in the future to assist with maintenance.


By the way, where exactly is the spellcheck repository/website? Searching online for something along the lines of 'spellcheck bash script checking' always leads me to software for checking command line grammar checking rather than script code.

henry

2018-06-17 18:33

manager   ~0009784

On OpenSuSE Tumbleweed I used

zypper install shellcheck

and I think it is available on Ubuntu using apt-get.

The web-site is https://www.shellcheck.net/

If you need the source code: https://github.com/koalaman/shellcheck

wyldckat

2018-06-17 19:16

updater   ~0009785

Many thanks and my apologies because I somehow memorized "spellcheck" instead of "shellcheck"...

I've used it now on Ubuntu and it looks like I made a mistake in the changes I made to wrmdep. I'll have another revision in a few minutes.

wyldckat

2018-06-17 20:01

updater  

proposition_v2.patch (4,035 bytes)   
diff --git a/wmake/wclean b/wmake/wclean
index f8d475a..9a96360 100755
--- a/wmake/wclean
+++ b/wmake/wclean
@@ -38,7 +38,7 @@ Script=${0##*/}
 
 # Source the wmake functions
 # shellcheck source=scripts/wmakeFunctions
-. ${0%/*}/scripts/wmakeFunctions
+. "${0%/*}/scripts/wmakeFunctions"
 
 error() {
     while [ "$#" -ge 1 ]; do echo "$1"; shift; done
diff --git a/wmake/wdep b/wmake/wdep
index 64db3f6..6dd0ad2 100755
--- a/wmake/wdep
+++ b/wmake/wdep
@@ -39,7 +39,7 @@ Script=${0##*/}
 
 # Source the wmake functions
 # shellcheck source=scripts/wmakeFunctions
-. ${0%/*}/scripts/wmakeFunctions
+. "${0%/*}/scripts/wmakeFunctions"
 
 usage() {
     cat<<USAGE
diff --git a/wmake/wmake b/wmake/wmake
index 33c9de9..8dd8cdb 100755
--- a/wmake/wmake
+++ b/wmake/wmake
@@ -153,7 +153,7 @@ do
         # Parallel compilation on all cores of local machine
         -j)
             useAllCores
-            test $# -ge 2 && (($2 + 1)) > /dev/null 2>&1 \
+            [ $# -ge 2 ] && [ "$2" -gt 0 ] > /dev/null 2>&1 \
                 && shift && export WM_NCOMPPROCS=$1
             echo "Compiling enabled on $WM_NCOMPPROCS cores"
             ;;
@@ -222,11 +222,11 @@ then
     WM_NCOMPPROCS=$(wmakeScheduler -count) || unset WM_NCOMPPROCS
 fi
 
-if [ "$WM_NCOMPPROCS" ]
+if [ -n "$WM_NCOMPPROCS" ]
 then
     parOpt="-j $WM_NCOMPPROCS"
 
-    if [ "$WM_NCOMPPROCS" -gt 1 ] && [ ! "$MAKEFLAGS" ]
+    if [ "$WM_NCOMPPROCS" -gt 1 ] && [ -z "$MAKEFLAGS" ]
     then
         lockDir=$HOME/.$WM_PROJECT/.wmake
 
diff --git a/wmake/wrmdep b/wmake/wrmdep
index 644fab0..b2a9ecd 100755
--- a/wmake/wrmdep
+++ b/wmake/wrmdep
@@ -55,7 +55,7 @@ Script=${0##*/}
 
 # Source the wmake functions
 # shellcheck source=scripts/wmakeFunctions
-. ${0%/*}/scripts/wmakeFunctions
+. "${0%/*}/scripts/wmakeFunctions"
 
 usage() {
     cat<<USAGE
@@ -92,6 +92,18 @@ error() {
     exit 1
 }
 
+removeDepFiles()
+{
+    if [ "$#" -eq 1 ]
+    then
+        find "$1" -name '*.dep' -print0 | xargs -0t rm 2>/dev/null
+    elif [ "$#" -eq 2 ]
+    then
+        find "$1" -name '*.dep' -exec grep -q "$2" '{}' \; \
+             -exec rm '{}' \; -print
+    fi
+}
+
 #------------------------------------------------------------------------------
 # Parse arguments and options
 #------------------------------------------------------------------------------
@@ -144,20 +156,37 @@ files)
 
     findObjectDir .
 
-    # With the -a/-all option replace the current platform with a wildcard
-    if [ "$all" = "all" ]
-    then
-        objectsDir=$(echo "$objectsDir" | sed s%"$WM_OPTIONS"%*% )
-    fi
-
+    # First give the verbose information for the current searching method
     if [ "$#" -eq 0 ]
     then
         echo "removing all .dep files ..."
-        find "$objectsDir" -name '*.dep' -print0 | xargs -0t rm 2>/dev/null
     else
         echo "removing .dep files referring to $1 ..."
-        find "$objectsDir" -name '*.dep' -exec grep -q "$1" '{}' \; \
-             -exec rm '{}' \; -print
+    fi
+
+    # With the -a/-all option replace the current platform with a wildcard
+    if [ "$all" = "all" ]
+    then
+
+        # Handle the removal on all platforms for consistency
+        mainPath=${objectsDir%%/$WM_OPTIONS*}
+        objSubPath=${objectsDir##*$WM_OPTIONS/}
+
+        for pathEntry in "$mainPath"/*
+        do
+            if [ -e "$pathEntry/$objSubPath" ]
+            then
+                # Note:
+                # If $1 is empty, it should still work for the default method
+                removeDepFiles "$pathEntry/$objSubPath" "$1"
+            fi
+        done
+
+    else
+
+        # Handle only the removal for the current WM_OPTIONS
+        removeDepFiles "$objectsDir" "$1"
+
     fi
 
     ;;
diff --git a/wmake/wrmo b/wmake/wrmo
index a0fe20b..9adb8ab 100755
--- a/wmake/wrmo
+++ b/wmake/wrmo
@@ -39,7 +39,7 @@ Script=${0##*/}
 
 # Source the wmake functions
 # shellcheck source=scripts/wmakeFunctions
-. ${0%/*}/scripts/wmakeFunctions
+. "${0%/*}/scripts/wmakeFunctions"
 
 usage() {
     cat<<USAGE
proposition_v2.patch (4,035 bytes)   

wyldckat

2018-06-17 20:02

updater  

proposition_v2.tar.gz (7,800 bytes)

wyldckat

2018-06-17 20:09

updater   ~0009786

I believe I properly tested things this time around and shellcheck (built from source code version 0.5.0) now agrees with the changes too.

The attached files "proposition_v2" provides fixes for the issues originally mentioned, as well as fixing another bug I had introduced in "proposition_v1" in script 'wrmdep'.
Furthermore, I picked up on a change that didn't propagate to other files, namely the quoting of the path to 'wmakeFunctions'.


The modifications in this "proposition_v2" are as follows:

 - wmake/wmake

     - Changed the checking method for the number of cores next to '-j' to use '[ "$2" -gt 0 ]' instead of '(($2 + 1))'.

     - Properly check contents of the "$WM_NCOMPPROCS" and "$MAKEFLAGS" variables.


 - wmake/wrmdep

     - Properly handle searches for .dep files for either the current platform and all platforms, while still abiding to the quoted variables that shellcheck proposed.

     - This change is still the same cumbersome change, but tries to ensure that things are done properly.


 - wmake/wclean
 - wmake/wdep
 - wmake/wrmo

     - Quoted the source call to 'wmakeFunctions'.

henry

2018-06-17 20:45

manager   ~0009787

Thanks Bruno

Resolved by commit ee4ed5d94256fc9ee3bc726bdaddc6622014d21b

Issue History

Date Modified Username Field Change
2018-06-17 17:09 wyldckat New Issue
2018-06-17 17:09 wyldckat Status new => assigned
2018-06-17 17:09 wyldckat Assigned To => henry
2018-06-17 17:09 wyldckat File Added: proposition_v1.tar.gz
2018-06-17 17:09 wyldckat File Added: proposition_v1.patch
2018-06-17 17:10 wyldckat Summary Pathing a couple of bugs introduced with shellcheck in commit 0cca22576295 => Patching a couple of bugs introduced with shellcheck in commit 0cca22576295
2018-06-17 17:20 henry Note Added: 0009780
2018-06-17 17:41 wyldckat Note Added: 0009781
2018-06-17 17:59 henry Note Added: 0009782
2018-06-17 18:20 wyldckat Note Added: 0009783
2018-06-17 18:33 henry Note Added: 0009784
2018-06-17 19:16 wyldckat Note Added: 0009785
2018-06-17 20:01 wyldckat File Added: proposition_v2.patch
2018-06-17 20:02 wyldckat File Added: proposition_v2.tar.gz
2018-06-17 20:09 wyldckat Note Added: 0009786
2018-06-17 20:45 henry Status assigned => resolved
2018-06-17 20:45 henry Resolution open => fixed
2018-06-17 20:45 henry Fixed in Version => dev
2018-06-17 20:45 henry Note Added: 0009787