[CrashReporting] Archive PDBs on tag builds so a failed upload is recoverable

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 <run> -n <name>-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

@@ -289,21 +289,24 @@ 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
             [[ -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.
-          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,18 +646,16 @@ 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
-            # 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/<target>/ and NOT build/<target>/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.
@@ -671,11 +672,17 @@ 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/<target>/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.
-          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
@@ -820,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
@@ -844,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

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()

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}")