-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Capture ios|android sourcemaps as deploy job artifacts #9641
Capture ios|android sourcemaps as deploy job artifacts #9641
Conversation
"Bundle React Native code and images" is the phase that generates the JS bundle using `node_modules/react-native/scripts/react-native-xcode.sh` Providing a SOURCEMAP_FILE env variable would make the script generate a `.map` at build time The file can then be accessed from App project root as `ios.jsbundle.map`
At the moment there's a big in react-native with SOURCEMAP_FILE and hermes enabled facebook/react-native#32497
d6378f2
to
eabc197
Compare
@@ -32,7 +32,9 @@ | |||
"gh-actions-build": "./.github/scripts/buildActions.sh", | |||
"gh-actions-validate": "./.github/scripts/validateActionsAndWorkflows.sh", | |||
"analyze-packages": "ANALYZE_BUNDLE=true webpack --config config/webpack/webpack.common.js --env.envFile=.env.production", | |||
"check-metro-bundler-port": "node config/checkMetroBundlerPort.js" | |||
"check-metro-bundler-port": "node config/checkMetroBundlerPort.js", | |||
"symbolicate:android": "npx metro-symbolicate android/app/build/generated/sourcemaps/react/release/index.android.bundle.map", |
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.
Are these scripts used by react native automatically?
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.
Are these scripts used by react native automatically?
I'm not sure what do you mean - how would they be used automatically by react-native?
I've outlined sample usage in the "Details" section.
Something like this:
- Build pipeline runs and saves sourcemaps somewhere
- Firebase Crashlytics captures a crash
- Something uses crash report data to retrieve the source maps for the build (version)
- And runs
npm run symbolicate:{platform} < crashlytics.stactrace.txt
where.stacktrace.txt
are js stack traces from "Firebase Crashlytics"
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.
Awesome, thanks for the explanation. Sounds like a good plan
@roryabraham you mind being a full reviewer here as well? I'm a bit out of my understanding here and want to make sure I have a qualified set of second 👀 |
@@ -93,6 +93,12 @@ jobs: | |||
env: | |||
VERSION: ${{ env.VERSION_CODE }} | |||
|
|||
- name: Archive Android sourcemaps | |||
uses: actions/upload-artifact@v3 |
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.
Just noting here that even though we normally use SHAs for community actions instead of vX
tags, I think it might be fine to make exceptions for GitHub-maintained actions in the actions/
org.
What do you think @AndrewGable ?
@@ -32,7 +32,9 @@ | |||
"gh-actions-build": "./.github/scripts/buildActions.sh", | |||
"gh-actions-validate": "./.github/scripts/validateActionsAndWorkflows.sh", | |||
"analyze-packages": "ANALYZE_BUNDLE=true webpack --config config/webpack/webpack.common.js --env.envFile=.env.production", | |||
"check-metro-bundler-port": "node config/checkMetroBundlerPort.js" | |||
"check-metro-bundler-port": "node config/checkMetroBundlerPort.js", | |||
"symbolicate:android": "npx metro-symbolicate android/app/build/generated/sourcemaps/react/release/index.android.bundle.map", |
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.
Awesome, thanks for the explanation. Sounds like a good plan
npm has a |
npm has a |
The change is intentional - we added new scripts to |
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.
AFAICT it looks good on my end, will wait for the final comments/review
So I got an Android production build working and see the sourcemap being generated. So that's good 👍 However, using the following stacktrace copy/pasted from Firebase Crashlytics:
Then running
|
I had the same error, I've updated the description with
The line |
Cool, that worked 👍 My only other critique is that when we execute this via npm script i.e:
We can workaround that by just adding the Just two things to take note of that we'll need to workaround in the next phase where we hook up decoded stacktraces with the PHP <-> Firebase integration (cc @AndrewGable): |
Triggered auto assignment to @PauloGasparSv ( |
@roryabraham looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
Not an emergency, tests were passing |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@roryabraham |
🚀 Deployed to production by @luacmartins in version: 1.1.85-8 🚀
|
Details
Capture sourcemap files as deploy step artifacts
The artifacts can then be used in another job/workflow to decode Firebase Crashlytics stacktraces
Sample workflow
npm run symbolicate:{platform} < crashlytics.stactrace.txt
to decode the stack tracesThe symbolicate command should be able to replace just the stack trace content of a text containing stack traces leaving the rest of the text intact
Storing and sharing artifacts between workflows: https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts
Fixed Issues
$ #9293
Tests
Requirements to be able to test:
Test Steps
Verify production builds are producing source map files:
npx react-native run-ios --configuration Release --device "Your Device Name"
main.jsbundle.map
should be available inside the project root -Expensify/App/main.jsbundle.map
npx react-native run-android --variant=release
android/app/build/generated/sourcemaps/react/release/index.android.bundle.map
should be availableUse the generated source maps to symbolicate (decode) the stack traces
symbolicate:{platform}
commands{platform}.stacktrace.txt
filenode_modules/react-native-onyx/lib/Onyx.js
Notes on stack traces format
Decoding would fail or be incorrect if the stack traces format does not match
The format should be
{anyText}:{lineNumber}:{colNumber}
anyText
is preserved whilelineNumber:colNumber
are replaced with original file and original line:columnLine numbers should start from
1
(not 0)More details and examples of valid/invalid and decoded stack traces can be seen here: #9293 (comment)
Example
Minified stack trace content
Steps
android.stacktrace.txt
npm run symbolicate:android < android.stacktrace.txt > android.decoded.stacktrace.txt
stack trace is decoded to
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
After this is deployed, verify that you can download the sourcemap artifacts for the
platformDeploy.yml
workflow run in which this was deployed.No code changes
Minimal changes to ios build process - the app should still build and launch as before
Screenshots
Web
n/a
Mobile Web
n/a
Desktop
n/a
iOS
Android