Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
121 changes: 121 additions & 0 deletions .ci/scripts/abi_baseline_key.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
#!/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]
#
# Taking the newest entry in main's cache scope is wrong for three reasons.
#
# Scope. `gh cache list` shows every scope in the repository, but
# `cache/restore` can only read main's. An entry from another pull request's
# scope would miss, and the comparison would then 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 one. So candidates come from commits
# rather than from the cache listing. Keys are immutable, so the only race left
# is against the real main build for a real commit's key.
#
# Ancestry. abidiff compares two trees; it does not report what a pull request
# changed. When the baseline is not an ancestor of the build under test, main's
# own changes show up in the report inverted, and a change the pull request
# shares with one of them does not show up at all -- that is how a real break
# passes. So start from the base commit the build contains. Fall back to main's
# tip only when that base has no cached build left, and say that the comparison
# is no longer exact.

set -euo pipefail

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

# Runs "$@" up to three times, backing off in between, and echoes its output, so
# one API hiccup does not get to decide which baseline the check uses.
retry() {
local delay out
for delay in 0 5 10; do
sleep "${delay}"
if out=$("$@"); then
echo "${out}"
return 0
fi
done
return 1
}

# A failure to list the caches is safe to carry on from: no key can match, so
# the job stops at the hard error below rather than comparing against nothing.
available=$(retry gh cache list --ref refs/heads/main --key "${KEY_PREFIX}" --limit 100 --json key --jq '.[].key') || available=""

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.

If gh cache list fails three times, available="" and the job ends with "No __release_lnx cache on refs/heads/main … Rerun the 'CI' workflow on main via workflow dispatch". Failing is correct, but that advice is wrong: re-running main's CI won't fix an API outage. Checked with a stubbed gh that returns HTTP 502 for cache:

HTTP 502: Bad Gateway   (x3)
::error::No __release_lnx cache on refs/heads/main for any of its last 20 commits. Rerun the 'CI' workflow on main via workflow dispatch to regenerate it.

Suggest giving the listing failure its own ::error::Could not list caches … rerun this job, the same way ancestors_of failures are handled.

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.


# A failure here is not safe to carry on from. It yields no candidates, which is
# indistinguishable from "no cached ancestor" and would quietly pick the inexact
# baseline, so both callers stop the job.
ancestors_of() {
retry gh api "repos/${GITHUB_REPOSITORY}/commits?sha=$1&per_page=${WINDOW}" --jq '.[].sha'
}

# Echoes "<key> <commits skipped>" for the newest commit in $1 that has an
# entry, or returns 1 if none of them does.
newest_cached_in() {
local behind=0 sha
for sha in $1; 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.

base_commits=$(ancestors_of "${BASE_SHA}") || {
echo "::error::Could not list the commits reachable from ${BASE_SHA}. Rerun this job."
exit 1
}
result=$(newest_cached_in "${base_commits}") || result=""
fi
if [ -z "${result}" ]; then
exact=no
main_commits=$(ancestors_of main) || {
echo "::error::Could not list the commits on refs/heads/main. Rerun this job."
exit 1
}
result=$(newest_cached_in "${main_commits}") || {
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
17 changes: 4 additions & 13 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -172,19 +172,10 @@ 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; its first
# parent is the main commit this build contains. That commit is the
# baseline, not main's tip -- the script explains 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