From 567efd4acd7d0392f3fb3d431ba827490cfa834d Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Tue, 25 Apr 2023 19:47:35 +0200 Subject: [PATCH 1/6] build: run clippy for powerset of features This will catch compiler & clippy warnings in all feature combinations. We should probably use cargo hack for build and test as well, but, that's quite expensive and would add to overall CI wait times. obsoletes https://github.com/neondatabase/neon/pull/4073 refs https://github.com/neondatabase/neon/pull/4070 --- .github/workflows/build_and_test.yml | 14 ++++++++++++-- run_clippy.sh | 5 +++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 3212b7673119..6eba83058203 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -111,8 +111,18 @@ jobs: - name: Get postgres headers run: make postgres-headers -j$(nproc) - - name: Run cargo clippy - run: ./run_clippy.sh + - uses: taiki-e/install-action@cargo-hack + + # cargo hack runs the given cargo subcommand (clippy in this case) for all feature combinations. + # This will catch compiler & clippy warnings in all feature combinations. + # TODO: use cargo hack for build and test as well, but, that's quite expensive. + # NB: keep clippy args in sync with ./run_clippy.sh + - run: | + echo "CLIPPY_COMMON_ARGS=--locked --workspace --all-targets -- -A unknown_lints -D warning" >> $GITHUB_ENV + - name: Run cargo clippy (debug) + run: cargo hack --feature-powerset clippy $CLIPPY_COMMON_ARGS + - name: Run cargo clippy (release) + run: cargo hack --feature-powerset clippy $CLIPPY_COMMON_ARGS --release # Use `${{ !cancelled() }}` to run quck tests after the longer clippy run - name: Check formatting diff --git a/run_clippy.sh b/run_clippy.sh index 9adfddedc2a2..bb762bc62bf8 100755 --- a/run_clippy.sh +++ b/run_clippy.sh @@ -11,4 +11,9 @@ # * `-A unknown_lints` – do not warn about unknown lint suppressions # that people with newer toolchains might use # * `-D warnings` - fail on any warnings (`cargo` returns non-zero exit status) + +# NB: the CI runs the full feature powerset, so, it catches slightly more errors +# at the expense of longer runtime. This script is used by developers, so, don't +# do that here. +# NB: keep the args after the `--` in sync with the CI YAMLs. cargo clippy --locked --all --all-targets --all-features -- -A unknown_lints -D warnings From b1c879ab4d1918b7fcdafb1953cda2585ef0cb2e Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 26 Apr 2023 15:48:20 +0200 Subject: [PATCH 2/6] invent a config file for clippy to store the common arguments --- .github/workflows/build_and_test.yml | 7 ++++++- run_clippy.sh | 14 +++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 6eba83058203..be288c2fc75a 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -118,7 +118,12 @@ jobs: # TODO: use cargo hack for build and test as well, but, that's quite expensive. # NB: keep clippy args in sync with ./run_clippy.sh - run: | - echo "CLIPPY_COMMON_ARGS=--locked --workspace --all-targets -- -A unknown_lints -D warning" >> $GITHUB_ENV + CLIPPY_COMMON_ARGS="$( source .neon_clippy_args; echo "$CLIPPY_COMMON_ARGS")" + if [ "$CLIPPY_COMMON_ARGS" = "" ]; then + echo "No clippy args found in .neon_clippy_args" + exit 1 + fi + echo "CLIPPY_COMMON_ARGS=${CLIPPY_COMMON_ARGS}" >> $GITHUB_ENV - name: Run cargo clippy (debug) run: cargo hack --feature-powerset clippy $CLIPPY_COMMON_ARGS - name: Run cargo clippy (release) diff --git a/run_clippy.sh b/run_clippy.sh index bb762bc62bf8..ae2a17ec0c43 100755 --- a/run_clippy.sh +++ b/run_clippy.sh @@ -1,4 +1,5 @@ -#!/bin/bash +#!/usr/bin/env bash +set -euo pipefail # If you save this in your path under the name "cargo-zclippy" (or whatever # name you like), then you can run it as "cargo zclippy" from the shell prompt. @@ -8,12 +9,11 @@ # warnings and errors right in the editor. # In vscode, this setting is Rust-analyzer>Check On Save:Command -# * `-A unknown_lints` – do not warn about unknown lint suppressions -# that people with newer toolchains might use -# * `-D warnings` - fail on any warnings (`cargo` returns non-zero exit status) - # NB: the CI runs the full feature powerset, so, it catches slightly more errors # at the expense of longer runtime. This script is used by developers, so, don't # do that here. -# NB: keep the args after the `--` in sync with the CI YAMLs. -cargo clippy --locked --all --all-targets --all-features -- -A unknown_lints -D warnings + +thisscript="${BASH_SOURCE[0]}" +thisscript_dir="$(dirname "$thisscript")" +CLIPPY_COMMON_ARGS="$( source .neon_clippy_args; echo "$CLIPPY_COMMON_ARGS")" +exec cargo clippy --all-features $CLIPPY_COMMON_ARGS From ba7a9c053d1e076aa28c6c6049b695980ef33a3a Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 26 Apr 2023 15:52:47 +0200 Subject: [PATCH 3/6] use rust image that has cargo-hack pre-installed from https://github.com/neondatabase/build/pull/53 --- .github/workflows/build_and_test.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index be288c2fc75a..0f7774c00722 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -86,7 +86,9 @@ jobs: check-codestyle-rust: runs-on: [ self-hosted, gen3, large ] container: - image: 369495373322.dkr.ecr.eu-central-1.amazonaws.com/rust:pinned + # test with image from https://github.com/neondatabase/build/actions/runs/4809401522/jobs/8560593772 + #image: 369495373322.dkr.ecr.eu-central-1.amazonaws.com/rust:pinned + image: 369495373322.dkr.ecr.eu-central-1.amazonaws.com/rust:4809401522-amd64 options: --init steps: @@ -111,8 +113,6 @@ jobs: - name: Get postgres headers run: make postgres-headers -j$(nproc) - - uses: taiki-e/install-action@cargo-hack - # cargo hack runs the given cargo subcommand (clippy in this case) for all feature combinations. # This will catch compiler & clippy warnings in all feature combinations. # TODO: use cargo hack for build and test as well, but, that's quite expensive. From 34ebbd39527ecdcbd526ed2b36028a87606b40d9 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 26 Apr 2023 15:56:07 +0200 Subject: [PATCH 4/6] fixup 'invent a config...': forgot to git-add it --- .neon_clippy_args | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .neon_clippy_args diff --git a/.neon_clippy_args b/.neon_clippy_args new file mode 100644 index 000000000000..25e09c61a67d --- /dev/null +++ b/.neon_clippy_args @@ -0,0 +1,4 @@ +# * `-A unknown_lints` – do not warn about unknown lint suppressions +# that people with newer toolchains might use +# * `-D warnings` - fail on any warnings (`cargo` returns non-zero exit status) +export CLIPPY_COMMON_ARGS="--locked --workspace --all-targets -- -A unknown_lints -D warnings" From fbc9e8c5d564032d0a6d6b24401752c756c34206 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 26 Apr 2023 16:02:16 +0200 Subject: [PATCH 5/6] fix the position of the `--release` arg before this patch it looked like this, i.e., --release after the -- cargo hack --feature-powerset clippy --locked --workspace --all-targets -- -A unknown_lints -D warnings --release --- .github/workflows/build_and_test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 0f7774c00722..1593759bd2ef 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -127,7 +127,7 @@ jobs: - name: Run cargo clippy (debug) run: cargo hack --feature-powerset clippy $CLIPPY_COMMON_ARGS - name: Run cargo clippy (release) - run: cargo hack --feature-powerset clippy $CLIPPY_COMMON_ARGS --release + run: cargo hack --feature-powerset clippy --release $CLIPPY_COMMON_ARGS # Use `${{ !cancelled() }}` to run quck tests after the longer clippy run - name: Check formatting From 5216386b414525adc40b8c556ed814e235687be5 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 26 Apr 2023 17:12:20 +0200 Subject: [PATCH 6/6] Revert "use rust image that has cargo-hack pre-installed" https://github.com/neondatabase/build/pull/53 is now merged and the rust:pinned now includes cargo-hack. This partially reverts commit ba7a9c053d1e076aa28c6c6049b695980ef33a3a. --- .github/workflows/build_and_test.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 1593759bd2ef..0d28cc1416cb 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -86,9 +86,7 @@ jobs: check-codestyle-rust: runs-on: [ self-hosted, gen3, large ] container: - # test with image from https://github.com/neondatabase/build/actions/runs/4809401522/jobs/8560593772 - #image: 369495373322.dkr.ecr.eu-central-1.amazonaws.com/rust:pinned - image: 369495373322.dkr.ecr.eu-central-1.amazonaws.com/rust:4809401522-amd64 + image: 369495373322.dkr.ecr.eu-central-1.amazonaws.com/rust:pinned options: --init steps: