-
Notifications
You must be signed in to change notification settings - Fork 4k
[No QA] Consume react-native dSYMS in CI workflows #98853
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
base: main
Are you sure you want to change the base?
Changes from 10 commits
d17f548
ac0c974
e26e82f
a3c240a
cbfc5cb
70efbf1
3e2bc4b
692cb46
665caa2
6033111
9a6ceb3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -27,6 +27,10 @@ on: | |||||
| description: Force a full native build, bypassing Rock remote cache | ||||||
| type: string | ||||||
| default: 'false' | ||||||
| download-dsyms: | ||||||
| description: Download the prebuilt react-native dSYMs so Sentry can symbolicate its native frames. Off by default - it adds two large downloads per build. | ||||||
| type: boolean | ||||||
| default: false | ||||||
|
|
||||||
| outputs: | ||||||
| IOS_VERSION: | ||||||
|
|
@@ -51,6 +55,7 @@ jobs: | |||||
| runs-on: blacksmith-12vcpu-macos-latest | ||||||
| env: | ||||||
| PULL_REQUEST_NUMBER: ${{ inputs.pull-request-number }} | ||||||
| RCT_SYMBOLICATE_PREBUILT_FRAMEWORKS: ${{ inputs.download-dsyms && '1' || '0' }} | ||||||
| outputs: | ||||||
| IOS_VERSION: ${{ steps.getIOSVersion.outputs.IOS_VERSION }} | ||||||
| ROCK_ARTIFACT_URL: ${{ steps.set-artifact-url.outputs.ARTIFACT_URL }} | ||||||
|
|
@@ -112,6 +117,21 @@ jobs: | |||||
| - name: Install New Expensify Gems | ||||||
| run: bundle install | ||||||
|
|
||||||
| - name: Resolve react-native artifact version | ||||||
|
mateuuszzzzz marked this conversation as resolved.
|
||||||
| id: resolve-rn-artifacts | ||||||
| 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"') | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or alternatively: update |
||||||
| echo "VERSION=$VERSION" >> "$GITHUB_OUTPUT" | ||||||
|
|
||||||
| - name: Cache react-native artifacts | ||||||
|
mateuuszzzzz marked this conversation as resolved.
|
||||||
| # v5.0.1 | ||||||
| uses: actions/cache@9255dc7a253b0ccc959486e2bca901246202afeb | ||||||
| with: | ||||||
| path: ~/Library/Caches/Expensify/react-native-artifacts | ||||||
| key: ${{ runner.os }}-rn-artifacts-react-hybrid-${{ steps.resolve-rn-artifacts.outputs.VERSION }}-dsyms-${{ inputs.download-dsyms }} | ||||||
|
|
||||||
| - name: Cache Pod dependencies | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that the Pods cache key doesn't account for
We could append
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| # v5.0.1 | ||||||
| uses: actions/cache@9255dc7a253b0ccc959486e2bca901246202afeb | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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), butrock.config.mjs:24-34does not includeRCT_SYMBOLICATE_PREBUILT_FRAMEWORKSin 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 tofingerprint.envor otherwise force distinct native fingerprints for the two modes.Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.