Skip to content

Commit

Permalink
Use bash strict mode in all CI scripts (#2012)
Browse files Browse the repository at this point in the history
"Bash strict mode" is a colloquial expression for a set of settings that
makes Bash stricter (safer). See e.g.
https://gist.github.com/robin-a-meade/58d60124b88b60816e8349d1e3938615

TLDR: to avoid errors from being silently ignored, we should set errexit
(exit script on error), nounset (exit script when an undefined variable
is encountered) and pipefail (propagate error codes in pipelines) in all
our scripts.

Unfortunately, `errexit` is in fact not enough to exit when any error
occurs, some errors are swallowed:
https://dev.to/banks/stop-ignoring-errors-in-bash-3co5

Because of this, I changed the whitespace script to gather the files in
a different way (I copied the approach from the clang-format script),
such
that the script will exit when an error occurs, instead of continuing to
run.

The whitespace script was missing a fetch which was causing `git
merge-base` to fail. Maybe our other scripts should also be updated to
only run on changed files.

Interestingly, GitHub already sets `-e` (`errexit`) when running
scripts, but it's best to enforce it in the script itself:

```
Run ./build_tools/github_actions/lint_whitespace_checks.sh
  ./build_tools/github_actions/lint_whitespace_checks.sh
  shell: /usr/bin/bash -e {0}
```

Fixes #2010
  • Loading branch information
mlevesquedion authored Feb 17, 2024
1 parent feb49e3 commit 5943c6b
Show file tree
Hide file tree
Showing 12 changed files with 58 additions and 12 deletions.
1 change: 1 addition & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ jobs:

- name: Run Whitespace Checks
run: |
git fetch --no-tags --prune --depth=1 origin "${GITHUB_BASE_REF?}:${GITHUB_BASE_REF?}"
./build_tools/github_actions/lint_whitespace_checks.sh
version-checks:
Expand Down
4 changes: 4 additions & 0 deletions build_tools/github_actions/ci_build_bazel.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

if [[ $# -ne 0 ]] ; then
echo "Usage: $0"
exit 1
Expand Down
4 changes: 4 additions & 0 deletions build_tools/github_actions/ci_build_cmake.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
# during `ci_configure`, and builds stablehlo in the directory specified
# by the second argument.

set -o errexit
set -o nounset
set -o pipefail

if [[ $# -ne 2 ]] ; then
echo "Usage: $0 <llvm_build_dir> <stablehlo_build_dir>"
exit 1
Expand Down
4 changes: 4 additions & 0 deletions build_tools/github_actions/ci_build_cmake_code_coverage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

print_usage() {
echo "Usage:"
echo "$0 [-g][-o output_dir] <llvm_build_dir> <stablehlo_build_dir>"
Expand Down
4 changes: 4 additions & 0 deletions build_tools/github_actions/ci_build_cmake_llvm.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
# This file is similar to build_mlir.sh, but passes different flags for
# caching in GitHub Actions to improve build speeds.

set -o errexit
set -o nounset
set -o pipefail

if [[ $# -ne 2 ]] ; then
echo "Usage: $0 <path/to/llvm> <build_dir>"
exit 1
Expand Down
1 change: 1 addition & 0 deletions build_tools/github_actions/ci_build_python_wheel.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
# At the moment it only builds a Linux variant of Python wheel for the
# default python3 version present on the system (set via GitHub action typically)
# TODO: Incorporate cibuildwheel and build manylinux wheels

set -o errexit
set -o nounset
set -o pipefail
Expand Down
10 changes: 7 additions & 3 deletions build_tools/github_actions/lint_clang_format.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,18 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

print_usage() {
echo "Usage: $0 [-fd]"
echo "Usage: $0 [-fb]"
echo " -f Auto-fix clang-format issues."
echo " -b <branch> Base branch name, default to origin/main."
echo " -b <branch> Base branch name, defaults to main."
}

FORMAT_MODE='validate'
BASE_BRANCH="$(git merge-base HEAD origin/main)"
BASE_BRANCH=main
while getopts 'fb:' flag; do
case "${flag}" in
f) FORMAT_MODE="fix" ;;
Expand Down
4 changes: 4 additions & 0 deletions build_tools/github_actions/lint_llvm_commit.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

print_usage() {
echo "Usage: $0 [-f] <path/to/stablehlo/root>"
echo " -f Auto-fix LLVM commit mismatch."
Expand Down
4 changes: 4 additions & 0 deletions build_tools/github_actions/lint_markdown.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
# If passing the files as a glob, be sure to wrap in quotes. For more info,
# see https://github.com/igorshubovych/markdownlint-cli#globbing

set -o errexit
set -o nounset
set -o pipefail

if [[ $# -gt 2 ]] ; then
echo "Usage: $0 [-f] <files|directories|globs>"
echo " -f Autofix markdown issues."
Expand Down
4 changes: 4 additions & 0 deletions build_tools/github_actions/lint_version.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
# checking if files named stablehlo_legalize_to_vhlo.X_Y_0.mlir and .mlir.bc
# exist.

set -o errexit
set -o nounset
set -o pipefail

## Setup VERSION variable as global:
VERSION_H="stablehlo/dialect/Version.h"
set_version_var() {
Expand Down
26 changes: 17 additions & 9 deletions build_tools/github_actions/lint_whitespace_checks.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,18 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

print_usage() {
echo "Usage: $0 [-f]"
echo "Usage: $0 [-fb]"
echo " -f Auto-fix whitespace issues."
echo " -b <branch> Base branch name, defaults to main."
}

FORMAT_MODE='validate'
BASE_BRANCH="$(git merge-base HEAD origin/main)"
BASE_BRANCH=main
while getopts 'fb:' flag; do
case "${flag}" in
f) FORMAT_MODE="fix" ;;
Expand All @@ -34,19 +39,22 @@ if [[ $# -ne 0 ]] ; then
exit 1
fi

get_source_files() {
git diff "$BASE_BRANCH" HEAD --name-only --diff-filter=d | grep '.*\.cpp$\|.*\.h$\|.*\.md$\|.*\.mlir$\|.*\.sh$\|.*\.td$\|.*\.txt$\|.*\.yml$\|.*\.yaml$'
}
echo "Checking whitespace:"
echo " $(get_source_files | xargs)"
echo "Gathering changed files..."
mapfile -t CHANGED_FILES < <(git diff "$BASE_BRANCH" HEAD --name-only --diff-filter=d | grep '.*\.cpp$\|.*\.h$\|.*\.md$\|.*\.mlir$\|.*\.sh$\|.*\.td$\|.*\.txt$\|.*\.yml$\|.*\.yaml$')
if (( ${#CHANGED_FILES[@]} == 0 )); then
echo "No files to check."
exit 0
fi

echo "${CHANGED_FILES[@]}"

files_without_eof_newline() {
# shellcheck disable=SC2016
get_source_files | xargs -L1 bash -c 'test "$(tail -c 1 "$0")" && echo "$0"'
printf "%s\n" "${CHANGED_FILES[@]}" | xargs -L1 bash -c 'test "$(tail -c 1 "$0")" && echo "$0"'
}

files_with_trailing_whitespace() {
get_source_files | xargs -L1 grep -lP '[ \t]+$'
printf "%s\n" "${CHANGED_FILES[@]}" | xargs -L1 grep -lP '[ \t]+$'
}

fix_files_without_eof_newline() {
Expand Down
4 changes: 4 additions & 0 deletions build_tools/github_actions/llvm_gcov.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
set -o nounset
set -o pipefail

# This is a helper script used by lcov in
# ci_build_cmake_code_coverage.sh
exec llvm-cov-14 gcov "$@"

0 comments on commit 5943c6b

Please sign in to comment.