Skip to content

Commit

Permalink
Add git pre-push hook (#728)
Browse files Browse the repository at this point in the history
* [ci] Extract scripts in YAML to individual ci/*.sh

* [ci] Rename check_job_dependencies for consistency

* [ci] Run some checks on pre-push

* [ci] check for yq before trying to run it

* [ci] replace globstar for better compatibility

* [ci] streamline check_fmt for performance

* [ci] Use /usr/bin/env for improved compatibility

* [ci] check that there are files for formatting

* [ci] set check_all_toolchains_tested executable

* [ci] make githook quieter (errors still show)

* [ci] help user to know they are using pre-push hook

* [ci] add documentation for git hook config

* [ci] use function args instead of globals in check_versions.sh

* [ci] fix formatting in git hooks documentation

* [ci] direct all ci script stdout to null in git hook

* [ci] make sure rustfmt stdout gets printed

* [ci] explain why check_fmt.sh is not silenced
  • Loading branch information
tommy-gilligan committed Feb 14, 2024
1 parent 76f5e49 commit bbc228a
Show file tree
Hide file tree
Showing 9 changed files with 173 additions and 137 deletions.
144 changes: 7 additions & 137 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -328,13 +328,7 @@ jobs:
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Check Rust formatting
run: |
set -eo pipefail
cargo fmt --check -p zerocopy
cargo fmt --check -p zerocopy-derive
shopt -s globstar
rustfmt --check tests/**/*.rs
rustfmt --check zerocopy-derive/tests/**/*.rs
run: ./ci/check_fmt.sh

check_readme:
needs: generate_cache
Expand All @@ -347,16 +341,7 @@ jobs:
uses: ./.github/actions/cache

- name: Check README.md
run: |
set -eo pipefail
# Install again in case the installation failed during the
# `generate_cache` step. We treat that step as best-effort and
# suppress all errors from it.
cargo install cargo-readme --version 3.2.0
diff <(./generate-readme.sh) README.md
exit $?
run: ./ci/check_readme.sh

check_msrv:
needs: generate_cache
Expand All @@ -378,25 +363,7 @@ jobs:
# that compiling zerocopy with the `derive` feature enabled would fail
# on its own published MSRV)
- name: Check MSRVs match
run: |
set -eo pipefail
# Usage: msrv <crate-name>
function msrv {
cargo metadata --format-version 1 | jq -r ".packages[] | select(.name == \"$1\").rust_version"
}
ver_zerocopy=$(msrv zerocopy)
ver_zerocopy_derive=$(msrv zerocopy-derive)
if [[ "$ver_zerocopy" == "$ver_zerocopy_derive" ]]; then
echo "Same MSRV ($ver_zerocopy) found for zerocopy and zerocopy-derive." | tee -a $GITHUB_STEP_SUMMARY
exit 0
else
echo "Different MSRVs found for zerocopy ($ver_zerocopy) and zerocopy-derive ($ver_zerocopy_derive)." \
| tee -a $GITHUB_STEP_SUMMARY >&2
exit 1
fi
run: ./ci/check_msrv.sh

check_versions:
needs: generate_cache
Expand All @@ -412,67 +379,8 @@ jobs:
# depends exactly upon the current version of zerocopy-derive. See
# `INTERNAL.md` for an explanation of why we do this.
- name: Check crate versions match
run: |
set -eo pipefail
# Usage: version <crate-name>
function version {
cargo metadata --format-version 1 | jq -r ".packages[] | select(.name == \"$1\").version"
}
ver_zerocopy=$(version zerocopy)
ver_zerocopy_derive=$(version zerocopy-derive)
# Usage: dependency-version <kind> <target>
function dependency-version {
KIND="$1"
TARGET="$2"
cargo metadata --format-version 1 \
| jq -r ".packages[] | select(.name == \"zerocopy\").dependencies[] | select((.name == \"zerocopy-derive\") and .kind == $KIND and .target == $TARGET).req"
}
# The non-dev dependency version (kind `null` filters out the dev
# dependency, and target `null` filters out the targeted version).
zerocopy_derive_dep_ver=$(dependency-version null null)
# The non-dev dependency, targeted version (kind `null` filters out
# the dev dependency).
zerocopy_derive_targeted_ver=$(dependency-version null '"cfg(any())"')
# The dev dependency version (kind `"dev"` selects only the dev
# dependency).
zerocopy_derive_dev_dep_ver=$(dependency-version '"dev"' null)
function assert-match {
VER_A="$1"
VER_B="$2"
SUCCESS_MSG="$3"
FAILURE_MSG="$4"
if [[ "$ver_zerocopy" == "$ver_zerocopy_derive" ]]; then
echo "$SUCCESS_MSG" | tee -a $GITHUB_STEP_SUMMARY
else
echo "$FAILURE_MSG" | tee -a $GITHUB_STEP_SUMMARY >&2
exit 1
fi
}
assert-match "$ver_zerocopy" "$ver_zerocopy_derive" \
"Same crate version ($ver_zerocopy) found for zerocopy and zerocopy-derive." \
"Different crate versions found for zerocopy ($ver_zerocopy) and zerocopy-derive ($ver_zerocopy_derive)."
# Note the leading `=` sign - the dependency needs to be an exact one.
assert-match "=$ver_zerocopy_derive" "$zerocopy_derive_dep_ver" \
"zerocopy depends upon same version of zerocopy-derive in-tree ($zerocopy_derive_dep_ver)." \
"zerocopy depends upon different version of zerocopy-derive ($zerocopy_derive_dep_ver) than the one in-tree ($ver_zerocopy_derive)."
# Note the leading `=` sign - the dependency needs to be an exact one.
assert-match "=$ver_zerocopy_derive" "$zerocopy_derive_dev_dep_ver" \
"In dev mode, zerocopy depends upon same version of zerocopy-derive in-tree ($zerocopy_derive_dev_dep_ver)." \
"In dev mode, zerocopy depends upon different version of zerocopy-derive ($zerocopy_derive_dev_dep_ver) than the one in-tree ($ver_zerocopy_derive)."
assert-match "$zerocopy_derive_dep_ver" "$zerocopy_derive_targeted_ver" \
"Same crate version ($zerocopy_derive_dep_ver) found for optional and targeted zerocopy-derive dependency." \
"Different crate versions found for optional ($zerocopy_derive_dep_ver) and targeted ($zerocopy_derive_targeted_ver) dependency."
run: ./ci/check_versions.sh

generate_cache:
runs-on: ubuntu-latest
name: Generate cache
Expand Down Expand Up @@ -511,22 +419,8 @@ jobs:
run: go install github.com/mikefarah/yq/v4@latest
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Run check
run: |
set -eo pipefail
run: ./ci/check_all_toolchains_tested.sh

# Check whether the set of toolchains tested in this file (other than
# 'msrv', 'stable', and 'nightly') is equal to the set of toolchains
# listed in the 'package.metadata.build-rs' section of Cargo.toml.
#
# If the inputs to `diff` are not identical, `diff` exits with a
# non-zero error code, which causes this script to fail (thanks to
# `set -e`).
diff \
<(cat .github/workflows/ci.yml | yq '.jobs.build_test.strategy.matrix.toolchain | .[]' | \
sort -u | grep -v '^\(msrv\|stable\|nightly\)$') \
<(cargo metadata --format-version 1 | \
jq -r ".packages[] | select(.name == \"zerocopy\").metadata.\"build-rs\" | keys | .[]" | \
sort -u)
check-job-dependencies:
runs-on: ubuntu-latest
name: Check all-jobs-succeeded depends on all jobs
Expand All @@ -535,31 +429,7 @@ jobs:
run: go install github.com/mikefarah/yq/v4@latest
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Run dependency check
run: |
set -eo pipefail
jobs=$(for i in $(find .github -iname '*.yaml' -or -iname '*.yml')
do
# Select jobs that are triggered by pull request.
if yq -e '.on | has("pull_request")' "$i" 2>/dev/null >/dev/null
then
# This gets the list of jobs that all-jobs-succeed does not depend on.
comm -23 \
<(yq -r '.jobs | keys | .[]' "$i" | sort | uniq) \
<(yq -r '.jobs.all-jobs-succeed.needs[]' "$i" | sort | uniq)
fi
# The grep call here excludes all-jobs-succeed from the list of jobs that
# all-jobs-succeed does not depend on. If all-jobs-succeed does
# not depend on itself, we do not care about it.
done | sort | uniq | (grep -v '^all-jobs-succeed$' || true)
)
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
exit 1
fi
run: ./ci/check_job_dependencies.sh

# Used to signal to branch protections that all other jobs have succeeded.
all-jobs-succeed:
Expand Down
15 changes: 15 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,21 @@ Commit messages should never contain references to any of:
https://github.com/google/zerocopy/commit/789b3deb)
1. Other entities which may not make sense to arbitrary future readers

### Git hooks

There is a pre-push git hook available at `./githooks/pre-push`. This hook runs
some quick checks locally before pushing so that these same checks won't later
fail during CI. The intention is to minimise needing to fix trivial breakages
after having already just pushed (which can be annoying).

Git won't use this hook automatically. If you would like to use it, set
repository config `core.hooksPath` to `githooks`. This can be done by running
(at repository root):

```sh
git config --local core.hooksPath githooks
```

## Community Guidelines

This project follows [Google's Open Source Community
Expand Down
16 changes: 16 additions & 0 deletions ci/check_all_toolchains_tested.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#!/usr/bin/env bash
set -eo pipefail

# Check whether the set of toolchains tested in this file (other than
# 'msrv', 'stable', and 'nightly') is equal to the set of toolchains
# listed in the 'package.metadata.build-rs' section of Cargo.toml.
#
# If the inputs to `diff` are not identical, `diff` exits with a
# non-zero error code, which causes this script to fail (thanks to
# `set -e`).
diff \
<(cat .github/workflows/ci.yml | yq '.jobs.build_test.strategy.matrix.toolchain | .[]' | \
sort -u | grep -v '^\(msrv\|stable\|nightly\)$') \
<(cargo metadata --format-version 1 | \
jq -r ".packages[] | select(.name == \"zerocopy\").metadata.\"build-rs\" | keys | .[]" | \
sort -u)
9 changes: 9 additions & 0 deletions ci/check_fmt.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/usr/bin/env bash
set -eo pipefail
files=$(find . -iname '*.rs' -type f -not -path './target/*')
# check that find succeeded
if [[ -z $files ]]
then
exit 1
fi
rustfmt --check $files
26 changes: 26 additions & 0 deletions ci/check_job_dependencies.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#!/usr/bin/env bash
set -eo pipefail
which yq > /dev/null
jobs=$(for i in $(find .github -iname '*.yaml' -or -iname '*.yml')
do
# Select jobs that are triggered by pull request.
if yq -e '.on | has("pull_request")' "$i" 2>/dev/null >/dev/null
then
# This gets the list of jobs that all-jobs-succeed does not depend on.
comm -23 \
<(yq -r '.jobs | keys | .[]' "$i" | sort | uniq) \
<(yq -r '.jobs.all-jobs-succeed.needs[]' "$i" | sort | uniq)
fi
# The grep call here excludes all-jobs-succeed from the list of jobs that
# all-jobs-succeed does not depend on. If all-jobs-succeed does
# not depend on itself, we do not care about it.
done | sort | uniq | (grep -v '^all-jobs-succeed$' || true)
)

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
exit 1
fi
19 changes: 19 additions & 0 deletions ci/check_msrv.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#!/usr/bin/env bash
set -eo pipefail

# Usage: msrv <crate-name>
function msrv {
cargo metadata --format-version 1 | jq -r ".packages[] | select(.name == \"$1\").rust_version"
}

ver_zerocopy=$(msrv zerocopy)
ver_zerocopy_derive=$(msrv zerocopy-derive)

if [[ "$ver_zerocopy" == "$ver_zerocopy_derive" ]]; then
echo "Same MSRV ($ver_zerocopy) found for zerocopy and zerocopy-derive." | tee -a $GITHUB_STEP_SUMMARY
exit 0
else
echo "Different MSRVs found for zerocopy ($ver_zerocopy) and zerocopy-derive ($ver_zerocopy_derive)." \
| tee -a $GITHUB_STEP_SUMMARY >&2
exit 1
fi
10 changes: 10 additions & 0 deletions ci/check_readme.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/usr/bin/env bash
set -eo pipefail

# Install again in case the installation failed during the
# `generate_cache` step. We treat that step as best-effort and
# suppress all errors from it.
cargo install cargo-readme --version 3.2.0 -q

diff <(./generate-readme.sh) README.md
exit $?
61 changes: 61 additions & 0 deletions ci/check_versions.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#!/usr/bin/env bash
set -eo pipefail

# Usage: version <crate-name>
function version {
cargo metadata --format-version 1 | jq -r ".packages[] | select(.name == \"$1\").version"
}

ver_zerocopy=$(version zerocopy)
ver_zerocopy_derive=$(version zerocopy-derive)

# Usage: dependency-version <kind> <target>
function dependency-version {
KIND="$1"
TARGET="$2"
cargo metadata --format-version 1 \
| jq -r ".packages[] | select(.name == \"zerocopy\").dependencies[] | select((.name == \"zerocopy-derive\") and .kind == $KIND and .target == $TARGET).req"
}

# The non-dev dependency version (kind `null` filters out the dev
# dependency, and target `null` filters out the targeted version).
zerocopy_derive_dep_ver=$(dependency-version null null)

# The non-dev dependency, targeted version (kind `null` filters out
# the dev dependency).
zerocopy_derive_targeted_ver=$(dependency-version null '"cfg(any())"')

# The dev dependency version (kind `"dev"` selects only the dev
# dependency).
zerocopy_derive_dev_dep_ver=$(dependency-version '"dev"' null)

function assert-match {
VER_A="$1"
VER_B="$2"
SUCCESS_MSG="$3"
FAILURE_MSG="$4"
if [[ "$VER_A" == "$VER_B" ]]; then
echo "$SUCCESS_MSG" | tee -a $GITHUB_STEP_SUMMARY
else
echo "$FAILURE_MSG" | tee -a $GITHUB_STEP_SUMMARY >&2
exit 1
fi
}

assert-match "$ver_zerocopy" "$ver_zerocopy_derive" \
"Same crate version ($ver_zerocopy) found for zerocopy and zerocopy-derive." \
"Different crate versions found for zerocopy ($ver_zerocopy) and zerocopy-derive ($ver_zerocopy_derive)."

# Note the leading `=` sign - the dependency needs to be an exact one.
assert-match "=$ver_zerocopy_derive" "$zerocopy_derive_dep_ver" \
"zerocopy depends upon same version of zerocopy-derive in-tree ($zerocopy_derive_dep_ver)." \
"zerocopy depends upon different version of zerocopy-derive ($zerocopy_derive_dep_ver) than the one in-tree ($ver_zerocopy_derive)."

# Note the leading `=` sign - the dependency needs to be an exact one.
assert-match "=$ver_zerocopy_derive" "$zerocopy_derive_dev_dep_ver" \
"In dev mode, zerocopy depends upon same version of zerocopy-derive in-tree ($zerocopy_derive_dev_dep_ver)." \
"In dev mode, zerocopy depends upon different version of zerocopy-derive ($zerocopy_derive_dev_dep_ver) than the one in-tree ($ver_zerocopy_derive)."

assert-match "$zerocopy_derive_dep_ver" "$zerocopy_derive_targeted_ver" \
"Same crate version ($zerocopy_derive_dep_ver) found for optional and targeted zerocopy-derive dependency." \
"Different crate versions found for optional ($zerocopy_derive_dep_ver) and targeted ($zerocopy_derive_targeted_ver) dependency."
10 changes: 10 additions & 0 deletions githooks/pre-push
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/usr/bin/env bash
set -eo pipefail
echo "Running pre-push git hook: $0"
# forego redirecting stdout to /dev/null on check_fmt.sh because the output
# from cargofmt is useful (and the good stuff is not delivered by stderr)
./ci/check_fmt.sh
./ci/check_job_dependencies.sh > /dev/null
./ci/check_msrv.sh > /dev/null
./ci/check_readme.sh > /dev/null
./ci/check_versions.sh > /dev/null

0 comments on commit bbc228a

Please sign in to comment.