Skip to content

Add a 32-bit autotools CI job #489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
oschwald merged 2 commits into from
Sep 14, 2026
+21 −5
Merged

Add a 32-bit autotools CI job #489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a8984e
Add a 32-bit autotools CI job
horgh Sep 13, 2026
665789d
Fold the 32-bit build into the autoconf matrix
horgh Sep 14, 2026
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
18 changes: 16 additions & 2 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,28 @@ on:
- cron: '3 20 * * SUN'
permissions: {}
jobs:
# The -m32 variants build and test as 32-bit. ssize_t and long are 32 bits

@horgh horgh Sep 14, 2026

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.

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.

# 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

@horgh horgh Sep 14, 2026

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.

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.

# libtap build, which has its own Makefile, and the test scripts that

@horgh horgh Sep 14, 2026

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.

"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.

# compile code themselves pick it up too. CFLAGS must stay out of the job
# environment: when it is set, even empty, make exports the configured
# CFLAGS to libtap, and libtap does not build on macOS with _POSIX_C_SOURCE.
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}}
arch: ['', -m32]
exclude:
- os: macos-latest
arch: -m32
name: Autotools build on ${{matrix.os}} using ${{matrix.cc}} ${{matrix.posix}} ${{matrix.arch}}

@horgh horgh Sep 14, 2026

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.

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.

runs-on: ${{ matrix.os }}
env:
CC: ${{ matrix.cc }}
CC: ${{ matrix.cc }} ${{ matrix.arch }}
CXXFLAGS: ${{ matrix.arch }}

@horgh horgh Sep 14, 2026

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.

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.

VERBOSE: 1
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
Expand All @@ -24,6 +36,8 @@ jobs:
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

@horgh horgh Sep 14, 2026

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.

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.

if: ${{ matrix.os == 'ubuntu-latest' && matrix.arch == '-m32' }}
- run: brew install autoconf automake libtool
if: ${{ matrix.os == 'macos-latest' }}
- run: ./bootstrap
Expand Down
8 changes: 5 additions & 3 deletions t/decoder_limits_t.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,16 @@
my $root = abs_path("$Bin/..");
my $include_dir = "$root/include";
my $src_dir = "$root/src";
my $cc = $ENV{CC} || 'cc';

# CC may carry flags, such as CC="gcc -m32".
my @cc = split ' ', $ENV{CC} || 'cc';

@horgh horgh Sep 14, 2026

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.

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 checks below rebuild the library with -Werror. Only gcc and clang are
# known to compile it cleanly with the flags used here, so skip elsewhere
# instead of failing on a missing compiler or an unrelated warning.
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 );

@horgh horgh Sep 14, 2026

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.

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.

$?;
};
$cc_version .= $cc_stderr;
Expand All @@ -44,7 +46,7 @@
map { split ' ' } grep { defined } @ENV{ 'CFLAGS', 'LDFLAGS' };

my @base = (
$cc,
@cc,
@instrumentation,
'-std=c99',
'-Wall',
Expand Down
Loading