Skip to content
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

feat: extract and upload embedded Portable PDB from PE #1463

Merged
merged 7 commits into from
Feb 8, 2023

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Feb 7, 2023

If CLI finds a PE file with embedded PPDB, it extracts the file and uploads it separately. This way, we don't need on any custom handling in the symbolicator and symbolic-debuginfo (feature flags) and also we don't upload the PE file just for the sake of the .pdb.

The following shows DIF uploads for samples\Sentry.Samples.Console.Basic\Sentry.Samples.Console.Basic.csproj with <DebugType>embedded</DebugType>
image

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm.
Though I do not fully understand the screenshot you posted.

Why do we suddenly get two files? Shouldn’t the portable pdb have everything in it? Or is it rather that sentry-cli creates a separate sourcebundle because you used --include-sources and it was collecting all the local files referenced from the portable pdb?

@vaind
Copy link
Collaborator Author

vaind commented Feb 8, 2023

Why do we suddenly get two files? Shouldn’t the portable pdb have everything in it? Or is it rather that sentry-cli creates a separate sourcebundle because you used --include-sources and it was collecting all the local files referenced from the portable pdb?

Yes, that run was without "embedded sources" (<EmbedAllSoruces> flag in the project) so the CLI looked for sources manually and created a source bundle

@Swatinem
Copy link
Member

Swatinem commented Feb 8, 2023

Perfect, that makes sense! Will it do the same when you actually have embedded sources though?

@vaind
Copy link
Collaborator Author

vaind commented Feb 8, 2023

Perfect, that makes sense! Will it do the same when you actually have embedded sources though?

No, it's just a single file. I was trying to find my yesterday's attempt but there were too many so wasn't sure which one, so ran it again now:
image

BTW it doesn't show that it includes sources, because Sentry isn't updated with the lastest symbolic v11 - getsentry/sentry#43832

@vaind vaind force-pushed the feat/pe-embedded-ppdb branch from 4b2cfc0 to c7d0791 Compare February 8, 2023 12:58
@vaind vaind force-pushed the feat/pe-embedded-ppdb branch from c7d0791 to 87d6ad5 Compare February 8, 2023 13:03
@vaind vaind marked this pull request as ready for review February 8, 2023 14:50
@vaind vaind requested a review from Swatinem February 8, 2023 14:50
src/utils/dif.rs Outdated Show resolved Hide resolved
src/utils/dif_upload.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embedded symbols/sources not found
2 participants