From c4ba4047e185e307a98d64dc6118dc72e86b7a72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Mon, 19 Oct 2026 09:19:46 +0200 Subject: [PATCH 1/4] [CrashReporting] Drop the invalid --release from the symbol upload `debug-files upload` never accepted `--release` (checked against the pinned sentry-cli 3.8.0: the subcommand declares --org/--project and a set of type/id/scan options, and execute() reads no release; the only global flags are --url/--header/--auth-token/--log-level/--quiet/--allow-failure). Clap rejects the unknown flag before any upload happens, so every tag build has been shipping without symbols, with continue-on-error keeping the release pipeline green. Release association is also the wrong mechanism: debug information files are matched to events by debug ID via the event's debug_meta. The Sentry release of a native event comes from the event itself (COCKATRICE_SENTRY_RELEASE), not from the DIF upload. Remove the flag and the now-unused SENTRY_RELEASE env from both invocations (Linux, and Windows/macOS). The symbol upload stays continue-on-error (a Sentry outage must not block the release assets); revisit once it has proven itself on a tag. --- .github/workflows/desktop-build.yml | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 97e375dee..8ef1af578 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -289,10 +289,6 @@ jobs: shell: bash env: SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} - # Same release string the client reports its events under (COCKATRICE_SENTRY_RELEASE - # = PROJECT_VERSION, see cmake/CrashReporting.cmake), so stack traces from this build - # de-symbolicate against these symbols. - SENTRY_RELEASE: ${{ needs.configure.outputs.sentry_release }} run: | paths=() for dir in build/cockatrice build/oracle build/servatrice; do @@ -303,7 +299,11 @@ jobs: exit 0 fi # Project slug on Sentry; must match the project the baked DSN points at. - sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop --release "$SENTRY_RELEASE" "${paths[@]}" + # No --release here: `debug-files upload` has no such option (sentry-cli errors on + # the unknown flag and uploads nothing), and debug information files are matched to + # events by debug ID via the event's debug_meta, not by release. The release grouping + # comes from the client event itself (COCKATRICE_SENTRY_RELEASE). + sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop "${paths[@]}" # Delete used cache to emulate a ccache update. See https://github.com/actions/cache/issues/342 - name: "Delete remote compiler cache (ccache)" @@ -643,10 +643,6 @@ jobs: shell: bash env: SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} - # Same release string the client reports its events under (COCKATRICE_SENTRY_RELEASE - # = PROJECT_VERSION, see cmake/CrashReporting.cmake), so stack traces from this build - # de-symbolicate against these symbols. - SENTRY_RELEASE: ${{ needs.configure.outputs.sentry_release }} run: | paths=() if [[ '${{ matrix.os }}' == 'Windows' ]]; then @@ -675,7 +671,9 @@ jobs: exit 0 fi # Project slug on Sentry; must match the project the baked DSN points at. - sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop --release "$SENTRY_RELEASE" "${paths[@]}" + # No --release: see the Linux leg above — the flag does not exist on + # `debug-files upload` and DIFs are matched by debug ID, not release. + sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop "${paths[@]}" # CPack reports a failed makensis run as "Problem running NSIS command" # plus a path to NSISOutput.log, and that log lives only on the runner, so From ee404ada3b3fece90b79baa71bd8d8eed3d608c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Mon, 19 Oct 2026 09:29:13 +0200 Subject: [PATCH 2/4] [CrashReporting] Split debug information out of packaged release binaries -g is required so the CI symbol upload has DWARF to send, but it was left in the shipped binaries too: the Linux Release client is 563 MiB, 552 MiB of it debug info, and every distribution channel pays for that. Keep the build tree unchanged (CI uploads symbols from build/) and strip only the packaged copy. A CPack pre-build script extracts each ELF's debug info to COCKATRICE_DEBUG_SYMBOL_DIR and then runs `strip --strip-debug --strip-unneeded`. .eh_frame is allocatable and survives, so unwinding is unaffected, and the build ID is identical in the stripped binary and the sidecar. Measured on a local Release build (GCC -O2 -g, WITH_CRASH_REPORTING=ON), bytes: binary build tree packaged before packaged after .debug sidecar cockatrice 590,722,560 590,722,560 28,086,480 579,623,576 oracle 53,432,608 53,432,608 1,649,888 52,075,344 tar.gz of both trees: 289,964,521 -> 19,018,058 (~15x smaller). Only ELF (Linux/BSD) is handled: Windows already links /DEBUG with separate PDBs, and macOS extracts a dSYM from the bundle. Opt-out via COCKATRICE_SPLIT_DEBUG_SYMBOLS; skipped with a warning when objcopy/strip are missing. CPACK_PRE_BUILD_SCRIPTS (not CPACK_INSTALL_SCRIPTS, which runs before staging) invokes it, and the script refuses to run unless CPACK_TEMPORARY_DIRECTORY is a real CPack staging tree so a missing variable can never turn its glob into a filesystem-wide walk. --- cmake/CrashReporting.cmake | 36 ++++++++++++++ cmake/SplitDebugSymbols.cmake.in | 80 ++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 cmake/SplitDebugSymbols.cmake.in diff --git a/cmake/CrashReporting.cmake b/cmake/CrashReporting.cmake index 2c8888800..db016269f 100644 --- a/cmake/CrashReporting.cmake +++ b/cmake/CrashReporting.cmake @@ -143,3 +143,39 @@ else() "(WITH_CRASH_REPORTING=${WITH_CRASH_REPORTING}, WITH_CLIENT=${WITH_CLIENT})" ) endif() + +# Packaged release binaries must not ship the DWARF that the symbol upload needs: it is +# 80-90% of an unstripped binary and nobody downloads a 500 MB installer to get it. The +# build tree keeps -g (CI uploads symbols from there); the *packaged copy* is split at +# install time by a CPack install script, which extracts the debug info next to the staging +# dir and strips the binary. Only ELF (Linux/BSD) needs this: Windows links /DEBUG with +# separate PDBs, and macOS keeps its dSYM extracted from the bundle. +option(COCKATRICE_SPLIT_DEBUG_SYMBOLS "Strip DWARF out of packaged release binaries" ON) +set(COCKATRICE_DEBUG_SYMBOL_DIR + "${CMAKE_BINARY_DIR}/debug-symbols" + CACHE PATH "Where the packaged binaries' split debug information is written (kept out of the package)" +) +if(WITH_CRASH_REPORTING + AND WITH_CLIENT + AND COCKATRICE_SPLIT_DEBUG_SYMBOLS + AND CMAKE_BUILD_TYPE STREQUAL "Release" + AND UNIX + AND NOT APPLE +) + find_program(COCKATRICE_OBJCOPY NAMES objcopy) + find_program(COCKATRICE_STRIP_TOOL NAMES strip) + if(COCKATRICE_OBJCOPY AND COCKATRICE_STRIP_TOOL) + configure_file( + "${CMAKE_CURRENT_LIST_DIR}/SplitDebugSymbols.cmake.in" "${CMAKE_BINARY_DIR}/cmake/SplitDebugSymbols.cmake" @ONLY + ) + # PRE_BUILD runs after CPack stages the install tree and before the package is produced. + # CPACK_INSTALL_SCRIPTS would run too early (staging is still empty); the script walks + # CPACK_TEMPORARY_DIRECTORY, never CMAKE_INSTALL_PREFIX, and refuses to run otherwise. + list(APPEND CPACK_PRE_BUILD_SCRIPTS "${CMAKE_BINARY_DIR}/cmake/SplitDebugSymbols.cmake") + message(STATUS "Crash reporting: packaged binaries stripped, debug info split to ${COCKATRICE_DEBUG_SYMBOL_DIR}") + else() + message(WARNING "COCKATRICE_SPLIT_DEBUG_SYMBOLS is ON but objcopy/strip were not found; " + "packaged release binaries will keep their debug information" + ) + endif() +endif() diff --git a/cmake/SplitDebugSymbols.cmake.in b/cmake/SplitDebugSymbols.cmake.in new file mode 100644 index 000000000..bfe954712 --- /dev/null +++ b/cmake/SplitDebugSymbols.cmake.in @@ -0,0 +1,80 @@ +# Splits DWARF out of the packaged ELF binaries, then strips them. +# +# This is a CPack pre-build script (CPACK_PRE_BUILD_SCRIPTS, wired in CrashReporting.cmake): +# CPack runs it after staging the install tree and before producing the package, so it only +# ever touches the copy that becomes the release artifact. The build tree is left alone: -g +# stays on, and CI uploads symbols to Sentry straight from build/. +# +# For every ELF file under CPACK_TEMPORARY_DIRECTORY the script extracts the debug information +# to COCKATRICE_DEBUG_SYMBOL_DIR and removes the debug sections from the packaged binary. +# COCKATRICE_DEBUG_SYMBOL_DIR is deliberately outside the staging tree, so sidecars never end +# up in the package; CI can archive/upload them if wanted (it normally uploads straight from +# the build tree). The only thing that must survive stripping is .eh_frame (allocatable, used +# for unwinding), which neither --only-keep-debug nor --strip-debug removes. +# +# Tool paths, the symbol directory and the tree to walk are baked in / provided at configure / +# run time; the guards below refuse to run on anything that is not a CPack staging directory, +# so a missing variable can never turn the glob into a filesystem-wide walk. + +set(_objcopy "@COCKATRICE_OBJCOPY@") +set(_strip "@COCKATRICE_STRIP_TOOL@") +set(_symbolDir "@COCKATRICE_DEBUG_SYMBOL_DIR@") + +if(_objcopy STREQUAL "" OR _strip STREQUAL "") + message(WARNING "split debug symbols: objcopy/strip not found at configure time, packaging unstripped binaries") + return() +endif() +if(_symbolDir STREQUAL "") + message(FATAL_ERROR "split debug symbols: COCKATRICE_DEBUG_SYMBOL_DIR is empty") +endif() + +# CPack sets CPACK_TEMPORARY_DIRECTORY to the staging tree; CMAKE_INSTALL_PREFIX is still the +# configured /usr prefix here, so globbing it would walk system directories. Refuse to run +# unless the path exists and really is a CPack staging tree. +if(NOT CPACK_TEMPORARY_DIRECTORY OR NOT IS_DIRECTORY "${CPACK_TEMPORARY_DIRECTORY}") + message(STATUS "split debug symbols: no CPack staging directory, skipping") + return() +endif() +if(NOT CPACK_TEMPORARY_DIRECTORY MATCHES "_CPack_Packages") + message(WARNING "split debug symbols: ${CPACK_TEMPORARY_DIRECTORY} is not a CPack staging tree, skipping") + return() +endif() + +file(MAKE_DIRECTORY "${_symbolDir}") + +file( + GLOB_RECURSE _entries + LIST_DIRECTORIES true + "${CPACK_TEMPORARY_DIRECTORY}/*" +) +set(_processed 0) +foreach(_file IN LISTS _entries) + if(IS_DIRECTORY "${_file}" OR IS_SYMLINK "${_file}") + continue() + endif() + # Only ELF: objcopy would fail on the icons, translations and Qt resources next to it. + file( + READ "${_file}" _magic + OFFSET 0 + LIMIT 4 + HEX + ) + if(NOT _magic STREQUAL "7f454c46") + continue() + endif() + get_filename_component(_name "${_file}" NAME) + set(_debug "${_symbolDir}/${_name}.debug") + execute_process(COMMAND "${_objcopy}" --only-keep-debug "${_file}" "${_debug}" RESULT_VARIABLE _rc) + if(NOT _rc EQUAL 0) + message(WARNING "split debug symbols: objcopy --only-keep-debug failed for ${_file}") + continue() + endif() + execute_process(COMMAND "${_strip}" --strip-debug --strip-unneeded "${_file}" RESULT_VARIABLE _rc) + if(NOT _rc EQUAL 0) + message(WARNING "split debug symbols: strip failed for ${_file}") + continue() + endif() + math(EXPR _processed "${_processed} + 1") +endforeach() + +message(STATUS "Split debug symbols for ${_processed} packaged ELF file(s); sidecars in ${_symbolDir}") From d7bc35d9d8443110493365f66713b0f2fd5b74f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Mon, 19 Oct 2026 10:54:07 +0200 Subject: [PATCH 3/4] [CrashReporting] Upload Windows PDBs from where Ninja actually puts them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows branch of the symbol upload looked in build//Release, which is the Visual Studio generator's layout. The Windows leg uses Ninja, which is single-config: linker PDBs land in build//, next to the binaries — the same paths the "[Windows] Upload PDBs" artifact step below globs with if-no-files-found: error. So paths was always empty on Windows, the step printed "No symbol paths found, skipping symbol upload" and exited 0: a silent no-op behind continue-on-error. Every Windows build would then report "A required debug information file was missing" for cockatrice.exe with a PDB-style debug id (GUID-age), exactly as if no upload step existed at all. Use the artifact step's globs. Also turn the empty-path case into a failure in both symbol steps: a silent skip is indistinguishable from a successful upload (which is how the wrong path went unnoticed), and continue-on-error already keeps a broken upload from failing the release. Verified by extracting both steps' run blocks and executing the Windows branch against a Ninja-style tree: it now passes the three linker PDBs to sentry-cli (and not the CMakeFiles/*.pdb intermediates), and exits non-zero with an ::error:: annotation when there is nothing to upload. --- .github/workflows/desktop-build.yml | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 8ef1af578..b1ea70070 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -295,8 +295,11 @@ jobs: [[ -d $dir ]] && paths+=("$dir") done if [[ ${#paths[@]} == 0 ]]; then - echo "No build output dirs found, skipping symbol upload" - exit 0 + # Fail loudly instead of skipping: see the Windows/macOS leg below — this step + # is the only way symbols reach Sentry and `continue-on-error` keeps the job + # green either way, so a silent skip looks exactly like a successful upload. + echo "::error::No debug symbol files found; nothing would be uploaded" + exit 1 fi # Project slug on Sentry; must match the project the baked DSN points at. # No --release here: `debug-files upload` has no such option (sentry-cli errors on @@ -646,11 +649,13 @@ jobs: run: | paths=() if [[ '${{ matrix.os }}' == 'Windows' ]]; then - # Top-level CMakeLists.txt links /DEBUG even in release, so PDBs land - # next to the binaries (same dirs as the PDB artifact step below). - for dir in build/cockatrice/Release build/oracle/Release build/servatrice/Release; do - [[ -d $dir ]] && paths+=("$dir") - done + # Top-level CMakeLists.txt links /DEBUG even in release, so the linker PDBs + # land next to the binaries. Ninja is single-config, so that is + # build// and NOT build//Release/ — the same globs the PDB + # artifact step below uses. + shopt -s nullglob + paths+=(build/cockatrice/*.pdb build/oracle/*.pdb build/servatrice/*.pdb) + shopt -u nullglob else # Release builds carry DWARF (-g, kept by CrashReporting.cmake); macOS keeps its # debug info in dSYM bundles, so materialize them before uploading. @@ -667,8 +672,12 @@ jobs: done fi if [[ ${#paths[@]} == 0 ]]; then - echo "No symbol paths found, skipping symbol upload" - exit 0 + # Fail loudly instead of skipping: this step is the only way symbols reach + # Sentry, and `continue-on-error` keeps the job green either way, so a + # silent skip is indistinguishable from a successful upload. It also hid a + # wrong Windows path (build//Release, which Ninja never creates). + echo "::error::No debug symbol files found; nothing would be uploaded" + exit 1 fi # Project slug on Sentry; must match the project the baked DSN points at. # No --release: see the Linux leg above — the flag does not exist on From fef0edbd0b869795642053e0a7bb31292232ba3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Mon, 19 Oct 2026 11:04:16 +0200 Subject: [PATCH 4/4] [CrashReporting] Archive PDBs on tag builds so a failed upload is recoverable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "[Windows] Upload PDBs" artifact step skipped tag builds (github.ref_type != 'tag'), so for a release the symbols existed only inside the Sentry upload — which is continue-on-error. A failed upload therefore lost the ability to symbolicate that release permanently: no rebuild produces the same PDB, since the debug id is a per-build GUID+age. That is how every Windows crash reported so far became unreadable. Archive the PDBs for tag builds as well, and order the step after "Upload to release" so a missing PDB can never block a release from being published. Retention stays at the repository default; nothing consumes the artifact automatically, it is the manual recovery path (gh run download -n -PDBs, then sentry-cli debug-files upload). Verified by re-parsing the workflow and asserting the step order (release -> PDBs -> attest) and the broadened condition. --- .github/workflows/desktop-build.yml | 32 +++++++++++++++++------------ 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index b1ea70070..9e7c15673 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -827,19 +827,6 @@ jobs: if-no-files-found: error path: ${{ steps.build.outputs.path }} - # Ninja is single-config: binaries and their PDBs land directly in the - # target directories instead of a Release subdirectory. - - name: "[Windows] Upload PDBs (Program Databases)" - if: matrix.os == 'Windows' && github.ref_type != 'tag' - uses: actions/upload-artifact@v7 - with: - if-no-files-found: error - name: ${{ steps.build.outputs.name }}-PDBs - path: | - build/cockatrice/*.pdb - build/oracle/*.pdb - build/servatrice/*.pdb - - name: "Upload to release" if: needs.configure.outputs.tag != null && matrix.make_package == '1' id: upload_release @@ -851,6 +838,25 @@ jobs: tag_name: ${{ needs.configure.outputs.tag }} run: gh release upload "$tag_name" "$asset_path#$asset_name" + # Ninja is single-config: binaries and their PDBs land directly in the + # target directories instead of a Release subdirectory. + # + # Archived for tag builds as well, and ordered after the release upload so + # a missing PDB can never block a release from being published. On a tag + # build this artifact is the only durable copy of the symbols: the Sentry + # upload is continue-on-error, so without it a failed upload would lose + # the ability to symbolicate that release permanently. + - name: "[Windows] Upload PDBs (Program Databases)" + if: matrix.os == 'Windows' + uses: actions/upload-artifact@v7 + with: + if-no-files-found: error + name: ${{ steps.build.outputs.name }}-PDBs + path: | + build/cockatrice/*.pdb + build/oracle/*.pdb + build/servatrice/*.pdb + - name: "Attest binary provenance" if: steps.upload_release.outcome == 'success' id: attestation