fix(build,ci): unblock clang packages + arm64 package verification (3.0.9)#5840
Conversation
Two release-blocking bugs surfaced while building 3.0.9/3.1.9 packages: 1. clang builds (16 distro variants) failed at deps build with exit 127. Root cause: vendored ParserSQL's Makefile hardcoded `CXX = g++`, which the `CC=.. CXX=.. make` env prefix does not override, so clang containers (no g++) could not compile src/sql_parser/*.o. Fixed upstream in ParserSQL 1.0.9 (CXX ?= g++); re-vendored the 1.0.9 tarball and repointed the symlink. No change needed to deps/Makefile. 2. arm64 base package builds (14 distros) failed in the new 'Verify package installs' step: verify-package-install.bash used `file` (and `which`), absent in slim base images, so verification aborted after a good build and blocked the upload. Replaced with a portable ELF-magic check (head -c4 + od) and `command -v`.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR updates the vendored parsersql dependency from 1.0.8 to 1.0.9 and refactors the container test script to use portable ELF magic-byte validation instead of relying on the ChangesDependency and test infrastructure updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request bumps the parsersql dependency version and replaces the use of the file and which commands in verify-package-install.bash with a custom, portable is_elf function and command -v. The review feedback suggests improving the robustness of the is_elf function by verifying that the file exists first, and safely handling command -v under set -e to prevent unexpected shell termination.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| # Portable ELF check — minimal base images (debian-slim, etc.) may not ship | ||
| # `file` or `which`, so validate the ELF magic bytes (7f 45 4c 46) directly. | ||
| is_elf() { [ "$(head -c4 "$1" 2>/dev/null | od -An -tx1 | tr -d ' \n')" = "7f454c46" ]; } |
There was a problem hiding this comment.
If is_elf is called with an empty string or a non-existent file path, head will be executed with an empty argument or fail. It is safer and more robust to check if the file exists and is a regular file first using [ -f "${1:-}" ]. Additionally, using tr -d '[:space:]' is more standard and robust than tr -d ' \n' for stripping all whitespace.
| is_elf() { [ "$(head -c4 "$1" 2>/dev/null | od -An -tx1 | tr -d ' \n')" = "7f454c46" ]; } | |
| is_elf() { [ -f "${1:-}" ] && [ "$(head -c4 "$1" 2>/dev/null | od -An -tx1 | tr -d '[:space:]')" = "7f454c46" ]; } |
|
|
||
| # Binary sanity check | ||
| if file "$(which proxysql)" | grep -q 'ELF 64-bit'; then | ||
| if is_elf "$(command -v proxysql)"; then |
There was a problem hiding this comment.
Running command -v inside a command substitution under set -e can sometimes lead to unexpected shell termination depending on the shell version if the command is not found. We can safely perform the assignment and check in the if condition itself, which prevents set -e from triggering on failure and avoids calling is_elf with an empty string.
| if is_elf "$(command -v proxysql)"; then | |
| if PROXYSQL_BIN="$(command -v proxysql 2>/dev/null)" && is_elf "$PROXYSQL_BIN"; then |
Regenerated via git archive of the released v1.0.9 tag (ProxySQL/ParserSQL) for provenance; tree is identical to the prior commit.
|
1 participant
Summary
Two release-blocking bugs found while building the 3.0.9 / 3.1.9 packages (dispatched via the per-distro
CI-package-*workflows). The binaries compile fine in both cases — the failures are a vendored-dep build flag and a CI verification step.1. clang packages (16 distro variants) — exit 127 in deps build
Vendored ParserSQL's Makefile hardcoded
CXX = g++, which aCC=.. CXX=.. makeenv prefix does not override. clang build containers ship only clang/clang++ (no g++), sosrc/sql_parser/*.ofailed withg++: command not found(exit 127). 3.0.8 shipped clang packages, so this is a regression from the ParserSQL vendoring (#5736).Fixed upstream in ParserSQL 1.0.9 (ProxySQL/ParserSQL#45:
CXX ?= g++) and re-vendored here (tarball swap + symlink repoint). No change todeps/Makefile— the existing invocation already passes the right compiler;?=now lets it win.2. arm64 base packages (14 distros) — verify step aborts before upload
The
Verify package installs on clean <distro>step (added in #5815) runsverify-package-install.bash, which usedfile(andwhich) — absent in slim base images (e.g.debian:bookworm-slim), so verification failed withfile: command not foundafter a good build, blocking the upload. Replaced with a portable ELF-magic check (head -c4 | od) andcommand -v.Why it matters for the release
arm64 base packages are required (served from repo.proxysql.com); clang packages shipped in 3.0.8. Both must build for 3.0.9/3.1.9.
Depends on
ProxySQL/ParserSQL#45 (vendored as 1.0.9 here; tarball is self-contained).
Summary by CodeRabbit
Chores
Tests