fix(ci): guard check-abi-manifest's empty abidiff_args for bash 3.2 - #1326
Merged
Conversation
abi/libwirelog-1.0.suppr does not exist in this repository, so abidiff_args is
empty in the default configuration -- not an edge case. Expanding an empty array
under `set -u` is an unbound-variable error on bash 3.2; bash 4.4 changed it.
Use ${abidiff_args[@]+"${abidiff_args[@]}"}. Verified on GNU bash 3.2.57 (built
from source with all 57 official patches, the version macOS ships) and on 5.3:
empty passes zero extra arguments rather than one empty string, and populated
passes --suppr and the path as two words, including when the path contains
spaces. The unquoted outer expansion neither globs nor word-splits, because the
inner expansion is quoted.
macOS does not reach this line, and not for the reason first written here:
meson builds libwirelog.dylib there (darwin_versions, meson.build:490), so the
`libwirelog.so not found` SKIP fires two checks earlier -- confirmed with
abidiff present on PATH. macos-latest is also arm64, which the x86_64 guard
stops independently. So this is a defensive fix for a Linux x86_64 host running
bash 3.2, not a latent CI break.
Sweep for the same shape across every script a suite-registered test can reach:
PUBLIC_HEADERS and SENTINELS are literal non-empty assignments, violations in
check-shared-library-closure.sh is already guarded by ((${#violations[@]})), and
check-threading-doc.sh's two sites are fixed in #1320. ${#arr[@]} on an
assigned-empty array is safe at 3.2, so the existing length guards are sound.
Refs #1324
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1324. Based on
main— independent of every other open PR.The bug
abi/libwirelog-1.0.supprdoes not exist in this repository, so the array is empty in thedefault configuration — not an edge case. Expanding an empty array under
set -uis anunbound-variable error on bash 3.2; bash 4.4 changed it.
Same class as #1320 (PR #1325), in a different file, found while reviewing it.
Verified against real bash 3.2.57
Built from the GNU tarball with all 57 official patches — the version macOS ships — and
driven through the real script with a stub
abidiff:The
argccheck is the point: the guard must pass zero extra arguments when empty, notone empty string. The reviewer additionally proved a suppression path containing spaces stays
one argument, and that the unquoted outer
${...+...}neither globs nor word-splits.What this is not
It is not a latent macOS CI break, and my first version of this comment said it was. I
wrote that libabigail's absence on macOS is what stops the line being reached. That names the
wrong guard. meson builds
libwirelog.dylibthere (darwin_versions,meson.build:490), sothe
libwirelog.so not foundSKIP fires two checks earlier — confirmed by running theunfixed script against a macOS-shaped build directory with
abidiffpresent on PATH.macos-latestis also arm64, which thex86_64guard stops independently.So the live exposure is a Linux x86_64 host running bash 3.2, which is near-nobody. This is a
defensive fix that removes an incidental dependency on guards that happen to fire first — not
something to prioritise as though a
brew installwould trip it. The issue body has beencorrected the same way.
Sweep
Required by the issue's acceptance criteria, done as a runtime check rather than a grep: every
suite-registered script parse-checked under 3.2.57 (zero failures), and every
"${arr[@]}"expansion assessed.
PUBLIC_HEADERSandSENTINELSare literal non-empty assignments;violationsincheck-shared-library-closure.shis already guarded by((${#violations[@]}));check-threading-doc.sh's two sites are PR #1325.${#arr[@]}on an assigned-empty array issafe at 3.2, so the existing length guards are sound.
My sweep method was incomplete and the reviewer's was not. Grepping
tests/meson.buildfinds only meson-registered scripts; two of my hits were comment prose, and registered tests
transitively execute several scripts that appear in no meson file. The reviewer swept all 52
scripts under
scripts/, and additionally tested${arr[*]}— a shape I never considered,which fails identically on 3.2 (six sites, all safe). The conclusion held; the derivation
didn't. Both are recorded in #1321, whose acceptance criteria specified the same incomplete
derivation.
Local: 304 Ok / 0 Fail / 12 Skipped, serialized, dedicated build dir.