[No QA] Consume react-native dSYMS in CI workflows - #98853
Conversation
mateuuszzzzz
changed the title
deploy.yml workflowdeploy.yml workflow
mateuuszzzzz
changed the title
deploy.yml workflow
| path: ~/Library/Caches/Expensify/react-native-artifacts | ||
| key: ${{ runner.os }}-rn-artifacts-react-hybrid-${{ steps.resolve-rn-artifacts.outputs.VERSION }} | ||
|
|
||
| - name: Cache Pod dependencies |
There was a problem hiding this comment.
It seems that the Pods cache key doesn't account for download-dsyms but the dSYM state lives inside the cached path: process_dsyms merges the symbols into the framework tarball under Pods/ReactNativeCore-artifacts (artifacts_dir in rncore.rb).
verifyHybridApp.yml calls this workflow with the download-dsyms: 'false' and runs on pull_request_target, so its caches are main-scoped and restorable by staging/production deploy runs. This creates a situation where a deploy that restores a Pods cache and then skips the CocoaPods installation will archive without the react-native dSYMs. As a result, Sentry silently loses the symbols, depending on who saved the cache last.
We could append -${{ inputs.download-dsyms }} to the cache key to prevent this from happening
There was a problem hiding this comment.
Regarding pods installation step: This step is a bit outdated at the moment. Currently it does not have Github token in context so it cannot even install pods properly. Pods are also installed by Rock (so we have duplication) and this step does it correctly, but at the same time it does not have advanced caching logic. This is something I'd like to address in separate PR that will improve current pods installation and disable this step in rock.
There was a problem hiding this comment.
|
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6033111017
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| runs-on: blacksmith-12vcpu-macos-latest | ||
| env: | ||
| PULL_REQUEST_NUMBER: ${{ inputs.pull-request-number }} | ||
| RCT_SYMBOLICATE_PREBUILT_FRAMEWORKS: ${{ inputs.download-dsyms && '1' || '0' }} |
There was a problem hiding this comment.
When the same commit is first built by verifyHybridApp.yml, this input defaults to false, while deployment and AdHoc builds later set it to true. Rock downloads an existing native build when its fingerprint matches (contributingGuides/SETUP_IOS.md:5-9), but rock.config.mjs:24-34 does not include RCT_SYMBOLICATE_PREBUILT_FRAMEWORKS in the fingerprint environment. Consequently, the dSYM-enabled run can reuse the archive produced without dSYMs and skip the pod install/archive work that consumes them, leaving React Native frames unsymbolicated; add this variable to fingerprint.env or otherwise force distinct native fingerprints for the two modes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This comment can be a bit misleading, but the point is:
The output of the build is an xcarchive. This archive may or may not include dSYMs. I don't think the lack of this distinction breaks the build (in fact, it shouldn't, since debug symbols don't affect the binary layout), but it does clearly draw a line between archives with and without dSYMs.
I.e., our final output depends on the dSYMs flag, so it makes sense to include it in the rock fingerprint.
|
@MelvinBot could you create a follow-up issue for this one: #98853 (comment) |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
| env: | ||
| GITHUB_TOKEN: ${{ github.token }} | ||
| run: | | ||
| VERSION=$(bun scripts/artifacts-utils/resolve-artifacts.ts --platform=ios --hybrid=true --new-dot-root="$GITHUB_WORKSPACE" | jq -r '.version // "source"') |
There was a problem hiding this comment.
| VERSION=$(bun scripts/artifacts-utils/resolve-artifacts.ts --platform=ios --hybrid=true --new-dot-root="$GITHUB_WORKSPACE" | jq -r '.version // "source"') | |
| VERSION=$(./node_modules/.bin/bun scripts/artifacts-utils/resolve-artifacts.ts --platform=ios --hybrid=true --new-dot-root="$GITHUB_WORKSPACE" | jq -r '.version // "source"') |
There was a problem hiding this comment.
or alternatively: update setupNode composite action to add that to the path so plain bun works. npx bun would also work, but it spins up a node runtime just to resolve a known binary path for the bun binary, which then must start up. So it adds ~60ms vs executing bun directly. nbd in this context
|
@roryabraham Does this need review? |
|
no thanks @mananjadhav |
5 participants
Explanation of Change
PR enables dSYMs consumption for staging, production and adhoc builds. During creating Xcode archive those dSYMs are consumed by Sentry build phase so later we can read
react-nativeframes in crash reports.Additionally, this PR removed redundancy from artifacts utility script and now package name is picked based on platform and hybrid flags.
dSYMs are large files so cache step was added for them so we could retrieve them quickly during workflow run.
For clarification:
xcframework. Finally, Xcode archive extracts those dSYMs into a directory that Sentry build phase reads.Fixed Issues
$ #99186
PROPOSAL:
Tests
Trigger an AdHoc build with workflow changes from this PR
buildIOSjob, verify theResolve react-native artifact versionstep outputs a real version (notsource)Cache react-native artifactsstep uses key…-rn-artifacts-react-hybrid-<version>Cache miss: downloading reactnative-core-dSYM-debug.tar.gz …andAdding symbols … to framework tarball …Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari