-
Notifications
You must be signed in to change notification settings - Fork 68
feat: add xcode dSYM upload script #412
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: feat/error-tracking
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,84 @@ | |||
| #!/bin/bash | |||
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.
we should call this with a .sh extension (and give chmod +x permission if not yet)
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.
Already committed with +x permission so we are good there.
Curious about the .sh extension? I believe it's not really needed since the shebang in the file should be sufficient to tell the system how to execute the file, and this will always run on mac. I personally find it more readable in Xcode build script. No strong opinions though, just curious for the reasoning here
| exit 0 | ||
| fi | ||
|
|
||
| # Find posthog-cli (Xcode doesn't load shell profiles) |
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 we matching the react native one here?
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.
I think they matched with a previous commit. Updated to use the same search locations. Don't know if npm local makes sense for iOS unless in case of monorepos?
| # Pass version info from Xcode build settings (overrides plist extraction) | ||
| if [ -n "${PRODUCT_BUNDLE_IDENTIFIER}" ]; then | ||
| CLI_ARGS="$CLI_ARGS --project $PRODUCT_BUNDLE_IDENTIFIER" | ||
| fi | ||
| if [ -n "${MARKETING_VERSION}" ]; then | ||
| CLI_ARGS="$CLI_ARGS --version $MARKETING_VERSION" | ||
| fi | ||
| if [ -n "${CURRENT_PROJECT_VERSION}" ]; then | ||
| CLI_ARGS="$CLI_ARGS --build $CURRENT_PROJECT_VERSION" | ||
| fi |
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.
we dont do this for the other platforms yet, but we should of course
just wondering if we are passing the right values here since i know the cli used different naming for things such as --project, is that the app namespace? why is it called project?
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.
My understanding is that project is the cli's cross-platform term for app namespace/identifier. Looking at releases.rs and the git release builder that uses repo name as the project seems to suggest this. @PostHog/team-error-tracking can confirm though
| # Pass main target dSYM name for accurate version extraction | ||
| if [ -n "${DWARF_DSYM_FILE_NAME}" ]; then | ||
| CLI_ARGS="$CLI_ARGS --main-dsym $DWARF_DSYM_FILE_NAME" | ||
| fi |
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.
why do we have directory and main-dsym params? why do we have a special main-dsym cmd?
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.
-directory points to DWARF_DSYM_FOLDER_PATH which contains all dSYM bundles from the current build (app + frameworks + dependencies)
-main-dsym this tells posthog-cli which dSYM to use to extract version info from plist (this will be the app's dsym). I added this in case the following lines fail to extract and pass version info directly from Xcode build settings
| CLI_ARGS="$CLI_ARGS --main-dsym $DWARF_DSYM_FILE_NAME" | ||
| fi | ||
|
|
||
| # Pass version info from Xcode build settings (overrides plist extraction) |
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.
why do we need this if the CLI seems to be parsing the info plist already?
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.
plist extraction is a fallback, not the primary source. Xcode build settings are preferred since we know we are reading version info from the correct target. No strong opinion here but if I had to chose I would stick with build vars
💡 Motivation and Context
Adds upload symbols script to be added as an Xcode build phase script
Assumes posthog-cli command from PostHog/posthog#43195
posthog-cli exp dsym upload \ --directory <path> # Directory containing dSYM bundles (required) --main-dsym <filename> # Main target's dSYM name (optional) --project <bundle_id> # App bundle identifier (optional) --version <version> # Marketing version e.g. "3.12.3" (optional) --build <build_number> # Build number e.g. "424" (optional)#skip-changelog
💚 How did you test it?
📝 Checklist