Make benchmark failures block merges and explain the fix

.github/scripts/check-benchmarks.sh

@@ -0,0 +1,7 @@
+#!/bin/bash
+# The benchmarks-ok check. Passes when every benchmark job passed, or when
+# there was nothing to benchmark. Usage: check-benchmarks.sh <changes result> <rake result>
+set -e
+
+echo "changes: $1, rake: $2"
+[ "$1" = success ] && { [ "$2" = success ] || [ "$2" = skipped ]; }

.github/scripts/pick-benchmarks.sh

@@ -0,0 +1,35 @@
+#!/bin/bash
+# Decides what a CI run benchmarks, and writes `run` and `files` to
+# $GITHUB_OUTPUT. Usage: pick-benchmarks.sh <base commit>
+#
+# A pull request runs only the benchmark files it changes, unless it changes
+# something every benchmark depends on. A push to main runs everything, if
+# anything that affects benchmarks changed.
+set -e
+
+base=$1
+
+if ! git cat-file -e "$base^{commit}" 2>/dev/null; then
+  echo "Base $base not found, running every benchmark"
+  echo "run=true" >> "$GITHUB_OUTPUT"
+  exit 0
+fi
+
+changed=$(git diff --name-only "$base" HEAD)
+files=$(git diff --name-only --diff-filter=d "$base" HEAD -- 'code/**/*.rb' | tr '\n' ' ')
+
+if echo "$changed" | grep -qE '^(Gemfile|Rakefile|compose\.yaml|docker/|\.github/workflows/benchmarks\.yml|\.github/scripts/)'; then
+  echo "Shared files changed, running every benchmark"
+  echo "run=true" >> "$GITHUB_OUTPUT"
+elif [ -z "$files" ]; then
+  # Docs only, or a PR that only deletes a benchmark.
+  echo "No benchmark to run"
+  echo "run=false" >> "$GITHUB_OUTPUT"
+elif [ "$GITHUB_EVENT_NAME" = pull_request ]; then
+  echo "Running: $files"
+  echo "run=true" >> "$GITHUB_OUTPUT"
+  echo "files=$files" >> "$GITHUB_OUTPUT"
+else
+  echo "Benchmarks changed, running every benchmark"
+  echo "run=true" >> "$GITHUB_OUTPUT"
+fi

.github/scripts/report-failures.sh

@@ -0,0 +1,27 @@
+#!/bin/bash
+# After a failed benchmark job: one annotation per failed benchmark, shown on
+# the PR's changed files, and a list on the run's summary page.
+# Usage: report-failures.sh <log file> <ruby service>
+set -e
+
+log=$1
+ruby=$2
+
+failed=$(sed -n '/^Failed benchmarks:/,$p' "$log" | grep -E '^code/.+\.rb$' || true)
+[ -n "$failed" ] || exit 0
+
+hint='If it needs a newer Ruby, make it skip older ones, see "Benchmarks that need a newer Ruby" in CONTRIBUTING.md.'
+
+{
+  echo "### Benchmarks that failed on $ruby"
+  echo
+  echo "$failed" | while read -r file; do echo "- \`$file\`"; done
+  echo
+  echo "$hint"
+} >> "$GITHUB_STEP_SUMMARY"
+
+echo "$failed" | while read -r file; do
+  # Annotation properties cannot contain raw % , or :
+  escaped=$(printf '%s' "$file" | sed 's/%/%25/g; s/,/%2C/g; s/:/%3A/g')
+  echo "::error file=$escaped,title=Fails on $ruby::Crashed on $ruby. $hint"
+done

.github/scripts/run-benchmarks.sh

@@ -0,0 +1,11 @@
+#!/bin/bash
+# Runs one Ruby's benchmark job and keeps its output in benchmarks.log for
+# report-failures.sh. Usage: FILES="..." run-benchmarks.sh <ruby service>
+# Without FILES, every benchmark runs.
+set -eo pipefail
+
+# Unquoted on purpose: one argument per file. set -f keeps names like
+# dig-vs-[]-vs-fetch.rb from being read as glob patterns.
+set -f
+# shellcheck disable=SC2086
+docker compose run --rm -T "$1" $FILES 2>&1 | tee benchmarks.log

.github/workflows/benchmarks.yml

@@ -1,22 +1,32 @@
 name: Benchmarks
 
-# Only changes that can affect a benchmark run it: README-only changes do not.
+# Always runs, so the benchmarks-ok check at the end always reports and can be
+# required. The changes job decides whether there is anything to benchmark.
 on:
   push:
     branches: [ main ]
-    paths: &benchmark_paths
-      - 'code/**/*.rb'
-      - 'Gemfile'
-      - 'Rakefile'
-      - 'compose.yaml'
-      - 'docker/**'
-      - '.github/workflows/benchmarks.yml'
   pull_request:
     branches: [ main ]
-    paths: *benchmark_paths
 
 jobs:
+  changes:
+    runs-on: ubuntu-latest
+    outputs:
+      run: ${{ steps.pick.outputs.run }}
+      files: ${{ steps.pick.outputs.files }}
+
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+      - name: Pick benchmarks
+        id: pick
+        # A pull_request checks out a merge commit: HEAD^1 is the base.
+        run: .github/scripts/pick-benchmarks.sh "${{ github.event_name == 'pull_request' && 'HEAD^1' || github.event.before }}"
+
   rake:
+    needs: changes
+    if: needs.changes.outputs.run == 'true'
     runs-on: ubuntu-latest
     timeout-minutes: 60
 
@@ -38,36 +48,22 @@ jobs:
         run: |
           echo "SHARE=1" >> "$GITHUB_ENV"
       - uses: actions/checkout@v4
-        with:
-          # A pull_request checks out a merge commit: HEAD^1 is the base.
-          fetch-depth: 2
-      # On a pull request, run only the benchmark files it changes, unless it
-      # changes something every benchmark depends on. Pushes to main run all.
-      - name: Pick benchmarks
-        id: pick
-        if: github.event_name == 'pull_request'
-        run: |
-          changed=$(git diff --name-only HEAD^1 HEAD)
-          if echo "$changed" | grep -qE '^(Gemfile|Rakefile|compose\.yaml|docker/|\.github/workflows/benchmarks\.yml)'; then
-            echo "Shared files changed, running every benchmark"
-          else
-            files=$(git diff --name-only --diff-filter=d HEAD^1 HEAD -- 'code/**/*.rb' | tr '\n' ' ')
-            if [ -z "$files" ]; then
-              # For example a PR that only deletes a benchmark.
-              echo "No benchmark to run"
-              echo "skip=true" >> "$GITHUB_OUTPUT"
-            else
-              echo "Running: $files"
-              echo "files=$files" >> "$GITHUB_OUTPUT"
-            fi
-          fi
       - name: Run benchmarks on ${{ matrix.ruby }}
-        if: steps.pick.outputs.skip != 'true'
         env:
-          FILES: ${{ steps.pick.outputs.files }}
-        run: |
-          # Unquoted on purpose: one argument per file. set -f keeps names like
-          # dig-vs-[]-vs-fetch.rb from being read as glob patterns.
-          set -f
-          # shellcheck disable=SC2086
-          docker compose run --rm -T ${{ matrix.ruby }} $FILES
+          FILES: ${{ needs.changes.outputs.files }}
+        run: .github/scripts/run-benchmarks.sh ${{ matrix.ruby }}
+      - name: Report failed benchmarks
+        if: failure()
+        run: .github/scripts/report-failures.sh benchmarks.log ${{ matrix.ruby }}
+
+  # The check to require on main. Passes when every benchmark job passed, or
+  # when there was nothing to benchmark.
+  benchmarks-ok:
+    needs: [changes, rake]
+    if: always()
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@v4
+      - name: Check the benchmark jobs
+        run: .github/scripts/check-benchmarks.sh ${{ needs.changes.result }} ${{ needs.rake.result }}

CONTRIBUTING.md

@@ -8,6 +8,13 @@ These idioms list here are trying to satisfy following goals:
 
 [![GOALS](/images/Goals.png)](https://speakerdeck.com/sferik/writing-fast-ruby?slide=11)
 
+## Contents
+
+- [Note on entry](#note-on-entry)
+- [Running it on other Rubies](#running-it-on-other-rubies)
+- [Benchmarks that need a newer Ruby](#benchmarks-that-need-a-newer-ruby)
+- [License](#license)
+
 ## Note on entry
 
 Fast code first.
@@ -31,9 +38,11 @@ end
 Run your result:
 
 ```
-$ ruby -v code/your-new/entry.rb
+ruby -v code/your-new/entry.rb
 ```
 
+## Running it on other Rubies
+
 To run it on a Ruby you don't have installed, use Docker. There is one service
 per Ruby in the CI matrix (see `compose.yaml`):
 
@@ -51,12 +60,42 @@ the head builds go stale. To get the latest nightly build:
 docker compose build --no-cache ruby_head
 ```
 
+## Benchmarks that need a newer Ruby
+
+CI runs every benchmark on every Ruby in `compose.yaml`, back to Ruby 2.1, and
+fails when one crashes. If your entry uses something older Rubies do not have,
+make it skip them.
+
+Skip one report, so the rest still run everywhere:
+
+```ruby
+Benchmark.ips do |x|
+  x.report('String#delete_suffix') { fast } if RUBY_VERSION >= '2.5.0'
+  x.report('String#sub')           { slow }
+  x.compare!
+end
+```
+
+Skip the whole file when nothing in it makes sense without the feature:
+
+```ruby
+if RUBY_VERSION >= '2.5.0'
+  # everything, including Benchmark.ips
+end
+```
+
+New syntax (for example `<<~` before 2.3) cannot be skipped this way: older
+Rubies fail to parse the file before the `if` runs. Write it with syntax they
+understand instead.
+
+To check an entry on an older Ruby, see [Running it on other Rubies](#running-it-on-other-rubies).
+
+## License
+
 Thanks in advance!!! Look forward to learning more from you!
 
 <3 [JuanitoFatas](https://twitter.com/juanitofatas)
 
-###### License
-
 <small>The documentation is [CC BY-SA 4.0 (International)](https://github.com/JuanitoFatas/fast-ruby#license).</small>
 
 <small>And code will be [CC0 1.0 Universal](https://github.com/JuanitoFatas/fast-ruby#code-license).</small>

Rakefile

@@ -12,7 +12,11 @@ task :run_benchmark do
     failed << benchmark unless system("ruby", "-v", "-W0", benchmark)
   end
 
-  abort "Failed benchmarks:\n#{failed.join("\n")}" unless failed.empty?
+  unless failed.empty?
+    abort "Failed benchmarks:\n#{failed.join("\n")}\n\n" \
+          "If a benchmark needs a newer Ruby, make it skip older ones, see " \
+          "\"Benchmarks that need a newer Ruby\" in CONTRIBUTING.md."
+  end
 end
 
 task default: :run_benchmark

docker/run-benchmarks.sh

@@ -23,5 +23,8 @@ done
 
 if [ -n "$failed" ]; then
   echo "Failed benchmarks:$failed" >&2
+  echo >&2
+  echo "If a benchmark needs a newer Ruby, make it skip older ones, see" \
+    "\"Benchmarks that need a newer Ruby\" in CONTRIBUTING.md." >&2
   exit 1
 fi