Skip to content

ci: fix the Windows "Install GMT" step in tests.yml and docs.yml - #9134

Merged
Esteban82 merged 15 commits into
masterfrom
fix-windows-install-ci
Aug 25, 2026
Merged

ci: fix the Windows "Install GMT" step in tests.yml and docs.yml#9134
Esteban82 merged 15 commits into
masterfrom
fix-windows-install-ci

Conversation

@Esteban82

Copy link
Copy Markdown
Member

This PR fixes the Windows build problem in tests.yml and docs.yml.

It applies the same fix already merged for build.yml in #9097: the install moves into the cmd step that sources vcvars64.bat, and the Unix install step is guarded with if: runner.os != 'Windows'.

Assisted-by: Claude Opus 5 (extra effort)

PR #9097 fixed the Windows install step in build.yml but the same fix was
never applied to tests.yml and docs.yml, so the Windows jobs of both
workflows have been failing at "Install GMT" ever since.

The step ran `cmake --build . --target install` with the default
`bash -el {0}` shell and no OS guard, so on Windows it executed outside
the MSVC environment set up by vcvars64.bat. It also inherited INSTALLDIR
with backslashes, which CMake treats as escape sequences.

Consequences on Windows:

- tests.yml: "Run full tests" and "Run DOS batch examples" were reported
  as `skipped` in every recent run, i.e. the Windows test suite has not
  been executed at all while still reporting a red job.
- docs.yml: the Windows job failed before packaging.

Apply the same fix as build.yml: move the install into the existing `cmd`
step that already sets up vcvars64 (normalising INSTALLDIR and the vcpkg
root along the way), and guard the Unix install step with
`if: runner.os != 'Windows'`. The Windows step bodies are now identical
across build.yml, tests.yml and docs.yml.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Esteban82
Esteban82 requested review from joa-quim and seisman August 21, 2026 18:29
@Esteban82 Esteban82 added the AI-assisted All (or most) of the code was written by Artificial Intelligence. label Aug 21, 2026
@Esteban82 Esteban82 changed the title ci: fix the Windows "Install GMT" step in tests.yml and docs.yml WIP ci: fix the Windows "Install GMT" step in tests.yml and docs.yml Aug 21, 2026
Esteban82 and others added 4 commits August 21, 2026 22:55
With the Windows install step fixed, the Windows job now reaches
"Check a few simple commands" for the first time and crashes immediately on
the bare `gmt` call:

    + gmt
    *** stack smashing detected ***: terminated

The cause is the shell, not GMT. The workflow default is `bash -el {0}`, and
the login shell sources Git Bash's /etc/profile, which puts /usr/bin and
/mingw64/bin ahead of the vcpkg directories in PATH. gmt.exe then resolves its
netCDF/GDAL/curl DLLs to the MSYS2/MinGW copies instead of the vcpkg ones it
was linked against, and the ABI mismatch corrupts the stack.

build.yml runs the same script against the same binary with
`shell: bash` (i.e. `--noprofile --norc`) and passes, printing the splash
screen normally.

Split the step so Windows uses the same non-login shell as build.yml while
Linux and macOS keep the login shell they need for conda. Doing this per-OS
rather than globally is deliberate: macOS currently passes this step in
tests.yml, and build.yml's macOS job -- which does use `shell: bash` for it --
fails later in `gmt end` with `gmtinit_process_figures returned error 79`, so
that shell is not safe to adopt for every platform here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit's theory was wrong: giving Windows the same non-login
`shell: bash` and `export PATH` that build.yml uses did not help, and gmt.exe
still dies on the bare `gmt` call:

    + gmt
    *** stack smashing detected ***: terminated

So the crash is not caused by the login shell. What does work, and is proven
green in build.yml, is running the checks from cmd via
ci/simple-gmt-tests.bat: the same binary prints its splash screen normally
there.

Restrict the bash check to Linux/macOS and let Windows use the cmd script
alone. This also removes a genuine duplication: until now Windows ran the
simple checks twice, once through simple-gmt-tests.sh and once through
simple-gmt-tests.bat.

`where gmt` is added to the cmd step so the log records which gmt.exe is
picked up, which is the first thing needed if this ever regresses.

If the cmd script also crashes, the problem is in the binary itself rather
than in the shell environment -- the Windows test build differs from the
build.yml one only by SUPPORT_EXEC_IN_BINARY_DIR and the DO_* test flags --
and it will need a fix in the C code rather than in CI.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Esteban82 Esteban82 added the maintenance Boring but important stuff for the core devs label Aug 23, 2026
Esteban82 and others added 4 commits August 23, 2026 23:23
The cmd script crashes exactly like the bash one, so the claim that the binary
"runs fine from cmd" was wrong. Every gmt.exe invocation dies with
STATUS_STACK_BUFFER_OVERRUN (exit code 0xC0000409) in the Windows test build,
regardless of the shell used to launch it.

Keep the split, since running the same checks twice on Windows served no
purpose, but drop the incorrect explanation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Do not merge. To be reverted once the crashing function is identified.

Every gmt.exe invocation in the Windows test build dies with
STATUS_STACK_BUFFER_OVERRUN (exit code 0xC0000409), from cmd just as much as
from Git Bash, so the fault is in the binary rather than in the CI
environment. Reading the code has not been enough to locate it, so get a
backtrace from the runner instead.

Two changes, both Windows-only:

- Add -g to the cmake invocation. This only adds debug symbols; the Release
  optimisation level is untouched, so the crash should still reproduce while
  the backtrace gains function names.
- Add a step that runs gmt under gdb and dumps `bt full`, the backtrace of
  every thread (OpenMP is enabled, so the fault may not be on the main one)
  and the list of loaded DLLs. It is continue-on-error so the existing
  checks still report as before.

If the crash does *not* reproduce with -g, that is informative in itself and
points at optimisation-dependent undefined behaviour.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes the crash that made every gmt.exe call in the Windows test build abort
with STATUS_STACK_BUFFER_OVERRUN (0xC0000409) before printing anything.

A gdb backtrace from the runner shows the fault is not in GMT at all:

    #0  __stack_chk_fail ()    from C:\Miniconda\Library\bin\libgomp-1.dll
    #1  init ()               from C:\Miniconda\Library\bin\libgomp-1.dll
    #2  __main ()             from C:\Miniconda\Library\bin\libgomp-1.dll
    #3  __DllMainCRTStartup () from C:\Miniconda\Library\bin\libgomp-1.dll
    #4  ntdll!LdrInitializeThunk ()

It is the OpenMP runtime blowing up in its own DLL initialisation, before
main() ever runs, and it is loaded from conda rather than from MinGW.

The chain: with RUN_TESTS=true this script adds dvc to the conda packages, and
dvc pulls in conda-forge's libgomp-16.1.0 plus _openmp_mutex, which land in
C:\Miniconda\Library\bin. GMT is compiled by the MinGW GCC 15.2.0 in
C:\mingw64 with -fopenmp, so the libgomp built for GCC 16 is not the one it
was linked against, and loading it corrupts the stack.

This also explains why build.yml is unaffected: it runs with RUN_TESTS=false,
never installs dvc, and so never gets a libgomp under Miniconda.

Put C:\mingw64\bin on PATH ahead of the conda directories, mirroring what the
script already does for vcpkg on the line below.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reverts a0eb072. The backtrace it produced did its job: it showed the crash
was in conda's libgomp during DLL initialisation, not in GMT, which led to the
PATH fix in 8e751b7. With that in place the Windows job now gets through the
simple-command check and runs the full test suite, so the gdb step and the
extra -g have nothing left to report.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Esteban82 Esteban82 changed the title WIP ci: fix the Windows "Install GMT" step in tests.yml and docs.yml ci: fix the Windows "Install GMT" step in tests.yml and docs.yml Aug 24, 2026
@Esteban82

Copy link
Copy Markdown
Member Author

I think this can be reviewed now. The fail is during the "Run full tests" step. The "Compile and Install GMT (Windows)" works fine.

Comment thread .github/workflows/tests.yml Outdated
Comment on lines 163 to 167
# On Windows this check is done by the cmd script below instead, so that the same
# checks are not run twice on that platform.
- name: Check a few simple commands
if: runner.os != 'Windows'
run: bash ci/simple-gmt-tests.sh

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should disable this one. There are many users who run GMT with bash on Windows.

Comment thread ci/install-dependencies-windows.sh Outdated
Comment on lines +55 to +64
# GMT is compiled with the MinGW GCC in C:\mingw64, so its runtime libraries must win
# over the ones conda drops into Library\bin. When RUN_TESTS is true we install dvc,
# which pulls in conda-forge's libgomp built for a newer GCC; loading that instead of
# the MinGW one makes every gmt.exe call die during DLL init with
# STATUS_STACK_BUFFER_OVERRUN (0xC0000409). Added before the vcpkg line below so that
# it ends up after vcpkg but ahead of conda in PATH.
if [ -d "/c/mingw64/bin" ]; then
echo 'C:\mingw64\bin' >> $GITHUB_PATH
fi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one also makes no sense, because on Windows, we expect to use Visual Studio, not MinGW GCC.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel changes in this file can be reverted.

Comment thread .github/workflows/tests.yml Outdated
Comment thread ci/install-dependencies-windows.sh Outdated
Comment on lines +55 to +64
# GMT is compiled with the MinGW GCC in C:\mingw64, so its runtime libraries must win
# over the ones conda drops into Library\bin. When RUN_TESTS is true we install dvc,
# which pulls in conda-forge's libgomp built for a newer GCC; loading that instead of
# the MinGW one makes every gmt.exe call die during DLL init with
# STATUS_STACK_BUFFER_OVERRUN (0xC0000409). Added before the vcpkg line below so that
# it ends up after vcpkg but ahead of conda in PATH.
if [ -d "/c/mingw64/bin" ]; then
echo 'C:\mingw64\bin' >> $GITHUB_PATH
fi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel changes in this file can be reverted.

Esteban82 and others added 6 commits August 25, 2026 12:54
Resolves the conflict with #9144, which replaced the hardcoded Visual Studio
path with the setup-msvc-dev action on Windows, in the same steps this branch
rewrites to fix the install.

Take master's approach in both tests.yml and docs.yml: adopt the new
"Setup MSVC (Windows)" step and drop the `call ...\vcvars64.bat` line it makes
redundant, while keeping this branch's install work (the merged
compile-and-install step, the INSTALLDIR/vcpkg path normalisation and the
`if: runner.os != 'Windows'` guard on the Unix install step).

The result is byte-identical to the Windows steps master already carries in
build.yml, which is the point of this branch: propagate to tests.yml and
docs.yml what build.yml has.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
- tests.yml: stop skipping "Check a few simple commands" on Windows. Many
  users run GMT under bash on Windows, so this check should keep exercising
  that path there instead of being routed through the cmd script alone.

- install-dependencies-windows.sh: revert the C:\mingw64\bin PATH change.
  GMT on Windows is meant to be built with Visual Studio, not MinGW GCC, so
  working around a MinGW-vs-conda runtime clash by prioritising MinGW is not
  the right fix.

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-windows-install-ci

The remote's merge of master (32ee394, made via GitHub's web UI) combined
the two conflicting resolutions of the "Compile GMT (Windows)" step
incorrectly: it kept the step named without "Install", dropped the
`setlocal enabledelayedexpansion` and the INSTALLDIR/vcpkg-root
normalisation, but still kept `cmake --build . --target install` and the
`!WININSTALLDIR!`/`!VAR!` delayed-expansion lines with no `setlocal` in
scope to enable them and an unmatched `endlocal`. That combination is not
merely stylistically wrong, it silently reintroduces the install bug this
branch exists to fix: without the INSTALLDIR normalisation CMake sees
backslashes as escapes again, and without delayed expansion enabled
`!WININSTALLDIR!` is never substituted, so `%GITHUB_PATH%` gets a literal
garbage line instead of the install's bin directory.

Restore this branch's version of the step in both tests.yml and docs.yml:
`setlocal enabledelayedexpansion` right after `cd build`, the INSTALLDIR and
vcpkg-root normalisation, and the step named "Compile and Install GMT
(Windows)" to match what it now does.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Esteban82

Copy link
Copy Markdown
Member Author

The Windows install now works — Compile and Install GMT (Windows) and both Check a few simple commands steps pass.

Run full tests has 6 failing tests, but that looks unrelated to this PR and worth tracking in a separate issue.

@Esteban82
Esteban82 merged commit 719b06b into master Aug 25, 2026
2 of 4 checks passed
@Esteban82
Esteban82 deleted the fix-windows-install-ci branch August 25, 2026 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-assisted All (or most) of the code was written by Artificial Intelligence. maintenance Boring but important stuff for the core devs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants