Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions .ci/scripts/abi_baseline_key.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
#!/bin/bash
#===============================================================================
# Copyright contributors to the oneDAL project
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#===============================================================================

# Chooses the cached main build that the ABI check compares against and writes
# its cache key to GITHUB_OUTPUT.
#
# Usage: abi_baseline_key.sh <base-commit> [window] [stale-after]
#
# Three things the baseline has to satisfy that "newest entry in main's cache
# scope" does not:
#
# Scope. `gh cache list` reports every scope in the repository while
# `cache/restore` can only read main's, so an entry belonging to another pull
# request's scope would miss and the comparison would run against an empty
# directory.
#
# Provenance. Any job whose GITHUB_REF is main can write into main's cache

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any job whose GITHUB_REF is main can write into main's cache scope, the fork pull request jobs in Nightly-test included

This no longer matches GitHub's documented behaviour. Runs triggered by workflow_run (and pull_request_target, issue_comment) get read-only access to the default branch's cache unless the workflow declares a write-capable cache-mode. Only push, workflow_dispatch, repository_dispatch, schedule, delete, registry_package and page_build can write to it (dependency caching → Cache access for low-trust workflow triggers). CodeQL's cache-poisoning query models the same split: it flags nightly-test.yml only for its workflow_dispatch trigger.

Deriving keys from commits is still the right design, so no code change is needed, but the comment should say what it actually defends against. Also, "keys are immutable, so the only race left is against the real main build" does not cover keys for older main commits whose entries have expired (7-day TTL) or been evicted. Whoever can write could plant one of those, and the walk would pick it if nothing newer is cached. The impact is bounded either way, since the restored tree only feeds abidiff as data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

# scope, the fork pull request jobs in Nightly-test included, and a key that did
# not exist before is always the newest. So candidates are derived from commits
# rather than read off the cache listing. Keys are immutable, which leaves only
# a race against the genuine main build for a real commit's key.
#
# Ancestry. abidiff reports the difference between two trees, not what one pull
# request changed. A baseline that is not an ancestor of the build under test
# puts main's own changes into the report inverted -- and puts a change the pull
# request shares with one of them into the report as no change at all, which is
# how a real break passes. So start from the base commit the build contains, not
# from main's tip, and fall back to the tip only when that base has no cached
# build left, with a warning that the comparison is no longer exact.

set -eo pipefail

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, AGENTS.md asks for set -euo pipefail and I shipped -eo. Changed in c8d0188.

The script was already written to survive -u -- the optional inputs use ${1:-} and ${GITHUB_OUTPUT:-} -- so the only behaviour change is the one you point at: with GITHUB_REPOSITORY unset the lookup now dies with GITHUB_REPOSITORY: unbound variable at the point of use instead of requesting repos//commits?sha=.... Verified by running the script with that variable unset.

Worth noting the other three scripts under .ci/ (build.sh, env/openblas.sh, env/tbb.sh) are all on set -eo pipefail. Leaving them alone here since adding -u to them is not this PR's subject.


BASE_SHA=${1:-}
WINDOW=${2:-20}
STALE_AFTER=${3:-5}
KEY_PREFIX=__release_lnx

available=$(gh cache list --ref refs/heads/main --key "${KEY_PREFIX}" --limit 100 --json key --jq '.[].key')

# Echoes "<key> <commits skipped>" for the newest cached commit reachable from
# $1, or returns 1 if none of the first ${WINDOW} of them has an entry.
newest_cached_from() {
local behind=0 sha
for sha in $(gh api "repos/${GITHUB_REPOSITORY}/commits?sha=$1&per_page=${WINDOW}" --jq '.[].sha'); do
if grep -qxF "${KEY_PREFIX}-${sha}" <<< "${available}"; then
echo "${KEY_PREFIX}-${sha} ${behind}"
return 0
fi
behind=$((behind + 1))
done
return 1
}

result=""
exact=yes
if [ -n "${BASE_SHA}" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty BASE_SHA silently falls through to the "no cached main build for this pull request's base" warning. That message is wrong for this case, and it is the reason the bug above passed CI unnoticed. In pull_request runs an empty base is always a wiring bug, not a cache gap. Suggest failing fast instead:

if [ -z "${BASE_SHA}" ]; then
    echo "::error::No base commit was passed. The caller must pass the first parent of the merge commit under test."
    exit 1
fi

(This would change the "non-merge HEAD falls back and warns" case from the validation list. If that case is still wanted, it should at least print its own message.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

result=$(newest_cached_from "${BASE_SHA}") || result=""
fi

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and it was worse than a miss: for sha in $(gh api ...) iterates zero times when the lookup fails, and set -e does not catch a failed command substitution in a for list. So a 502 looked exactly like "no cached ancestor" and the fallback picked main's tip -- the non-ancestor baseline this whole change exists to avoid.

Fixed in c8d0188 by taking the API calls out of the loop. ancestors_of is now assigned to a variable at the top level, so a failure is a distinct status the caller stops on:

base_commits=$(ancestors_of "${BASE_SHA}") || {
    echo "::error::Could not list the commits reachable from ${BASE_SHA}. Rerun this job."
    exit 1
}

newest_cached_in is pure string matching now, so "no candidates" can only mean no candidates. Added three tries with a 0/5/10s backoff so a single hiccup does not turn into a red check.

The cache listing keeps carrying on past a failure, and that asymmetry is deliberate: an empty listing cannot match any key, so the job stops at the existing "No __release_lnx cache on refs/heads/main" error instead of comparing against nothing. Failing there is safe; failing in the commit lookup was not. Both directions are covered by the offline dry run -- an injected 502 on the base lookup now exits 1 where it previously printed exact: no and exited 0.

if [ -z "${result}" ]; then
exact=no
result=$(newest_cached_from main) || {
echo "::error::No ${KEY_PREFIX} cache on refs/heads/main for any of its last ${WINDOW} commits. Rerun the 'CI' workflow on main via workflow dispatch to regenerate it."
exit 1
}
fi
read -r cache_key behind <<< "${result}"

if [ "${exact}" = no ]; then
echo "::warning::No cached main build for this pull request's base or its last ${WINDOW} ancestors, so the baseline is main's tip. Changes merged into main since the base will show up in this check, and a change this pull request shares with one of them will not show up at all. Rebase onto main for an exact comparison."
elif [ "${behind}" -gt "${STALE_AFTER}" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description says the diff will contain "only the pull request's own changes: no spurious failures from drift, and no masking either". That holds only when the match is 0 commits back. If the base itself is not cached, the walk picks an older ancestor. The diff then also contains the main commits in between, and a pull request that undoes one of them is hidden. I reproduced this with abidiff (libabigail from Ubuntu 24.04):

  • M0: exports f
  • M1 = M0 + adds S (an addition, so no label needed)
  • PR on M1 removes S
abidiff libM1.so libPR.so  -> "1 Removed function: int S()"   rc=12  (abi_check.sh fails)
abidiff libM0.so libPR.so  -> (no output)                      rc=0   (abi_check.sh passes)

This window comes up regularly: on current main, the __release_lnx-<sha> entry is created about 70 minutes after the commit lands (51b69f2: committed 13:25Z, cached 14:34Z; 82acef1: 16:39Z → 17:49Z). Any PR run in that window matches 1 commit back, and with STALE_AFTER=5 it gets no warning at all.

Suggestions, in order of preference:

  • treat behind > 0 as ::error:: ("baseline for is not cached yet, the main CI run is probably still in progress; re-run this job later"), or
  • at least default STALE_AFTER to 0, so any inexact match is visible.

The description should also be adjusted to say that the masking guarantee only holds for an exact match.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

echo "::warning::Baseline is ${behind} commits behind this pull request's base. Changes merged into main in between will show up in this check."
fi

echo "${cache_key} (exact: ${exact}, ${behind} commits back)"
if [ -n "${GITHUB_OUTPUT:-}" ]; then
echo "key=${cache_key}" >> "${GITHUB_OUTPUT}"
fi
3 changes: 3 additions & 0 deletions .github/workflows/ci-win.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ on:
- main
- 'feature/**'

permissions:
contents: read

env:
DEP_DIR: ${{ github.workspace }}/__deps
TBB_VERSION: v2023.1.0
Expand Down
18 changes: 5 additions & 13 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -172,19 +172,11 @@ jobs:
- name: Check Cache
id: get-cache-key
run: |
# `gh cache list` reports every scope in the repository, not only the
# ones this run can restore from. Without `--ref` the newest matching
# key can belong to another pull request's scope, which `cache/restore`
# cannot read: the restore then misses silently and the check below
# compares against an empty directory. Only main-scoped entries are
# both restorable here and produced by a build of main.
CACHE_KEY=$(gh cache list --ref refs/heads/main --sort created_at --limit 1 --key __release_lnx --json key --jq '.[0].key')
if [ -z "${CACHE_KEY}" ] || [ "${CACHE_KEY}" = "null" ]; then
echo "::error::No __release_lnx cache found on refs/heads/main. Rerun the 'CI' workflow on main via workflow dispatch to regenerate it."
exit 1
fi
echo "key=${CACHE_KEY}" >> "$GITHUB_OUTPUT"
echo $CACHE_KEY
# `checkout` leaves the pull request's merge commit at HEAD, and its
# first parent is the main commit this build contains. That commit, not
# main's tip, is the baseline the comparison has to start from; see the
# script for why.
.ci/scripts/abi_baseline_key.sh "$(git log -1 --format=%P | cut -d' ' -f1)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actions/checkout fetches with --depth=1 (this run's log: fetch --depth=1 origin +8198e87…:refs/remotes/pull/3838/merge). In a shallow clone git treats the boundary commit as parentless, so git log -1 --format=%P prints an empty string and the script is called with "".

I reproduced this with the same fetch as the job:

$ git fetch --depth=1 origin +refs/pull/3838/merge:refs/remotes/pull/3838/merge && git checkout --detach refs/remotes/pull/3838/merge
$ git log -1 --format=%P                               # -> ""   (what the PR passes)
$ git rev-parse HEAD^1                                 # -> fails, "HEAD^1"
$ git cat-file -p HEAD | awk '/^parent /{print $2; exit}'   # -> 82acef1606a33d34cfc22116267d73eab4d2f579

Then I ran the script from this PR against the live cache listing with each input:

abi_baseline_key.sh ""        -> __release_lnx-51b69f23… (exact: no, 0 commits back)  + fallback warning
abi_baseline_key.sh 82acef16… -> __release_lnx-82acef16… (exact: yes, 0 commits back)

The commit object stores its parents even when the clone is shallow, so reading it directly works:

Suggested change
.ci/scripts/abi_baseline_key.sh "$(git log -1 --format=%P | cut -d' ' -f1)"
.ci/scripts/abi_baseline_key.sh "$(git cat-file -p HEAD | awk '/^parent /{print $2; exit}')"

${{ github.event.pull_request.base.sha }} would also work: for this run it equals the merge commit's first parent (82acef16…), and re-runs reuse the same payload. cat-file has the advantage of reading the exact commit that was built.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

env:
GH_TOKEN: ${{ github.token }}
- name: Download oneDAL main build
Expand Down
Loading