-
Notifications
You must be signed in to change notification settings - Fork 694
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
[Proposal] Added IncludeSymbolsInPackage property #1971
Conversation
Workaround: NuGet/Home#4142 (comment) Proposal: NuGet/NuGet.Client#1971
@@ -45,6 +45,7 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
<ImportNuGetBuildTasksPackTargetsFromSdk Condition="'$(ImportNuGetBuildTasksPackTargetsFromSdk)' == ''">false</ImportNuGetBuildTasksPackTargetsFromSdk> | |||
<AllowedOutputExtensionsInPackageBuildOutputFolder>.dll; .exe; .winmd; .json; .pri; .xml; $(AllowedOutputExtensionsInPackageBuildOutputFolder)</AllowedOutputExtensionsInPackageBuildOutputFolder> | |||
<AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder>.pdb; .mdb; $(AllowedOutputExtensionsInPackageBuildOutputFolder); $(AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder)</AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder> | |||
<AllowedOutputExtensionsInPackageBuildOutputFolder Condition="'$(IncludeSymbolsInPackage)' == 'true'">$(AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder)</AllowedOutputExtensionsInPackageBuildOutputFolder> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather have .pdb and .mdb in a property called SymbolExtensions, and that property be appended to both AllowedOutputExtensionsInPackageBuildOutputFolder (if IncludeSymbolsInPackage is set to true) and AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohit21agrawal Sure! Would the extensions .dll; .exe; .winmd; .json; .pri; .xml
get the same treatment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I think it's fine the way it is. they aren't used any place else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the symbol extensions are prepended. Did you want me to switch it to append them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ordering doesn’t really matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment
Would like to make sure this is considered as part of our coming symbol server work. Let's not do this until we finish that detailed design beyond the current published spec. |
@rrelyea does this make it harder? It's not like it's on by default, this is still opt-in... |
Agreed with @davidfowl here, seems like the two things are potentially orthogonal. This is biting us in development right now as we were previously shipping symbols for devs to use on their dev machines during debugging with the old tooling. The new tooling isn't allowing us to do that, aside from the specified workaround, which feels brittle. |
@rohit21agrawal - Please do not merge until we talk through this and how it relates to our new work. |
Yes, not planning on it. Just approved the code review |
closing this as we have finalized the spec for newer format of symbol packages : https://github.com/NuGet/Home/wiki/NuGet-Package-Debugging-&-Symbols-Improvements |
And symbols packages will work for local nuget feeds? Or is settting up a symbol server required... |
Symbol packages do not work with local NuGet feeds. You can get around this by embedding your symbols into your NuGet package. Alternatively, you can use BaGet to setup a symbol server: https://github.com/loic-sharma/BaGet |
#shamelessplug 😄 |
Is there a sensible way to easily include symbols now? |
https://learn.microsoft.com/en-us/nuget/create-packages/symbol-packages-snupkg
When are we going to get support for debugging from private nuget feeds? |
@dcanning Azure DevOps already has symbol server support. It's just that you publish the pdbs to it directly, rather than publishing via a snupkg. Our docs page on hosting your own packages lists a bunch of nuget servers that can be used for private feeds (some are cloud hosted, some can be self hosted), and I know BaGet supports symbols, googling teamcity symbol server and artifactory symbols sever shows results that make it appear that they support symbols as well. So, I don't believe there's anything preventing private feeds from hosting symbols. It's just that the symbols are downloaded via the symbol server protocol, not NuGet's protocol, and how you upload the symbols depends on the server. |
Addresses NuGet/Home#4142. The rationale for why people are moving away from using separate symbol packages is stated well by Oren. I think in the increasing era of source-linked and source-embedded PDBs, the demand for NuGet.org package debugging to "just work" is only going to increase as well.
The current workaround (at the time NuGet/Home#4142 was closed) is too much to type, or bookmark and paste; it is not documented, and it shows its implementation details:
Also, it's missing .mdb.
AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder
takesAllowedOutputExtensionsInPackageBuildOutputFolder
and adds .pdb and .mdb.Could we please have a nicer bool property like the
IncludeSymbolsInPackage
@davidfowl suggested? It's the only future-proof or palatable long-term solution that I've seen so far. Very specifically,AllowedOutputExtensionsInPackageBuildOutputFolder
needs to be evaluated afterAllowedOutputExtensionsInSymbolsPackageBuildOutputFolder
is defined so that it can simply copy it whenIncludeSymbolsInPackage
is true. (See the commit.)Testing/Validation
Tests Added: No
Reason for not adding tests: need guidance
Validation done: