Skip to content

Commit

Permalink
[githooks] CI scripts pipe failures to stderr (#1300)
Browse files Browse the repository at this point in the history
Previously, some scripts had output which would only be generated on
failure, but this output was passed to stdout, which `githooks/pre-push`
pipes to `/dev/null`. In this commit, any such output is unconditionally
redirected to `stderr` - if there's any output, we want to see it. This
previously resulted in very opaque errors during `git push`, which
should be addressed by this commit.

Also, in #728, we added `ci/check_all_toolchains_tested.sh`, but didn't
add it to `githooks/pre-push`. In this commit, we fix that, and also add
a test to `githooks/pre-push` to validate that all scripts in `ci/*` at
least show up in `githooks/pre-push` by name. This isn't a foolproof
check, but it should catch obvious errors.
  • Loading branch information
joshlf committed May 18, 2024
1 parent 1c77a9d commit cc5821c
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 9 deletions.
2 changes: 1 addition & 1 deletion ci/check_all_toolchains_tested.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@ diff \
sort -u | grep -v '^\(msrv\|stable\|nightly\)$') \
<(cargo metadata -q --format-version 1 | \
jq -r ".packages[] | select(.name == \"zerocopy\").metadata.\"build-rs\" | keys | .[]" | \
sort -u)
sort -u) >&2
2 changes: 1 addition & 1 deletion ci/check_fmt.sh
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,4 @@ if [[ -z $files ]]
then
exit 1
fi
rustfmt --check $files
rustfmt --check $files >&2
2 changes: 1 addition & 1 deletion ci/check_job_dependencies.sh
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ jobs=$(for i in $(find .github -iname '*.yaml' -or -iname '*.yml')
if [ -n "$jobs" ]
then
missing_jobs="$(echo "$jobs" | tr ' ' '\n')"
echo "all-jobs-succeed missing dependencies on some jobs: $missing_jobs" | tee -a $GITHUB_STEP_SUMMARY
echo "all-jobs-succeed missing dependencies on some jobs: $missing_jobs" | tee -a $GITHUB_STEP_SUMMARY >&2
exit 1
fi
2 changes: 1 addition & 1 deletion ci/check_readme.sh
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ set -eo pipefail
# suppress all errors from it.
cargo install -q cargo-readme --version 3.2.0

diff <(cargo -q run --manifest-path tools/Cargo.toml -p generate-readme) README.md
diff <(cargo -q run --manifest-path tools/Cargo.toml -p generate-readme) README.md >&2
exit $?
24 changes: 19 additions & 5 deletions githooks/pre-push
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ echo "Running pre-push git hook: $0"
# `cargo fmt` is useful (and the good stuff is not delivered by stderr).
#
# Background all jobs and wait for them so they can run in parallel.
./ci/check_fmt.sh & FMT_PID=$!
./ci/check_job_dependencies.sh >/dev/null & JOB_DEPS_PID=$!
./ci/check_msrv.sh >/dev/null & MSRV_PID=$!
./ci/check_readme.sh >/dev/null & README_PID=$!
./ci/check_versions.sh >/dev/null & VERSIONS_PID=$!
./ci/check_fmt.sh & FMT_PID=$!
./ci/check_all_toolchains_tested.sh >/dev/null & TOOLCHAINS_PID=$!
./ci/check_job_dependencies.sh >/dev/null & JOB_DEPS_PID=$!
./ci/check_msrv.sh >/dev/null & MSRV_PID=$!
./ci/check_readme.sh >/dev/null & README_PID=$!
./ci/check_versions.sh >/dev/null & VERSIONS_PID=$!

# `wait <pid>` exits with the same status code as the job it's waiting for.
# Since we `set -e` above, this will have the effect of causing the entire
Expand All @@ -27,7 +28,20 @@ echo "Running pre-push git hook: $0"
# jobs, it exits with code 0 even if one of the backgrounded jobs does not, so
# we can't use it here.
wait $FMT_PID
wait $TOOLCHAINS_PID
wait $JOB_DEPS_PID
wait $MSRV_PID
wait $README_PID
wait $VERSIONS_PID

# Ensure that this script calls all scripts in `ci/*`. This isn't a foolproof
# check since it just checks for the string in this script (e.g., it could be in
# a comment, which would trigger a false positive), but it should catch obvious
# errors. Also note that this entire hook is a nice-to-have - failures that
# aren't caught here will still be caught in CI.
#
# This was added because, in #728, we added `ci/check_all_toolchains_tested.sh`
# without calling it from this script.
for f in ./ci/*; do
grep "$f" githooks/pre-push >/dev/null || { echo "$f not called from githooks/pre-push" >&2 ; exit 1; }
done

0 comments on commit cc5821c

Please sign in to comment.