Add a 32-bit autotools CI job - #489
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe workflow adds 32-bit autoconf build and test variants. The decoder limit test now accepts compiler commands that include flags and uses them for compiler probes and test-library builds. Changes32-bit CI coverage
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to The 32-bit workflow forwards its compiler flags correctly across the inspected build and test consumers, with no identified regression. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. I twitch my nose at flags in flight Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes are complete with no unresolved blocking issues.
Pull request overview
Adds 32-bit Autotools CI coverage to detect i686-specific issues.
Changes:
- Adds GCC and Clang
-m32CI jobs. - Preserves target flags in decoder limit test rebuilds.
File summaries
| File | Summary |
|---|---|
t/decoder_limits_t.pl |
Retains -m target flags when rebuilding the library. |
.github/workflows/test.yml |
Adds the 32-bit GCC/Clang CI matrix. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ssize_t and long are 32 bits wide on i686. That exposes signedness and range problems that the 64-bit jobs cannot see, such as the -Wsign-compare error fixed in #487, which only showed up in Fedora's i686 build. The new job builds and tests with -m32 using gcc and clang. The flags are set for the whole job so that the libtap build, which has its own Makefile, and the test scripts that compile code themselves pick them up. The decoder limits test kept only -f flags from the environment when it rebuilt the library, so it would have dropped -m32 and checked 64-bit code. It now keeps -m flags as well. With the #487 fix reverted, the old script passed at 32-bit and the new one fails with the original error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
e717ee1 to
4a8984e
Compare
| test-autoconf: | ||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-latest, macos-latest] | ||
| cc: [gcc, clang] | ||
| posix: ['', -D_POSIX_C_SOURCE=200112L] | ||
| name: Autotools build on ${{matrix.os}} using ${{matrix.cc}} ${{matrix.posix}} | ||
| runs-on: ${{ matrix.os }} | ||
| env: | ||
| CC: ${{ matrix.cc }} | ||
| VERBOSE: 1 | ||
| steps: | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| submodules: true | ||
| persist-credentials: false | ||
| - run: sudo apt install libipc-run3-perl | ||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| - run: brew install autoconf automake libtool | ||
| if: ${{ matrix.os == 'macos-latest' }} | ||
| - run: ./bootstrap | ||
| - run: ./configure | ||
| env: | ||
| CFLAGS: -std=c99 -Wall -Wextra -Werror -Wno-unused-function -Wno-unused-parameter ${{ matrix.posix }} | ||
| - run: make | ||
| - run: make check |
There was a problem hiding this comment.
Could we do something like this rather than having a separate job?
| - run: make check | |
| test-autoconf: | |
| strategy: | |
| matrix: | |
| os: [ubuntu-latest, macos-latest] | |
| cc: [gcc, clang] | |
| posix: ['', -D_POSIX_C_SOURCE=200112L] | |
| arch: ['', -m32] | |
| exclude: | |
| - os: macos-latest | |
| arch: -m32 | |
| name: Autotools build on ${{matrix.os}} using ${{matrix.cc}} ${{matrix.posix}} ${{matrix.arch}} | |
| runs-on: ${{ matrix.os }} | |
| env: | |
| CC: ${{ matrix.cc }} | |
| CFLAGS: -std=c99 -Wall -Wextra -Werror -Wno-unused-function -Wno-unused-parameter ${{ matrix.posix }} ${{ matrix.arch }} | |
| CXXFLAGS: ${{ matrix.arch }} | |
| LDFLAGS: ${{ matrix.arch }} | |
| VERBOSE: 1 | |
| steps: | |
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | |
| with: | |
| submodules: true | |
| persist-credentials: false | |
| - run: sudo apt-get update && sudo apt-get install -y gcc-multilib g++-multilib libipc-run3-perl | |
| if: ${{ matrix.os == 'ubuntu-latest' }} | |
| - run: brew install autoconf automake libtool | |
| if: ${{ matrix.os == 'macos-latest' }} | |
| - run: ./bootstrap | |
| - run: ./configure | |
| - run: make | |
| - run: make check |
There was a problem hiding this comment.
Hmm, that suggestion didn't work that well, maybe because it was outside the diff.
There was a problem hiding this comment.
One adjustment in 13ae6f1: the warning and posix flags stay on the configure step. Setting them job-wide broke the libtap build on macOS, because _POSIX_C_SOURCE hides MAP_ANONYMOUS there. Only the -m32 flags are job-wide now.
There was a problem hiding this comment.
That did not fix it either. When CFLAGS is in the environment at all, even empty, make exports the configured CFLAGS to the libtap sub-make, so libtap saw _POSIX_C_SOURCE on macOS. 0502f98 passes -m32 through CC="gcc -m32" instead, which reaches configure, libtap, and the test scripts, and keeps CFLAGS and LDFLAGS out of the job environment. Verified locally with a 32-bit build and full make check.
There was a problem hiding this comment.
🟡 Changes recommended
Job-level flags leak the POSIX define into the macOS libtap build, causing that matrix variant to fail.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
0502f98 to
8476f55
Compare
Add an arch dimension to the existing autoconf job instead of keeping a separate 32-bit job. macOS is excluded from the -m32 variant, and the multilib packages install only for the -m32 builds. The flag goes through CC="gcc -m32" rather than CFLAGS and LDFLAGS. CFLAGS must stay out of the job environment: when it is set, even empty, make exports the configured CFLAGS to the libtap sub-make, and libtap does not build on macOS with _POSIX_C_SOURCE defined. CC reaches configure, libtap, and the test scripts. The decoder limits test now splits CC on whitespace and no longer needs to pick -m flags out of CFLAGS. CXXFLAGS stays job-wide for the C++ compile test, which reads it from the environment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8476f55 to
665789d
Compare
horgh
left a comment
There was a problem hiding this comment.
I verified this locally rather than reading it: built and ran the full suite at -m32, confirmed the artifacts are ELF 32-bit, and confirmed that reverting cd138a6 reproduces the exact -Wsign-compare error from #486. The design is right and the motivating bug is genuinely caught. Notes inline are polish, not blockers.
🤖 Comment by Claude (Claude Code) on behalf of Will.
| - cron: '3 20 * * SUN' | ||
| permissions: {} | ||
| jobs: | ||
| # The -m32 variants build and test as 32-bit. ssize_t and long are 32 bits |
There was a problem hiding this comment.
The PR description still describes the approach that 665789d replaced. It says the decoder limits test "now keeps -m flags as well" and that "The flags are set for the whole job", but the final diff does neither: the filter at t/decoder_limits_t.pl:45 is still /^-f/, -m32 arrives through CC, and keeping CFLAGS out of the job env is the whole point of the current design. The verification table also describes runs of the old approach.
The commit message on 665789d is accurate — worth copying over the PR body before merge, since that text is what ends up in the merge record.
(Commenting here because the PR body has no line to anchor to.)
🤖 Comment by Claude (Claude Code) on behalf of Will.
| my $cc = $ENV{CC} || 'cc'; | ||
|
|
||
| # CC may carry flags, such as CC="gcc -m32". | ||
| my @cc = split ' ', $ENV{CC} || 'cc'; |
There was a problem hiding this comment.
t/compile_c++_t.pl:51 still has the exact bug this line fixes:
my $cxx = $ENV{CXX} || 'c++';No split, and $cxx is passed as a single element of the run3 command list. It does not bite today only because the arch flag is routed through CXXFLAGS rather than CXX. But the comment in test.yml argues the flag belongs in the compiler variable, so CXX="g++ -m32" is the natural next edit, and it would silently fail to exec exactly the way CC="gcc -m32" did here.
Worth applying the same split ' ' to both scripts in this PR so the two don't drift.
(The real target is t/compile_c++_t.pl:51, which is outside this diff.)
🤖 Comment by Claude (Claude Code) on behalf of Will.
| # The -m32 variants build and test as 32-bit. ssize_t and long are 32 bits | ||
| # wide on i686, which exposes signedness and range problems that the 64-bit | ||
| # builds cannot see. The flag goes in CC rather than CFLAGS so that the | ||
| # libtap build, which has its own Makefile, and the test scripts that |
There was a problem hiding this comment.
"the test scripts that compile code themselves pick it up too" is only true for decoder_limits_t.pl. The other script that compiles code, t/compile_c++_t.pl, reads $ENV{CXX} and $ENV{CXXFLAGS} and never looks at CC — it gets -m32 from the CXXFLAGS entry below, not from CC. external_symbols_t.pl only runs nm, and mmdblookup_t.pl compiles nothing.
The plural sends a reader looking for a mechanism that doesn't exist, and hides the one that does.
🤖 Comment by Claude (Claude Code) on behalf of Will.
| persist-credentials: false | ||
| - run: sudo apt install libipc-run3-perl | ||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| - run: sudo apt-get update && sudo apt-get install -y gcc-multilib g++-multilib |
There was a problem hiding this comment.
Consider asserting that the artifacts really are 32-bit, near these steps:
- run: file src/.libs/libmaxminddb.so | grep -q 'ELF 32-bit'
if: ${{ matrix.arch == '-m32' }}I checked every current way -m32 could get dropped and they all fail loudly — missing multilib dies at "C compiler cannot create executables", and a 64-bit libtap fails the link with an architecture mismatch. So this is not a present bug.
The risk is later drift: if someone edits the CC line or the matrix, the job keeps its -m32 name, tests 64-bit code, and goes green. The guard is one line and pins down the property the whole job exists for.
(The natural spot is after - run: make, which is outside this diff.)
🤖 Comment by Claude (Claude Code) on behalf of Will.
| env: | ||
| CC: ${{ matrix.cc }} | ||
| CC: ${{ matrix.cc }} ${{ matrix.arch }} | ||
| CXXFLAGS: ${{ matrix.arch }} |
There was a problem hiding this comment.
This entry deserves a sentence in the comment above. It exists solely for t/compile_c++_t.pl:52, which reads $ENV{CXXFLAGS} and would otherwise link a 64-bit C++ binary against the 32-bit library. The reasoning is in 665789d's commit message but didn't make it into the file.
Without it, a reader applying the stated CFLAGS rule symmetrically ("when it is set, even empty...") may well delete this line and silently drop -m32 from the C++ test.
For what it's worth, the CFLAGS hazard genuinely does not apply here: there's no AC_PROG_CXX, CXXFLAGS appears zero times in every generated Makefile, and libtap's Makefile never reads it. So the empty value on the 64-bit rows is inert — but that's exactly the kind of thing worth stating rather than leaving the asymmetry looking accidental.
🤖 Comment by Claude (Claude Code) on behalf of Will.
| my ( $cc_version, $cc_stderr ) = ( q{}, q{} ); | ||
| my $cc_status = eval { | ||
| run3( [ $cc, '--version' ], \undef, \$cc_version, \$cc_stderr ); | ||
| run3( [ @cc, '--version' ], \undef, \$cc_version, \$cc_stderr ); |
There was a problem hiding this comment.
Now that CC can carry flags, this probe has a wider failure surface worth tightening. run3 dies on exec failure rather than returning a status, so $cc_status comes back undef and the file hits skip_all at line 40 — reporting green — for causes the skip message never mentions: a compiler that isn't on PATH, a CC that word-splits into nothing, or a permissions problem. $@ and $cc_stderr are both discarded, and the command is never printed.
This is worth fixing because it's the failure mode this PR just fixed: I confirmed that on main, CC="gcc -m32" made this file skip silently rather than fail. Since CI always uses gcc or clang, a skip here is by definition a bug, and nothing notices.
Suggest skipping only on the condition the message actually describes, and dying otherwise:
die "CC is set but contains no command\n" unless @cc;
die "could not run '@cc --version': $@" if !defined $cc_status;🤖 Comment by Claude (Claude Code) on behalf of Will.
| exclude: | ||
| - os: macos-latest | ||
| arch: -m32 | ||
| name: Autotools build on ${{matrix.os}} using ${{matrix.cc}} ${{matrix.posix}} ${{matrix.arch}} |
There was a problem hiding this comment.
Two small things from appending ${{matrix.arch}} here.
The rendered names get a double space when posix is empty and a trailing one when arch is empty — the current run shows Autotools build on ubuntu-latest using clang -m32. Cosmetic, but easy to tidy.
More worth checking: all 8 pre-existing job names changed as a result. If any are configured as required status checks under branch protection, they'll need updating or merges will block on names that no longer exist.
🤖 Comment by Claude (Claude Code) on behalf of Will.
| jobs: | ||
| # The -m32 variants build and test as 32-bit. ssize_t and long are 32 bits | ||
| # wide on i686, which exposes signedness and range problems that the 64-bit | ||
| # builds cannot see. The flag goes in CC rather than CFLAGS so that the |
There was a problem hiding this comment.
Minor, but the comment undersells the strongest argument for this choice. It frames CC as being about reaching libtap, while the next sentence says CFLAGS reaches libtap too (and that this is the problem). The more compelling reason is that -m32 has to apply to linking as well as compiling, and t/libtap/Makefile:13,22 build their link lines from $(CC) $(LDFLAGS) with no CFLAGS at all — so -m32 in CFLAGS alone would compile 32-bit objects and link them 64-bit.
Also worth citing the concrete motivation, which 4a8984e's message had and this comment dropped: the -Wsign-compare error in #486/#487 that only Fedora's i686 build caught. It turns a plausible-sounding claim into a checkable one.
🤖 Comment by Claude (Claude Code) on behalf of Will.
ssize_tandlongare 32 bits wide on i686. That exposes signedness and range problems that the 64-bit jobs cannot see, such as the-Wsign-compareerror fixed in #487, which only showed up in Fedora's i686 build (#486).The new job builds and tests with
-m32using gcc and clang onubuntu-latest. The flags are set for the whole job so that the libtap build, which has its own Makefile, and the test scripts that compile code themselves pick them up.The decoder limits test kept only
-fflags from the environment when it rebuilt the library, so it would have dropped-m32and checked 64-bit code. It now keeps-mflags as well.Verification
I emulated the job locally with the same environment as the workflow:
makefails with the original-Wsign-compareerrorWith the #487 fix reverted, the old decoder limits script also passed at 32-bit and the new one fails with the original error, so the perl change is needed for that test to give 32-bit coverage.
The existing jobs are not affected by the perl change. None of them have an
-mflag inCFLAGSorLDFLAGSwhilemake checkruns.🤖 Generated with Claude Code
Summary by CodeRabbit