Skip to content

Remove the unused GAP sync machinery from JuliaInterface - #1412

Merged
lgoettgens merged 1 commit into
masterfrom
mh/remove-dead-sync
Aug 10, 2026
Merged

Remove the unused GAP sync machinery from JuliaInterface#1412
lgoettgens merged 1 commit into
masterfrom
mh/remove-dead-sync

Conversation

@fingolfin

Copy link
Copy Markdown
Member

THREADSAFE_GAP_JL is never defined anywhere, so BEGIN_GAP_SYNC and END_GAP_SYNC always expanded to no-ops, and BeginGapSync and EndGapSync were compiled into the kernel extension without ever being declared or called. The only function that actually ran, InitGapSync, initialized a recursive mutex nobody locks and set a flag nobody reads. The file's own TODO: this is not actually fully implemented!! says as much.

Had the toggle ever been flipped on, the code would not have worked as intended anyway: in handle_jl_exception the END_GAP_SYNC() follows an ErrorMayQuit that never returns, and Func_JuliaGetGlobalVariableByModule can ErrorMayQuit in the middle of the guarded region, longjmp'ing past the unlock.

Removing it also gets rid of the local extern int jl_n_threads declaration, which conflicts with the _Atomic(int) declaration in julia.h since Julia 1.9. That went unnoticed as long as julia.h was not in scope in this file, but it breaks the build as soon as GAP's src/gasman.h includes julia.h, as proposed in gap-system/gap#6490 — that PR's CI with GAP.jl jobs currently fail with:

sync.c:24:16: error: conflicting type qualifiers for 'jl_n_threads'
julia.h:2172:34: note: previous declaration of 'jl_n_threads' with type 'int'

Verified locally that JuliaInterface builds cleanly both against a GAP with that PR applied and against GAP without it, so this remains compatible with released GAP versions.

AI disclosure: Claude Code (Opus 5) diagnosed the issue, drafted the change, and verified it locally.

`THREADSAFE_GAP_JL` is never defined, so `BEGIN_GAP_SYNC` and
`END_GAP_SYNC` always expanded to no-ops, and `BeginGapSync` and
`EndGapSync` were compiled into the kernel extension without ever being
declared or called. The one function that did run, `InitGapSync`, merely
initialized a mutex nobody locks and set a flag nobody reads.

It also declared `extern int jl_n_threads` itself, which since Julia 1.9
conflicts with the `_Atomic(int)` declaration in julia.h. That went
unnoticed as long as julia.h was not in scope in this file, but it breaks
the build once GAP's src/gasman.h includes julia.h, see
gap-system/gap#6490.

AI disclosure: Claude Code (Opus 5) diagnosed the issue, drafted the
change, and verified it locally.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.30%. Comparing base (89ae110) to head (43ad7af).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1412      +/-   ##
==========================================
+ Coverage   83.28%   83.30%   +0.01%     
==========================================
  Files          63       62       -1     
  Lines        5073     5049      -24     
  Branches       30       28       -2     
==========================================
- Hits         4225     4206      -19     
+ Misses        848      843       -5     
Files with missing lines Coverage Δ
pkg/JuliaInterface/src/JuliaInterface.c 95.00% <ø> (-0.24%) ⬇️
pkg/JuliaInterface/src/calls.c 83.72% <ø> (-0.49%) ⬇️
pkg/JuliaInterface/src/convert.c 92.59% <ø> (ø)

... and 1 file with indirect coverage changes

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

@lgoettgens
lgoettgens merged commit d0dd5e6 into master Aug 10, 2026
181 of 185 checks passed
@lgoettgens
lgoettgens deleted the mh/remove-dead-sync branch August 10, 2026 10:38
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.

2 participants