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

Add symbols to MSI #1364

Merged
merged 3 commits into from
Apr 14, 2021
Merged

Add symbols to MSI #1364

merged 3 commits into from
Apr 14, 2021

Conversation

kevingosse
Copy link
Collaborator

@kevingosse kevingosse commented Apr 13, 2021

Include the PDB symbols to the installer

@kevingosse kevingosse requested a review from a team as a code owner April 13, 2021 10:47
@colin-higgins
Copy link
Member

Those wix files are actually kept in sync with the PrepareRelease tool.
We should probably add a comment mentioning they are auto-generated.
image
https://github.com/DataDog/dd-trace-dotnet/blob/master/build/tools/PrepareRelease/Program.cs#L45
https://github.com/DataDog/dd-trace-dotnet/blob/master/build/tools/PrepareRelease/SyncMsiContent.cs

@lucaspimentel
Copy link
Member

For the dlls we install in the GAC, we should copy the PDBs into %WINDIR%\System32\Symbols\dll (in addition to Program Files like you did here).

@kevingosse
Copy link
Collaborator Author

For the dlls we install in the GAC, we should copy the PDBs into %WINDIR%\System32\Symbols\dll (in addition to Program Files like you did here).

I've considered it, but I'm not sure we should. I don't really like putting additional files into System32, and it doesn't bring much (the symbols are still available in the Datadog folder, the developer just has to load them manually).

Not to mention that the GAC is versioned and %WINDIR%\System32\Symbols\dll wouldn't be.

Copy link
Member

@lucaspimentel lucaspimentel left a comment

Choose a reason for hiding this comment

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

See @colin-higgins's comment about the PrepareRelease tool: #1364 (comment)

And regenerate the wxs files
@kevingosse
Copy link
Collaborator Author

Those wix files are actually kept in sync with the PrepareRelease tool.

I was wondering why nobody ever reformatted those files 😄

@@ -11,18 +11,36 @@
Source="$(var.TracerHomeDirectory)\net45\Datadog.Trace.AspNet.dll"
KeyPath="yes" Checksum="yes" Assembly=".net"/>
</Component>
<Component Win64="$(var.Win64)">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we need additional logic...we don't want the pdb files to be added to this "Net45.GAC" file or the "Net461.GAC" file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ugh, good catch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in fe760dd

Copy link
Collaborator

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

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

LGTM good stuff!

@kevingosse kevingosse merged commit 8b69238 into master Apr 14, 2021
@kevingosse kevingosse deleted the kevin/symbols_msi branch April 14, 2021 16:16
@andrewlock andrewlock added this to the 1.26.0 milestone Apr 19, 2021
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.

5 participants