View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002983 | OpenFOAM | Patch | public | 2018-06-17 17:09 | 2018-06-17 20:45 |
| Reporter | wyldckat | Assigned To | henry | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Platform | GNU/Linux | OS | Ubuntu | OS Version | 16.04 |
| Product Version | dev | ||||
| Fixed in Version | dev | ||||
| Summary | 0002983: Patching a couple of bugs introduced with shellcheck in commit 0cca22576295 | ||||
| Description | The 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). | ||||
| Tags | No tags attached. | ||||
|
|
|
|
|
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
;;
|
|
|
> 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. |
|
|
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... |
|
|
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. |
|
|
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. |
|
|
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 |
|
|
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. |
|
|
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
|
|
|
|
|
|
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'. |
|
|
Thanks Bruno Resolved by commit ee4ed5d94256fc9ee3bc726bdaddc6622014d21b |
| 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 |
