diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 97e375dee..9e7c15673 100644 --- a/.github/workflows/desktop-build.yml +++ b/.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// 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. @@ -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//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 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}")