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.

Sign up for GitHub

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

Jump to bottom

Shell: Address various shellcheck issues and formatting #3548

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Shell: Address various shellcheck issues and formatting #3548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8f192a3
CI(windows): Address shellcheck warnings in build_osgeo4w.sh
echoix Mar 30, 2024
7306341
test: Quote variables in test_thorough.sh
echoix Mar 30, 2024
bba2c8c
CI(ubuntu): Create makecmd variable as an array instead of a string t…
echoix Mar 30, 2024
b83fddf
CI(ubuntu): Sync structure of build_ubuntu-22.04_without_x.sh with bu…
echoix Mar 30, 2024
5368efb
docker(alpine): Incomplete shebang in docker/alpine/grass_tests.sh
echoix Mar 30, 2024
182478b
CI(macOS): Add missing quotes in make command
echoix Mar 30, 2024
865b79d
CI(macOS): Remove double quotes sandwiched in quoted CONFIGURE_FLAGS.
echoix Mar 30, 2024
ba3b9cd
docker: Uniformize shell scripts formatting
echoix Mar 30, 2024
9594172
binder: Double quote variables in postBuild
echoix Mar 30, 2024
882ea97
binder: Uniformize shell script formatting
echoix Mar 30, 2024
f2c4bdf
utils(module_synopsis): Use $(...) notation instead of legacy backtic…
echoix Mar 30, 2024
9089e1e
utils(module_synopsis): Exit if cd fails
echoix Mar 30, 2024
eb93acc
utils(module_synopsis): Double quote $(...) command substitutions
echoix Mar 30, 2024
9ceeaa4
utils(module_synopsis): Disable some shellcheck errors as false posit…
echoix Mar 30, 2024
00e99ae
utils(module_synopsis): Check exit code directly instead of with `$?`…
echoix Mar 30, 2024
556efc7
utils(module_synopsis): Uniformize shell script formatting
echoix Mar 30, 2024
cfd93f6
utils(module_synopsis): Disable SC2016 when single-quoting $GISBASE i…
echoix Mar 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/build_osgeo4w.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ set -e
test -d "$1" && cd "$1"

export OSGEO4W_ROOT_MSYS=/c/OSGeo4W
export SRC=$(pwd)
SRC="$(pwd)"
export SRC
export UNITTEST=1

# Build according to OSGeo4W recipe
${SRC}/mswindows/osgeo4w/build_osgeo4w.sh
"${SRC}"/mswindows/osgeo4w/build_osgeo4w.sh
8 changes: 4 additions & 4 deletions .github/workflows/build_ubuntu-22.04.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ fi
# Adding -Werror to make's CFLAGS is a workaround for configuring with
# an old version of configure, which issues compiler warnings and
# errors out. This may be removed with upgraded configure.in file.
makecmd="make"
if [[ "$#" -ge 2 ]]; then
makecmd=("make")
if [[ $# -ge 2 ]]; then
ARGS=("$@")
makecmd="make CFLAGS='$CFLAGS ${ARGS[@]:1}' CXXFLAGS='$CXXFLAGS ${ARGS[@]:1}'"
makecmd=("make" "CFLAGS=$CFLAGS ${ARGS[@]:1}" "CXXFLAGS=$CXXFLAGS ${ARGS[@]:1}")
Comment on lines -22 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I'd suggest to refactor, to make things more clear:

# concatenate arguments, from the second up till the last
function get_args_str() {
    local args=("$@");
    for arg in "${args[@]:1}"; do a+="${arg} "; done; echo "${a% }"
}

# Adding -Werror to make's CFLAGS is a workaround for configuring with
# an old version of configure, which issues compiler warnings and
# errors out. This may be removed with upgraded configure.in file.
makecmd="make"
if [[ "$#" -ge 2 ]]; then
    args_str=$(get_args_str "$@")
    makecmd+=" CFLAGS='${CFLAGS} ${args_str}' CXXFLAGS='${CXXFLAGS} ${args_str}'"
fi

fi

# non-existent variables as an errors
Expand Down Expand Up @@ -52,5 +52,5 @@ export INSTALL_PREFIX=$1
--with-fftw \
--with-netcdf

eval $makecmd
"${makecmd[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"${makecmd[@]}"
eval "$makecmd"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite the same. When having the quoted $makecmd, the arguments aren't split. Having unquoted $makecmd is problematic for globs, or all reasons why you'd want to quote variables.

Double quoted array with [@], is equivalent of double quoting each element. That would allow to properly handle a path with spaces as a single argument. (https://www.shellcheck.net/wiki/SC2068)

See https://www.shellcheck.net/wiki/SC2086 for general quoting tips (near the end), and https://www.shellcheck.net/wiki/SC2068 and https://www.shellcheck.net/wiki/SC2048 and
https://www.shellcheck.net/wiki/SC2294

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes with above suggestion.

make install
9 changes: 5 additions & 4 deletions .github/workflows/build_ubuntu-22.04_without_x.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ fi
# Adding -Werror to make's CFLAGS is a workaround for configuring with
# an old version of configure, which issues compiler warnings and
# errors out. This may be removed with upgraded configure.in file.
makecmd="make"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See build_ubuntu-22.04.sh

if [[ "$#" -eq 2 ]]; then
makecmd="make CFLAGS='$CFLAGS $2' CXXFLAGS='$CXXFLAGS $2'"
makecmd=("make")
if [[ $# -ge 2 ]]; then
ARGS=("$@")
makecmd=("make" "CFLAGS=$CFLAGS ${ARGS[@]:1}" "CXXFLAGS=$CXXFLAGS ${ARGS[@]:1}")
fi

# non-existent variables as an errors
Expand Down Expand Up @@ -50,5 +51,5 @@ export INSTALL_PREFIX=$1
--with-fftw \
--with-netcdf

eval $makecmd
"${makecmd[@]}"
make install
6 changes: 3 additions & 3 deletions .github/workflows/macos_install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ CONFIGURE_FLAGS="\
--with-cairo \
--with-cairo-includes=${CONDA_PREFIX}/include/cairo \
--with-cairo-libs=${CONDA_PREFIX}/lib \
--with-cairo-ldflags="-lcairo" \
--with-cairo-ldflags=-lcairo \
--with-zstd \
--with-zstd-libs=${CONDA_PREFIX}/lib \
--with-zstd-includes=${CONDA_PREFIX}/include \
Expand Down Expand Up @@ -70,7 +70,7 @@ export CPPFLAGS="-isystem${CONDA_PREFIX}/include"
./configure $CONFIGURE_FLAGS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
./configure $CONFIGURE_FLAGS
./configure "$CONFIGURE_FLAGS"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I wasn't sure that it would work on posix shell on Mac, and didn't know the complete side effects

Copy link
Contributor

@nilason nilason Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What shellcheck suggest is the same on Mac.


EXEMPT="-Wno-error=deprecated-non-prototype -Wno-error=strict-prototypes"
make -j$(sysctl -n hw.ncpu) CFLAGS="$CFLAGS -Werror $EXEMPT" \
CXXFLAGS="$CXXFLAGS -Werror $EXEMPT"
make -j"$(sysctl -n hw.ncpu)" CFLAGS="$CFLAGS -Werror $EXEMPT" \
CXXFLAGS="$CXXFLAGS -Werror $EXEMPT"

make install
6 changes: 3 additions & 3 deletions .github/workflows/test_thorough.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
set -e

grass --tmp-location XY --exec \
g.download.location url=https://grass.osgeo.org/sampledata/north_carolina/nc_spm_full_v2alpha2.tar.gz path=$HOME
g.download.location url=https://grass.osgeo.org/sampledata/north_carolina/nc_spm_full_v2alpha2.tar.gz path="$HOME"

grass --tmp-location XY --exec \
python3 -m grass.gunittest.main \
--grassdata $HOME --location nc_spm_full_v2alpha2 --location-type nc \
--min-success 100 $@
--grassdata "$HOME" --location nc_spm_full_v2alpha2 --location-type nc \
--min-success 100 "$@"
12 changes: 6 additions & 6 deletions binder/postBuild
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ make

# put command on path
# ensure the user specific bin dir exists (already on path)
mkdir -p $HOME/.local/bin/
mkdir -p "$HOME/.local/bin/"
# create link to build
ln -s $HOME/bin.*/grass* $HOME/.local/bin/grass
ln -s "$HOME"/bin.*/grass* "$HOME/.local/bin/grass"

# download a sample dataset
mkdir -p data/grassdata \
&& curl -SL https://grass.osgeo.org/sampledata/north_carolina/nc_basic_spm_grass7.zip > nc_basic_spm_grass7.zip \
&& unzip -qq nc_basic_spm_grass7.zip \
&& mv nc_basic_spm_grass7 data/grassdata \
&& rm nc_basic_spm_grass7.zip
&& curl -SL https://grass.osgeo.org/sampledata/north_carolina/nc_basic_spm_grass7.zip > nc_basic_spm_grass7.zip \
&& unzip -qq nc_basic_spm_grass7.zip \
&& mv nc_basic_spm_grass7 data/grassdata \
&& rm nc_basic_spm_grass7.zip
14 changes: 7 additions & 7 deletions docker/alpine/grass_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#/bin/bash
#!/bin/bash

# to be used in alpine Dockerfile

Expand All @@ -11,14 +11,14 @@ apk add --no-cache py3-scikit-learn
grass --tmp-location EPSG:25832 --exec r.in.pdal input="/tmp/simple.laz" output="count_1" method="n" resolution=1 -g

# Test GRASS GIS Python-addon installation
grass --tmp-location XY --exec g.extension extension=r.learn.ml2 operation=add && \
grass --tmp-location XY --exec g.extension extension=r.learn.ml2 operation=remove -f
grass --tmp-location XY --exec g.extension extension=r.learn.ml2 operation=add \
&& grass --tmp-location XY --exec g.extension extension=r.learn.ml2 operation=remove -f

# Test GRASS GIS C-addon installation: raster and vector
grass --tmp-location XY --exec g.extension extension=r.gwr operation=add && \
grass --tmp-location XY --exec g.extension extension=r.gwr operation=remove -f
grass --tmp-location XY --exec g.extension extension=v.centerpoint operation=add && \
grass --tmp-location XY --exec g.extension extension=v.centerpoint operation=remove -f
grass --tmp-location XY --exec g.extension extension=r.gwr operation=add \
&& grass --tmp-location XY --exec g.extension extension=r.gwr operation=remove -f
grass --tmp-location XY --exec g.extension extension=v.centerpoint operation=add \
&& grass --tmp-location XY --exec g.extension extension=v.centerpoint operation=remove -f

# cleanup dependency
apk del py3-scikit-learn
24 changes: 12 additions & 12 deletions docker/testdata/test_docker_image.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,31 +36,31 @@ grass --tmp-location EPSG:25832 --exec r.in.pdal input="/tmp/simple.laz" output=
printf "\n############\nTesting GRASS GIS Python-addon installation:\n############\n"
/usr/bin/python3 -m pip install --no-cache-dir scikit-learn

grass --tmp-location XY --exec g.extension extension=r.learn.ml2 operation=add && \
grass --tmp-location XY --exec g.extension extension=r.learn.ml2 operation=remove -f
grass --tmp-location XY --exec g.extension extension=r.learn.ml2 operation=add \
&& grass --tmp-location XY --exec g.extension extension=r.learn.ml2 operation=remove -f

# cleanup dependency
/usr/bin/python3 -m pip uninstall -y scikit-learn

# Test GRASS GIS C-addon installation: raster and vector
printf "\n############\nTesting GRASS GIS C-addon installation:\n############\n"
grass --tmp-location XY --exec g.extension extension=r.gwr operation=add && \
grass --tmp-location XY --exec g.extension extension=r.gwr operation=remove -f
grass --tmp-location XY --exec g.extension extension=v.centerpoint operation=add && \
grass --tmp-location XY --exec g.extension extension=v.centerpoint operation=remove -f
grass --tmp-location XY --exec g.extension extension=r.gwr operation=add \
&& grass --tmp-location XY --exec g.extension extension=r.gwr operation=remove -f
grass --tmp-location XY --exec g.extension extension=v.centerpoint operation=add \
&& grass --tmp-location XY --exec g.extension extension=v.centerpoint operation=remove -f

# show GRASS GIS, PROJ, GDAL etc versions
printf "\n############\nPrinting GRASS, PDAL and Python versions:\n############\n"
grass --tmp-location EPSG:4326 --exec g.version -rge && \
pdal --version && \
python3 --version
grass --tmp-location EPSG:4326 --exec g.version -rge \
&& pdal --version \
&& python3 --version

# Test presence of central python packages
printf "\n############\nPrinting versions of central python packages:\n############\n"
python3 -c "import psycopg2;import numpy as np;print(psycopg2.__version__);print(np.__version__)"

# Run testsuite
if [ $TESTSUITE ] ; then
printf "\n############\nRunning the testsuite:\n############\n"
bash /grassdb/.github/workflows/test_thorough.sh
if [ "$TESTSUITE" ]; then
printf "\n############\nRunning the testsuite:\n############\n"
bash /grassdb/.github/workflows/test_thorough.sh
fi
Loading
Loading