Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Include dSYMs in release for macOS dependencies #270

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

jcm93
Copy link
Contributor

@jcm93 jcm93 commented Dec 15, 2024

Description

Adds a wildcard to the end of the filenames to be uploaded in the final release for macOS artifacts.

Motivation and Context

In releases, dSYMs for macOS dependencies are not being uploaded properly. Checksums are being generated, showing that the files are present. The Windows final deps upload paths include a wildcard at the end (to capture the -PDBs suffix), but this wildcard is not present for the macOS artifacts.

How Has This Been Tested?

Tested on local fork.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@jcm93 Did you run a test release on your fork to check if the glob works as intended?

@jcm93
Copy link
Contributor Author

jcm93 commented Jan 17, 2025

Yes; I linked it in the earlier conversation: https://github.com/jcm93/obs-deps/releases/tag/2024-12-23

@PatTheMav
Copy link
Member

Yes; I linked it in the earlier conversation: https://github.com/jcm93/obs-deps/releases/tag/2024-12-23

But there's only dSYMs for Qt6 it seems?

@jcm93
Copy link
Contributor Author

jcm93 commented Jan 17, 2025

It looks like the missing dSYMs (for the generic macos-deps-2024-12-23-arm64.tar.xz etc.) are not among the downloaded artifacts for the release flow in the first place, looking at the checksums there and in the latest official release. dSYMs are not present there nor are PDBs for the Windows counterparts (windows-deps-2024-12-23-x64.zip etc). I had actually assumed that that was expected and there was some other reason those debug symbols weren't available. If we want I can expand the scope of this to figure out why dSYMs and PDBs aren't included for the main prebuilt batch.

@RytoEX
Copy link
Member

RytoEX commented Jan 17, 2025

It looks like the missing dSYMs (for the generic macos-deps-2024-12-23-arm64.tar.xz etc.) are not among the downloaded artifacts for the release flow in the first place, looking at the checksums there and in the latest official release. dSYMs are not present there nor are PDBs for the Windows counterparts (windows-deps-2024-12-23-x64.zip etc). I had actually assumed that that was expected and there was some other reason those debug symbols weren't available. If we want I can expand the scope of this to figure out why dSYMs and PDBs aren't included for the main prebuilt batch.

We have historically never enabled PDBs for the non-Qt Windows dependencies. That is a separate item to address (which required no longer cross-compiling many of the deps) and need not be part of this PR.

@PatTheMav
Copy link
Member

IMO this PR is incomplete if only the Qt6 dSYMs are made available in a release (as all the other dSYMs are generated as artefacts). Because FFmpeg and main dependencies are smushed together into a new archive, you'd probably have to extend the workflow step to do the same for the dSYM archives as well.

@jcm93
Copy link
Contributor Author

jcm93 commented Jan 18, 2025

I'll take a look at getting dSYMs for everything else.

@RytoEX
Copy link
Member

RytoEX commented Jan 18, 2025

IMO this PR is incomplete if only the Qt6 dSYMs are made available in a release (as all the other dSYMs are generated as artefacts). Because FFmpeg and main dependencies are smushed together into a new archive, you'd probably have to extend the workflow step to do the same for the dSYM archives as well.

My main point about the Windows deps is that we do not even generate PDBs for the FFmpeg dependencies on Windows yet. Addressing that here would be out of scope. If PDBs for everything else are already generated, but not included, then yes, it is fine to address that here.

@PatTheMav
Copy link
Member

IMO this PR is incomplete if only the Qt6 dSYMs are made available in a release (as all the other dSYMs are generated as artefacts). Because FFmpeg and main dependencies are smushed together into a new archive, you'd probably have to extend the workflow step to do the same for the dSYM archives as well.

My main point about the Windows deps is that we do not even generate PDBs for the FFmpeg dependencies on Windows yet. Addressing that here would be out of scope. If PDBs for everything else are already generated, but not included, then yes, it is fine to address that here.

Yep the PDBs are a different matter entirely. For macOS all dependencies flavours generate dSYMs already though.

@jcm93
Copy link
Contributor Author

jcm93 commented Jan 20, 2025

Updated; added a new step that packages the debug symbols, since it uses a separate glob expression. Test release is here.

Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

There is no need for an additional step, this should do just fine:

for arch in arm64 x86_64 universal; do
  _temp=$(mktemp -d)
  pushd "${_temp}" > /dev/null
  for artifact in ${GITHUB_WORKSPACE}/**/macos-@(deps|ffmpeg)-!(qt6*)-${arch}?(-dSYMs).*; do
    case ${artifact} in
      *.zip) unzip -o ${artifact} > /dev/null ;;
      *.tar.xz) XZ_OPT=-T0 tar -xvJf ${artifact} ;;
      *.tar.gz) tar -xvzf ${artifact} ;;
    esac
  done
  XZ_OPT=-T0 tar -cvJf macos-deps-${{ steps.metadata.outputs.version }}-${arch}.tar.xz -- !(*.dSYM)
  XZ_OPT=-T0 tar -cvJf macos-deps-${{ steps.metadata.outputs.version }}-${arch}-dSYMs.tar.xz -- *.dSYM
  mv macos-deps-${{ steps.metadata.outputs.version }}-${arch}?(-dSYMs).tar.xz ${GITHUB_WORKSPACE}
  popd > /dev/null
done

@jcm93
Copy link
Contributor Author

jcm93 commented Jan 20, 2025

Alright yeah that seems to have worked. Pushed; test release here.

@RytoEX RytoEX requested a review from PatTheMav January 22, 2025 19:10
.github/workflows/main.yaml Show resolved Hide resolved
.github/workflows/main.yaml Outdated Show resolved Hide resolved
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

Looks good to me - obviously depends on the artefact contents staying as-is, but the entire CI infrastructure depends on every file fulfilling some expectation like that. 😅

@RytoEX RytoEX merged commit e659d61 into obsproject:master Jan 23, 2025
17 checks passed
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.

3 participants