Skip to content

Fail the ABI check when abidiff cannot run - #3511

Open
justsmth wants to merge 1 commit into
aws:mainfrom
justsmth:abidiff-fail-on-tool-error
Open

Fail the ABI check when abidiff cannot run#3511
justsmth wants to merge 1 commit into
aws:mainfrom
justsmth:abidiff-fail-on-tool-error

Conversation

@justsmth

@justsmth justsmth commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Context and motivation

diff.sh tested abidiff's exit status with [[ $? -ge 4 ]]. That status is a bitmask, not an ordered severity, so exit 1 (ABIDIFF_ERROR) and 2 (ABIDIFF_USAGE_ERROR) fell through to exit 0: a run that never completed was reported as a clean ABI.

Description of changes

Capture the status and test the bits. Tool and usage errors now fail with a message on stderr. The ABI-change path (bits 4 and 8) is unchanged, so what the incremental and release checks report today is unaffected.

Testing

Stubbed abidiff and ran the real script across every documented exit code:

rc before after message
0 pass pass -
1, 2, 3 pass fail names ABIDIFF_ERROR / ABIDIFF_USAGE_ERROR
4, 8, 12 fail fail names ABIDIFF_ABI_CHANGE / _INCOMPATIBLE_CHANGE
5 fail fail names both the error and the ABI change

Status propagates verbatim, so a failed step keeps abidiff's bitmask rather than flattening to 1. No automated test added, since there is no harness for these container scripts. shellcheck and bash -n are clean.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@justsmth
justsmth requested a review from a team as a code owner September 10, 2026 20:09
@justsmth
justsmth force-pushed the abidiff-fail-on-tool-error branch from ffcb8d1 to 895bc16 Compare September 10, 2026 20:17
@github-actions

Copy link
Copy Markdown
Contributor

🔒 Security ReviewView Report

Please review before merging.

@dougch dougch left a comment

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.

Curious how you found this?

@codecov-commenter

codecov-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.17%. Comparing base (f8f6fb5) to head (14d18e2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3511      +/-   ##
==========================================
- Coverage   78.36%   78.17%   -0.20%     
==========================================
  Files         700      700              
  Lines      125744   125743       -1     
  Branches    17388    17384       -4     
==========================================
- Hits        98540    98298     -242     
- Misses      26332    26576     +244     
+ Partials      872      869       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

abidiff reports its result as a bitmask: 1 ABIDIFF_ERROR, 2
ABIDIFF_USAGE_ERROR, 4 ABIDIFF_ABI_CHANGE, 8
ABIDIFF_ABI_INCOMPATIBLE_CHANGE. The result was tested with
`[[ $? -ge 4 ]]`, which reads the status as an ordered severity rather than a
bitmask, so exit codes 1, 2 and 3 fell through to `exit 0` and the check
passed. A run that never completed was reported as a clean ABI.

No non-zero status is a pass, so fail on any of them and name the bits that
are set. Exit with abidiff's own status rather than 1, so the bitmask survives
into the failed step.
@justsmth
justsmth force-pushed the abidiff-fail-on-tool-error branch from 895bc16 to 14d18e2 Compare September 14, 2026 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants