feat(cli): add dsym upload command#43195
Merged
Conversation
4 tasks
marandaneto
reviewed
Dec 16, 2025
marandaneto
reviewed
Dec 16, 2025
marandaneto
reviewed
Dec 16, 2025
marandaneto
reviewed
Dec 16, 2025
Member
|
left a few questions related to this PR here as well |
marandaneto
requested review from
a team,
ablaszkiewicz,
daibhin,
hpouillot and
oliverb123
hpouillot
reviewed
Dec 16, 2025
ablaszkiewicz
requested changes
Dec 16, 2025
| thiserror = "2.0.12" | ||
| tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } | ||
| posthog-rs = { version = "0.3.5", default-features = false } | ||
| sha2 = "0.10.9" |
Contributor
There was a problem hiding this comment.
Remember to bump the version. It is described in ./cli/CONTRIBUTING.md
Contributor
Author
There was a problem hiding this comment.
Should this be done on this branch or on a release branch as described in ./cli/CONTRIBUTING.md btw?
Contributor
There was a problem hiding this comment.
On this branch.
Once you have required approvals and all is green etc, you can push the tag in the format described in that file to this branch and then merge this branch to master
Contributor
There was a problem hiding this comment.
Ah but bump the version and update the lock file before that :)
Contributor
There was a problem hiding this comment.
Pushing the tag will trigger a release immediately
Contributor
Author
|
Hey guys, tried to address all the comments above so when you have a chance please have another look. We won't be using this right now, not until we start working on symbolication, so we may probably revisit anw |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
ablaszkiewicz
approved these changes
Dec 22, 2025
Contributor
ablaszkiewicz
left a comment
ablaszkiewicz
left a comment
There was a problem hiding this comment.
Looks good to me. Agree that we can revisit this
Contributor
Author
|
Looks like release has failed here https://github.com/PostHog/posthog/actions/runs/20460693242 |
Contributor
|
Release workflow has been fixed, for context: https://posthog.slack.com/archives/C0113360FFV/p1767019809101999 |
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Contributor
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+57
to
+66
| pub fn into_uploads(self) -> Vec<SymbolSetUpload> { | ||
| self.uuids | ||
| .into_iter() | ||
| .map(|uuid| SymbolSetUpload { | ||
| chunk_id: uuid, | ||
| release_id: self.release_id.clone(), | ||
| data: self.data.clone(), | ||
| }) | ||
| .collect() | ||
| } |
Contributor
There was a problem hiding this comment.
logic: For universal binaries with multiple architectures, cloning self.data for each UUID creates N full copies of potentially large dSYM data in memory. Consider using Arc<Vec<u8>> to share the data across uploads instead of cloning.
Suggested change
| pub fn into_uploads(self) -> Vec<SymbolSetUpload> { | |
| self.uuids | |
| .into_iter() | |
| .map(|uuid| SymbolSetUpload { | |
| chunk_id: uuid, | |
| release_id: self.release_id.clone(), | |
| data: self.data.clone(), | |
| }) | |
| .collect() | |
| } | |
| impl DsymFile { | |
| /// Convert to SymbolSetUploads (one per UUID/architecture) | |
| pub fn into_uploads(self) -> Vec<SymbolSetUpload> { | |
| let data = std::sync::Arc::new(self.data); | |
| self.uuids | |
| .into_iter() | |
| .map(|uuid| SymbolSetUpload { | |
| chunk_id: uuid, | |
| release_id: self.release_id.clone(), | |
| data: (*data).clone(), // Still need the Vec for API, but Arc avoids duplication | |
| }) | |
| .collect() | |
| } | |
| } |
Note: If SymbolSetUpload is refactored to use Arc<Vec<u8>>, this becomes cleaner.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/dsym/mod.rs
Line: 57:66
Comment:
**logic:** For universal binaries with multiple architectures, cloning `self.data` for each UUID creates N full copies of potentially large dSYM data in memory. Consider using `Arc<Vec<u8>>` to share the data across uploads instead of cloning.
```suggestion
impl DsymFile {
/// Convert to SymbolSetUploads (one per UUID/architecture)
pub fn into_uploads(self) -> Vec<SymbolSetUpload> {
let data = std::sync::Arc::new(self.data);
self.uuids
.into_iter()
.map(|uuid| SymbolSetUpload {
chunk_id: uuid,
release_id: self.release_id.clone(),
data: (*data).clone(), // Still need the Vec for API, but Arc avoids duplication
})
.collect()
}
}
```
Note: If `SymbolSetUpload` is refactored to use `Arc<Vec<u8>>`, this becomes cleaner.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+117
to
+127
| let resolved_project = project.clone().or_else(|| { | ||
| plist_info | ||
| .as_ref() | ||
| .and_then(|p| p.bundle_identifier.clone()) | ||
| }); | ||
| let resolved_version = version | ||
| .clone() | ||
| .or_else(|| plist_info.as_ref().and_then(|p| p.short_version.clone())); | ||
| let resolved_build = build | ||
| .clone() | ||
| .or_else(|| plist_info.as_ref().and_then(|p| p.bundle_version.clone())); |
Contributor
There was a problem hiding this comment.
style: Unnecessary clone operations - resolved_project, resolved_version, and resolved_build are already Option<String> and only used once after cloning. Use .as_ref() or reference the original options directly instead.
Suggested change
| let resolved_project = project.clone().or_else(|| { | |
| plist_info | |
| .as_ref() | |
| .and_then(|p| p.bundle_identifier.clone()) | |
| }); | |
| let resolved_version = version | |
| .clone() | |
| .or_else(|| plist_info.as_ref().and_then(|p| p.short_version.clone())); | |
| let resolved_build = build | |
| .clone() | |
| .or_else(|| plist_info.as_ref().and_then(|p| p.bundle_version.clone())); | |
| // Determine project, version, build - CLI args take precedence over plist | |
| let resolved_project = project.as_ref().or_else(|| { | |
| plist_info | |
| .as_ref() | |
| .and_then(|p| p.bundle_identifier.as_ref()) | |
| }); | |
| let resolved_version = version | |
| .as_ref() | |
| .or_else(|| plist_info.as_ref().and_then(|p| p.short_version.as_ref())); | |
| let resolved_build = build | |
| .as_ref() | |
| .or_else(|| plist_info.as_ref().and_then(|p| p.bundle_version.as_ref())); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/dsym/upload.rs
Line: 117:127
Comment:
**style:** Unnecessary clone operations - `resolved_project`, `resolved_version`, and `resolved_build` are already `Option<String>` and only used once after cloning. Use `.as_ref()` or reference the original options directly instead.
```suggestion
// Determine project, version, build - CLI args take precedence over plist
let resolved_project = project.as_ref().or_else(|| {
plist_info
.as_ref()
.and_then(|p| p.bundle_identifier.as_ref())
});
let resolved_version = version
.as_ref()
.or_else(|| plist_info.as_ref().and_then(|p| p.short_version.as_ref()));
let resolved_build = build
.as_ref()
.or_else(|| plist_info.as_ref().and_then(|p| p.bundle_version.as_ref()));
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+131
to
+133
| // Create relative path within the zip | ||
| let relative_path = path.strip_prefix(dsym_path.parent().unwrap_or(dsym_path))?; | ||
| let zip_path = relative_path.to_string_lossy(); |
Contributor
There was a problem hiding this comment.
logic: The relative path calculation uses dsym_path.parent() as the strip prefix, which includes the parent directory in the zip. This appears incorrect - to preserve just the .dSYM bundle name (as intended by zip structure), strip relative to parent and include the bundle name itself.
Suggested change
| // Create relative path within the zip | |
| let relative_path = path.strip_prefix(dsym_path.parent().unwrap_or(dsym_path))?; | |
| let zip_path = relative_path.to_string_lossy(); | |
| // Create relative path within the zip (preserves .dSYM bundle name) | |
| let relative_path = path.strip_prefix(dsym_path.parent().unwrap_or(dsym_path))?; |
However, verify that the intended structure is MyApp.app.dSYM/Contents/... in the zip and not just Contents/....
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/dsym/mod.rs
Line: 131:133
Comment:
**logic:** The relative path calculation uses `dsym_path.parent()` as the strip prefix, which includes the parent directory in the zip. This appears incorrect - to preserve just the `.dSYM` bundle name (as intended by zip structure), strip relative to parent and include the bundle name itself.
```suggestion
// Create relative path within the zip (preserves .dSYM bundle name)
let relative_path = path.strip_prefix(dsym_path.parent().unwrap_or(dsym_path))?;
```
However, verify that the intended structure is `MyApp.app.dSYM/Contents/...` in the zip and not just `Contents/...`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+131
to
+133
| // Create relative path within the zip | ||
| let relative_path = path.strip_prefix(dsym_path.parent().unwrap_or(dsym_path))?; | ||
| let zip_path = relative_path.to_string_lossy(); |
Contributor
There was a problem hiding this comment.
style: The relative path includes both the .dSYM bundle name and its contents in the zip archive. This is correct but worth documenting - verify that downstream processing expects the full bundle structure (e.g., MyApp.app.dSYM/Contents/...) and not just the contents.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/dsym/mod.rs
Line: 131:133
Comment:
**style:** The relative path includes both the `.dSYM` bundle name and its contents in the zip archive. This is correct but worth documenting - verify that downstream processing expects the full bundle structure (e.g., `MyApp.app.dSYM/Contents/...`) and not just the contents.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Contributor
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Comment on lines
+147
to
+150
| // Add git info as metadata if available (but don't use it for project/version) | ||
| if let Ok(Some(git_info)) = get_git_info(Some(directory.clone())) { | ||
| release_builder.with_git(git_info); | ||
| } |
Contributor
There was a problem hiding this comment.
Git info leaks into project/version
The comment says "don't use it for project/version," but ReleaseBuilder::with_git() sets project to the git remote URL and version to the commit SHA when the builder is empty (which it always is here, since it's created via default() on line 145). The overrides on lines 157-162 only apply when resolved_project / full_version are Some.
This means if neither CLI args nor the plist provide a project or version, the release will silently be created with the git remote URL as "project" and the commit SHA as "version" — which is semantically incorrect for an iOS dSYM upload and contradicts the stated intent.
To match the comment's intent, use with_metadata directly to attach git info without populating project/version:
Suggested change
| // Add git info as metadata if available (but don't use it for project/version) | |
| if let Ok(Some(git_info)) = get_git_info(Some(directory.clone())) { | |
| release_builder.with_git(git_info); | |
| } | |
| // Add git info as metadata if available (but don't use it for project/version) | |
| if let Ok(Some(git_info)) = get_git_info(Some(directory.clone())) { | |
| let _ = release_builder.with_metadata("git", git_info); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/dsym/upload.rs
Line: 147:150
Comment:
**Git info leaks into project/version**
The comment says "don't use it for project/version," but `ReleaseBuilder::with_git()` sets `project` to the git remote URL and `version` to the commit SHA when the builder is empty (which it always is here, since it's created via `default()` on line 145). The overrides on lines 157-162 only apply when `resolved_project` / `full_version` are `Some`.
This means if neither CLI args nor the plist provide a project or version, the release will silently be created with the git remote URL as "project" and the commit SHA as "version" — which is semantically incorrect for an iOS dSYM upload and contradicts the stated intent.
To match the comment's intent, use `with_metadata` directly to attach git info without populating project/version:
```suggestion
// Add git info as metadata if available (but don't use it for project/version)
if let Ok(Some(git_info)) = get_git_info(Some(directory.clone())) {
let _ = release_builder.with_metadata("git", git_info);
}
```
How can I resolve this? If you propose a fix, please make it concise.# Conflicts: # cli/CHANGELOG.md # cli/Cargo.lock # cli/Cargo.toml
Contributor
|
Size Change: 0 B Total Size: 93.5 MB ℹ️ View Unchanged
|
Contributor
Author
|
@ablaszkiewicz Hey can I have a quick sanity check before I push tag |
marandaneto
reviewed
Feb 16, 2026
Comment on lines
+19
to
+22
| /// The bundle identifier (e.g., com.example.app). | ||
| /// If not provided, will be extracted from dSYM Info.plist. | ||
| #[arg(long)] | ||
| pub project: Option<String>, |
Member
There was a problem hiding this comment.
@ioannisj @hpouillot this was renamed in some other places (@hpouillot can share more), since we've not released this, we should fix it as well to avoid confusion
Contributor
Author
There was a problem hiding this comment.
Yeah I think this was renamed to name internally (and maps to that)
Wanted to get the bulk of this out so that I can continue work with symbolication
Some open discussions around this in PostHog/posthog-ios#412, so will follow up with a another PR here once we decide on the direction there.
Contributor
Author
There was a problem hiding this comment.
For iOS, this is just the bundle identifier (and seems to be mapping correctly). Maybe we can consider renaming the param to bundle-id even?
Member
There was a problem hiding this comment.
this has nothing to do with the bundle id, we just use the bundle id and version as identifiers for the release (since theres nothing better than that)
so i think release_name and release_version should be the ones we should use
#47510
Contributor
Author
There was a problem hiding this comment.
These are the CLI args through for dsym upload command (and we map with appropriate fields in ReleaseBuilder). For dSYM uploads specifically, --bundle-id feels more descriptive to me since it's apple only?
Agreed that --project should go though
Member
There was a problem hiding this comment.
my point is that release-name is a cli param, that can be overriden by the calleer to whatever they want, and we use the bundle id as a fallback, so calling it bundle-id does not make sense because it can be anything like 'uber driver' | 'uber passenger'
Contributor
Author
There was a problem hiding this comment.
Ah got you, fair point actually, even though will most likely be used exclusively in xcode script. In this case release_name and release_version may be better fit
marandaneto
reviewed
Feb 16, 2026
Comment on lines
+34
to
+38
| /// The main dSYM file name (e.g., MyApp.app.dSYM). | ||
| /// Used to extract version info from the correct dSYM when multiple are present. | ||
| /// This is typically $DWARF_DSYM_FILE_NAME in Xcode build phases. | ||
| #[arg(long)] | ||
| pub main_dsym: Option<String>, |
Member
There was a problem hiding this comment.
shouldnt we parse this info from plist.info instead?
Member
There was a problem hiding this comment.
ok now i see that we do this + Contents/Info.plist
marandaneto
reviewed
Feb 16, 2026
|
|
||
| # 0.5.30 | ||
|
|
||
| - Add experimental dSYM upload for iOS/macOS crash symbolication |
Member
There was a problem hiding this comment.
Suggested change
| - Add experimental dSYM upload for iOS/macOS crash symbolication | |
| - Add experimental dSYM upload for iOS/macOS stack trace symbolication |
abhischekt
pushed a commit
that referenced
this pull request
Feb 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Add CLI support to upload dSYM files through
exp dsym uploadsubcommandChanges
How did you test this code?
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Changelog: (features only) Is this feature complete?