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: windows symbol upload #607

Merged
merged 11 commits into from
Mar 9, 2022
Merged

Feat: windows symbol upload #607

merged 11 commits into from
Mar 9, 2022

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Mar 2, 2022

Part of windows native support #527

@vaind vaind changed the title Feat/windows symbol upload Feat: windows symbol upload Mar 2, 2022
@vaind vaind mentioned this pull request Mar 2, 2022
11 tasks
CHANGELOG.md Outdated Show resolved Hide resolved
// addPath(Path.GetFileNameWithoutExtension(executableName) + "_Data/Plugins/x86_64/sentry.dll");

// Configure the process using the StartInfo properties.
var process = new Process
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we would like to avoid spawning a blocking process from within the Unity editor and call Sentry CLI from a msbuild target during the build.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe an option after I figure out how to inject into the IL2CPP windows build. For "project-only" builds where unity itself doesn't compile the binaries it would also be the only option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While @bitsandfoxes is working on the integration into the unity build process for standalone windows, I'd say this PR still makes sense to merge as is now? Already brings support for the "build-in-unity" workflow

src/Sentry.Unity.Editor.iOS/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
}
};

addPath(executableName);
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we better off running sentry-cli on the root and let it find the relevant files? It'll also be more future proof if Unity introduces other native shared libraries

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've thought about that but didn't like that it would also upload:

  • UnityCrashHandler64.exe
  • crashpad_handler.exe

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean it uploads the executables? My understanding was that upload-dif would only scan for and upload debug information files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

executables also have some debug info. See for example here:

image

src/Sentry.Unity.Editor/Native/BuildPostProcess.cs Outdated Show resolved Hide resolved
@vaind vaind enabled auto-merge (squash) March 9, 2022 10:07
@vaind vaind merged commit 023d178 into main Mar 9, 2022
@vaind vaind deleted the feat/windows-symbol-upload branch March 9, 2022 10:14
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.

3 participants