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

Include symbols in package #76

Merged
merged 1 commit into from
Jun 12, 2019
Merged

Include symbols in package #76

merged 1 commit into from
Jun 12, 2019

Conversation

kzu
Copy link
Contributor

@kzu kzu commented Jun 12, 2019

Fixes http://work.azdo.io/917288, fixes AB#917288

@kzu
Copy link
Contributor Author

kzu commented Jun 12, 2019

Heya @jeffkl how quickly can we get an updated nuget package so we can bump the dep and trigger a new build + insertion round?

@jeffkl
Copy link
Owner

jeffkl commented Jun 12, 2019

Would it be better to push a symbol package or use SourceLink? I thought it was a bad practice to include PDBs in NUPKGs?

@kzu
Copy link
Contributor Author

kzu commented Jun 12, 2019

Not really. SourceLink is orthogonal to including the PDB. What's sort of considered "bad" practice, AFAIK, is to actually embed the PDB in the DLL, since it might be used in a resource-constrained device (i.e. via Xamarin) and cause unnecessary memory consumption. However, this is not the case here.
Also, it's a PPDB, so it's really really tiny compared to what placing a full PDB was, so, it should be fine.

Finally, for the purposes of VS insertion symbol check, the only thing that would make it green is if in the process of publishing this package, you also uploaded to that symbol server, which I think is way overkill.

@jeffkl
Copy link
Owner

jeffkl commented Jun 12, 2019

I'm not 100% convinced this is correct but I'd rather unblock you. Is Microsoft shipping this library?

@jeffkl jeffkl merged commit 102682d into jeffkl:master Jun 12, 2019
@jeffkl
Copy link
Owner

jeffkl commented Jun 12, 2019

Package is pushed, awaiting validation: https://www.nuget.org/packages/Microsoft.Dism/2.0.16

@kzu
Copy link
Contributor Author

kzu commented Jun 12, 2019

It seems we're using it as part of our Xamarin Android Device Manager. @garuma would know what for :)

@kzu
Copy link
Contributor Author

kzu commented Jun 12, 2019

Cool, bumped our dep, on track for insertion! Thanks a lot @jeffkl 💯

@jeffkl
Copy link
Owner

jeffkl commented Jun 12, 2019

No problem, thanks for the contribution

kzu added a commit to kzu/Polly that referenced this pull request Aug 22, 2019
Given that the new portable PDBs generated for NS projects are really small for the added benefit of 
useful stacktraces on errors, it would complement the sourcelink that's already in place. 

In particular, for VS extensions that use this package, including symbols in the package makes it easier 
for crash dumps to contain useful stacktraces. i.e. see jeffkl/ManagedDism#76

This would fix bug http://work.azdo.io/971173 too :)
reisenberger pushed a commit to App-vNext/Polly that referenced this pull request Oct 5, 2019
Given that the new portable PDBs generated for NS projects are really small for the added benefit of 
useful stacktraces on errors, it would complement the sourcelink that's already in place. 

In particular, for VS extensions that use this package, including symbols in the package makes it easier 
for crash dumps to contain useful stacktraces. i.e. see jeffkl/ManagedDism#76

This would fix bug http://work.azdo.io/971173 too :)
reisenberger pushed a commit to App-vNext/Polly that referenced this pull request Nov 27, 2019
Given that the new portable PDBs generated for NS projects are really small for the added benefit of 
useful stacktraces on errors, it would complement the sourcelink that's already in place. 

In particular, for VS extensions that use this package, including symbols in the package makes it easier 
for crash dumps to contain useful stacktraces. i.e. see jeffkl/ManagedDism#76

This would fix bug http://work.azdo.io/971173 too :)
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.

2 participants