Update maintainer and CI shell scripts #3326

jngrad · 2019-11-18T18:05:22Z

Description of changes:

reduce number of shellcheck warnings from 238 down to 104
- rules: SC2002 SC2006 SC2061 SC2063 SC2086 SC2116 SC2148 SC2166 SC2196
- 50 of the remaining warnings are false positives from build_cmake.sh (SC2086 SC2154)
refactor outdated maintainer shell scripts
remove deprecated shell syntax and shell commands
use POSIX alternatives to bashisms when the change is trivial
quote string variables to guard against whitespace-related issues
start unit tests before integration tests
run unit tests for all operating systems (Debian, OpenSUSE, Fedora)

Notes:

shellcheck is unsuitable for CI in its current form (e.g. no rule whitelisting capability)
the shell scripts now have an homogeneous syntax and few linter warnings, which is desirable if we decide to port them to Python in the future

POSIX does not specify test for > 4 arguments: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html shellcheck rule: SC2166

POSIX allows both the legacy `cmd` and the new-style $(cmd) syntaxes: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03 However, POSIX recommends using the new-style syntax, as it has less restrictions and allows nesting without escape characters: https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html#tag_23_02_06_03 shellcheck rule: SC2006

shellcheck rule: SC2086

The original pattern wouldn't work if a filename was the suffix of another, e.g. `lb.py` would be marked as used in the CMakeLists.txt because of a match at the line `python_test(FILE engine_lb.py)`. The new pattern is also not a regular expression: the dot in the file extension doesn't match any character, only the dot character.

Replace deprecated `egrep` by `grep -E`. shellcheck rule: SC2196

Add a shebang in all shell scripts. Downgrade shebangs from Bash to Dash (sh) in small scripts that don't use Bash-specific syntax. Use the most portable shebang (`#!/usr/bin/env <shell>`) to include the user $PATH in the search path. shellcheck rule: SC2148

shellcheck rules: SC2002 SC2116

shellcheck rules: SC2061 SC2063

Separate unit tests from python tests. Run unit tests on Fedora, Debian and OpenSUSE.

codecov · 2019-11-18T18:31:54Z

Codecov Report

Merging #3326 into python will decrease coverage by <1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           python   #3326    +/-   ##
=======================================
- Coverage      86%     86%    -1%     
=======================================
  Files         538     536     -2     
  Lines       25411   25562   +151     
=======================================
+ Hits        21895   21997   +102     
- Misses       3516    3565    +49

Impacted Files	Coverage Δ
src/utils/include/utils/mask.hpp	`100% <ø> (ø)`	⬆️
...core/electrostatics_magnetostatics/p3m-dipolar.cpp	`81% <0%> (-2%)`	⬇️
src/core/integrators/velocity_verlet_npt.cpp	`85% <0%> (-1%)`	⬇️
src/core/particle_data.cpp	`97% <0%> (-1%)`	⬇️
src/core/BoxGeometry.hpp	`100% <0%> (ø)`	⬆️
src/utils/include/utils/mpi/cart_comm.hpp	`100% <0%> (ø)`	⬆️
src/core/grid.cpp	`100% <0%> (ø)`	⬆️
...core/electrostatics_magnetostatics/p3m-dipolar.hpp	`96% <0%> (ø)`	⬆️
src/core/cells.hpp	`100% <0%> (ø)`	⬆️
src/core/polynom.hpp	`33% <0%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c39387f...93ec8c6. Read the comment docs.

fweik · 2019-11-18T19:08:40Z

Is it possible to run the scripts in the CI in POSIX compatibility mode (e.g. using sh, and not bash)?
This would be a simple test that they actually are POSIX compatible.

jngrad · 2019-11-19T09:58:04Z

Not really. For example build_cmake.sh has 12 bashisms. I've already downgraded scripts that could run in dash with trivial changes. For the less trivial bashisms, I could have a look if you want, but some of the known POSIX equivalents (see SC2039) don't look very maintainable.

Reduce bashisms. Downgrade shebangs from Bash to Dash (sh) in POSIX-compliant shell scripts. Use the most portable shebang (`#!/usr/bin/env <shell>`) to include the user $PATH in the search path.

jngrad · 2019-11-19T12:42:49Z

I've removed more bashisms, but several shell scripts still rely on useful bash-specific features:

variable indirections don't seem to have a POSIX equivalent without eval
shell traps: SIGINT, SIGTERM and SIGQUIT automatically trigger SIGEXIT in bash, while in POSIX one has to write a custom trap for each one of them (e.g. a SIGINT trap must forward SIGINT)
$BASH_COMMAND and $LINENO don't exist in POSIX (useful for debug messages)
arrays only exist natively in bash
build_cmake.sh downloads and executes a codecov script which is not POSIX-compliant

List of bash scripts:

head -n 1 $(find maintainer/ testsuite/ -name "*.sh")

maintainer/CI/build_cmake.sh
maintainer/CI/build_docker.sh
maintainer/CI/doc_warnings.sh
maintainer/benchmarks/runner.sh
maintainer/benchmarks/suite.sh
testsuite/cmake/BashUnitTests.sh
testsuite/cmake/test_install.sh

fweik · 2019-11-19T13:26:08Z

I also don't think it is so super important. At the end of the day most of those scripts only run in an environment that we control.

mkuron

Looks fine. We should really migrate this mess to Python scripts ASAP though.

mkuron · 2019-12-05T10:39:27Z

maintainer/CI/dox_warnings.sh

@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/usr/bin/env sh


/usr/bin/env sh is useless as /usr/bin/env is not guaranteed by POSIX (neither is /bin/sh, by the way). I would just use /bin/sh.

Here the idea was to be more portable, as the program env reads the user $PATH. I put this change in the POSIX bucket for convenience, since most scripts modified in that commit had heterogenous shebangs that needed fixing, although it has nothing to do with POSIX. In fact, sh might not even point to the dash interpreter.

KaiSzuttor

LGTM, it seems like we develop a DevOps library in bash without knowing it 😄

jngrad · 2019-12-05T11:46:57Z

bors r=KaiSzuttor

3326: Update maintainer and CI shell scripts r=KaiSzuttor a=jngrad Fixes #3242 Description of changes: - reduce number of shellcheck warnings from 238 down to 104 - rules: `SC2002 SC2006 SC2061 SC2063 SC2086 SC2116 SC2148 SC2166 SC2196` - 50 of the remaining warnings are false positives from `build_cmake.sh` (`SC2086 SC2154`) - refactor outdated maintainer shell scripts - remove deprecated shell syntax and shell commands - use POSIX alternatives to bashisms when the change is trivial - quote string variables to guard against whitespace-related issues - start unit tests before integration tests - run unit tests for all operating systems (Debian, OpenSUSE, Fedora) Notes: - `shellcheck` is unsuitable for CI in its current form (e.g. no rule whitelisting capability) - the shell scripts now have an homogeneous syntax and few linter warnings, which is desirable if we decide to port them to Python in the future Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>

bors · 2019-12-05T12:37:05Z

Build succeeded

ICP GitLab CI

jngrad added 19 commits November 15, 2019 18:35

CI: run unit tests before integration tests

898a96e

Use || instead of -o

bcbdfb6

POSIX does not specify test for > 4 arguments: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html shellcheck rule: SC2166

Add braces around shell variables

c36db70

Double quote string variables

e289050

shellcheck rule: SC2086

Formatting

a4341a1

Use POSIX grep

8ba479c

Replace deprecated `egrep` by `grep -E`. shellcheck rule: SC2196

Remove inexistent files from patterns

b15e032

Add new files in patterns

96c7fed

Fix broken stdout

6d1aafc

Fix broken script

d744ba1

Update shebangs

984b061

Add a shebang in all shell scripts. Downgrade shebangs from Bash to Dash (sh) in small scripts that don't use Bash-specific syntax. Use the most portable shebang (`#!/usr/bin/env <shell>`) to include the user $PATH in the search path. shellcheck rule: SC2148

Remove superfluous echo and cat

5b05833

shellcheck rules: SC2002 SC2116

Properly escape globs in grep and find

f23cb30

shellcheck rules: SC2061 SC2063

Run scripts in root directory

176b6bd

Document shell scripts

6a5d4a3

Add copyright headers

c36e2bf

Run unit tests on all operating systems

df1758a

Separate unit tests from python tests. Run unit tests on Fedora, Debian and OpenSUSE.

jngrad added this to the 4.1.2 milestone Nov 18, 2019

Use POSIX syntax

0f11fcc

Reduce bashisms. Downgrade shebangs from Bash to Dash (sh) in POSIX-compliant shell scripts. Use the most portable shebang (`#!/usr/bin/env <shell>`) to include the user $PATH in the search path.

Quote string variables

dac3b47

jngrad mentioned this pull request Nov 29, 2019

Disable P3M benchmark test #3347

Closed

Quote string variables

93ec8c6

mkuron approved these changes Dec 5, 2019

View reviewed changes

KaiSzuttor approved these changes Dec 5, 2019

View reviewed changes

bors bot merged commit 93ec8c6 into espressomd:python Dec 5, 2019

jngrad mentioned this pull request Dec 9, 2019

Adapt syntax to the refactored build_cmake.sh espressomd/docker#150

Merged

jngrad deleted the fix-3242 branch January 18, 2022 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update maintainer and CI shell scripts #3326

Update maintainer and CI shell scripts #3326

jngrad commented Nov 18, 2019

codecov bot commented Nov 18, 2019 •

edited

Loading

fweik commented Nov 18, 2019

jngrad commented Nov 19, 2019

jngrad commented Nov 19, 2019

fweik commented Nov 19, 2019

mkuron left a comment

mkuron Dec 5, 2019

jngrad Dec 5, 2019

KaiSzuttor left a comment

jngrad commented Dec 5, 2019

bors bot commented Dec 5, 2019

Update maintainer and CI shell scripts #3326

Update maintainer and CI shell scripts #3326

Conversation

jngrad commented Nov 18, 2019

codecov bot commented Nov 18, 2019 • edited Loading

Codecov Report

fweik commented Nov 18, 2019

jngrad commented Nov 19, 2019

jngrad commented Nov 19, 2019

fweik commented Nov 19, 2019

mkuron left a comment

Choose a reason for hiding this comment

mkuron Dec 5, 2019

Choose a reason for hiding this comment

jngrad Dec 5, 2019

Choose a reason for hiding this comment

KaiSzuttor left a comment

Choose a reason for hiding this comment

jngrad commented Dec 5, 2019

bors bot commented Dec 5, 2019

Build succeeded

codecov bot commented Nov 18, 2019 •

edited

Loading