BlackGad commented 3 years ago

Details about Problem

NuGet system implementation currently has almost all parts for full development cycle with symbols.

This is basically first stop on the long road to resolving target issue https://github.com/NuGet/Home/issues/6579.

So lets start with stages of full development cycle:

Create and publish - Includes build project, pack binary package, pack symbols package and publish it to the registries.
Restore and build - Includes binary package restore and dependent project build.
Launch and debug - Execute application from binaries, debugger attachment etc.

Current implementation state

Create and publish

pack

This stage fully implemented. Tools and applications which uses NuGet can pack binaries into packages accordingly to nuspec configuration. With additional options they can automatically create snupkg symbols packages. Also v3 web registries can consume binary AND symbols packages for future usage.

From technical perspective both .nupkg and .snupkg similar because same code used for their creation (just minor nuspec changes and exclude filters).

Update: Official NuGet registry reformat snupkg package internally to Symbols artifact for built-in Symbols server

Restore and build

Build_current

Stage begins from restore step. Tools and applications restores existing (install new ones) packages into specific folder to allow builder (Visual Studio on a picture) compose real application. NuGet protocol(framework) has built in discovery and cache mechanisms that why actual tools do not care about implementation. Everything what they must to do is call proper API methods.

Launch and debug

Debug_current_1

Executing our binaries and attach debugger (Visual Studio here). In order to debug external binaries it is required to load their symbols. Generic approach for Visual Studio is to use symbols from only one of three places:

The location that is specified inside the DLL or the executable (.exe) file.
The same folder as the DLL or .exe file.
Debugger options for symbol files.

Picture above describes current Visual Studio integration for .snupkg (Option 3). Visual Studio downloads from existing NuGet symbol server Symbols and map them to actual runtime.

Issues

Nupkg package structure MUST follow recommended format.
It is REQUIRED to have v3 WEB registry with additional symbols store and symbols server (for future debugging).
And I am quiet about debuggers which differs from Visual Studio. (All steps above must be implemented separately because NuGet API has no methods to do this)

Purposed implementation

We need to use Visual Studio The same folder as the DLL or .exe file pdb discovery feature.

Create and publish

Already ready for usage.

Restore and build

Build_purposed

Main difference here is handling snupkg packages as generic nupkg. This means:

Introduce new options for restore/install commands for symbols restore (as flags)
Create symbols global cache
Implement handlers for all resources into the protocol (File based v2/v3, Http v2/v3)
Implement nupkg and snupkg merge mechanism into one destination folder

When all symbols placed near binaries Visual Studio (MSBuild actually but whatever) will copy them to target folder.

Launch and debug

Debug_purposed

Magically there will be no additional NuGet specific logic for symbols. Visual Studio will resolve pdb files which placed near the binaries.

Purpose

Main purpose of this issue to begin discussion prior to Nuget/Home documentation PR and actual implementation.

Existing covered issues

https://github.com/NuGet/Home/issues/9667 https://github.com/NuGet/Home/issues/9456 https://github.com/NuGet/Home/issues/1696

Discovered problems

PackageReference do not support satellite files copy (.pdb and .xml files). https://github.com/NuGet/Home/issues/5926

BlackGad commented 3 years ago

@zivkan , @zkat , @nkolev92 what do you thing about this?

JonDouglas commented 3 years ago

There may be some additional context into symbols here as well:

https://github.com/NuGet/docs.microsoft.com-nuget/issues/2343

BlackGad commented 3 years ago

Any thoughts ? I dont want to develop feature before agreement from you guys :) @zkat, @zivkan, @nkolev92

zivkan commented 3 years ago

I haven't had time to look in detail, but I see a slight misunderstanding of how symbols currently work.

When you push snupks to nuget.org, it unzips the pdbs and sends them to a symbol server. Then when debugging, the debugger (VS or windbg, or whatever else) talk use the symbols protocol (that existed a long time before NuGet was ever created) to download the pdbs. Debuggers in fact know nothing about NuGet or snupkgs.

Your proposal basically is taking the HTTP symbol server out of the equation. I think you're proposing that restore should copy the pdbs locally, so there's "never" a need to download pdbs from a symbol server. However:

I'm not sure that snupkgs are part of the NuGet HTTP protocol. If not, there needs to be a standard for all nuget feeds, not just nuget.org, to implement, so that clients can download snupks.
I'm not sure that pdbs in the same directory as dlls works for PackageReference: https://github.com/NuGet/Home/issues/5926

This appears to be a feature request that affects multiple components of the .NET ecosystem, so perhaps a proposal in github.com/dotnet/designs is warranted, as it might require product changes outside of NuGet.

I dont want to develop feature before agreement from you guys

yes, please don't spend effort implementing something for a design that is not approved. Since this is not work we planned in advance, we don't necessarily have time to review and give feedback in short notice. Especially when the request requires cross-team collaboration. I know it's frustrating that this makes the process slower, but it is what it is.

BlackGad commented 3 years ago

Initial issue text was updated.

I'm not sure that snupkgs are part of the NuGet HTTP protocol. If not, there needs to be a standard for all nuget feeds, not just nuget.org, to implement, so that clients can download snupks.

Yes it is. v3 Nuget registry contains section

https://api.nuget.org/v3/index.json

{
  "version": "3.0.0",
  "resources": [
   {
      "@id": "https://www.nuget.org/api/v2/symbolpackage",
      "@type": "SymbolPackagePublish/4.9.0",
      "comment": "The gallery symbol publish endpoint."
    }
  ]
}

Pushed symbols package can be directly downloaded via link

https://www.nuget.org/api/v2/symbolpackage/<package id>/<package version>

Same structure in MyGet and BaGet. NuGet have functionality to push snupkg but not restore.

I'm not sure that pdbs in the same directory as dlls works for PackageReference: #5926

You are right. v3 project structure requires additional handling.

What else is incorrect?

From my point of view we need to implement restore functionality at least for v2 at start (maintain symbols package cache as well). It will allow to update v3 PackageReference with additional package and symbols merge logic (based not on satellites but on different sources from cache)

joelverhagen commented 3 years ago

For the V3 protocol support for .snupkg files I will make one clarification:

Yes it is. v3 Nuget registry contains section

It is true there is a standard protocol for uploading (a.k.a. pushing) .snupkg files. The official documentation for what you describe is here: https://docs.microsoft.com/en-us/nuget/api/symbol-package-publish-resource

However, the is no official protocol for downloading the .snupkg. In fact, the only reason the nuget.org has a way to download .snupkg files after pushing is to enable the web UI link on the package details page:

If we want some NuGet client-based flow for PDB downloading, I think we would have to add something new to the protocol. I see two options:

Advertise the symbol server URL in the V3 service index, some additional resource that advertises https://symbols.nuget.org/ in NuGet.org's case. NuGet Client could implement the same PDB download mechanism as the debugger based on this. This has the drawback of N requests per package (a package can have N DLLs and therefore N PDBs). We could add a V3 resource like:
```
{
    "@id": "https://symbols.nuget.org/",
    "@type": "SymbolDownload/6.0.0",
    "comment": "The symbol server base URL for downloading individual PDB files."
}
```
Advertise the .snupkg location for download. This feature request already exists here: https://github.com/NuGet/NuGetGallery/issues/7912. NuGet Package Explorer currently hardcodes the unofficial CDN URL for downloading .snupkg files. This is risky from a client perspective since it could change someday so if NuGet Client did this we would want to formalize the protocol in a specification, e.g. a new package content resource version:
```
{
    "@id": "https://api.nuget.org/v3-flatcontainer/",
    "@type": "PackageBaseAddress/6.0.0",
    "comment": "Base URL of where NuGet packages are stored including optional .snupkg files."
}
```

(This is a just a straw man to demonstrate what is missing, not much more 😄)

BlackGad commented 3 years ago

This is a just a straw man to demonstrate what is missing, not much more

This issue was created exactly for this purpose - determine what is missing. :)

Ok let's describe symbols restore feature separately for v2 and v3 C# projects:

V2 project

1) Used stand-alone source folder (default packages can be redirected via NuGet.config). NuGet also respects local PC NuGet cache folder. 2) MSBuild reference insertion looks like

<Reference Include="<assembly name>">
    <HintPath>..\packages\package_id.package_version>\lib\platform\assembly_name.dll</HintPath>
</Reference>

3) MSBuild main build task copies Reference items to output folder (satellite files as well pdb, xml etc)

restore_v2

Cases:

1) NuGet web registry -> NuGet (any local tool) -> Packages folder -> MSBuild Reference (will copy assemblies and symbols if present) -> Bin 2) NuGet local registry -> NuGet (any local tool) -> Packages folder -> MSBuild Reference (will copy assemblies and symbols if present) -> Bin

NuGet web registry - requires new service index SymbolDownload standardization

NuGet (any local tool) - can be implemented right now

MSBuild Reference - ready to use

NuGet local registry - can be implemented right now

V3 project

1) As a package source local PC NuGet cache folder is used (default c:\Users\User.nuget\packages\ can be redirected via environment variables or global NuGet configuration) 2) MSBuild reference insertion looks like

<PackageReference Include="<package id>" Version="<package version>" />

3) MSBuild build task copies required assembly to output folder accordingly to current build platform configuration (satellite files do not copied but we have https://github.com/NuGet/Home/issues/5926 )

restore_v3

Cases:

1) NuGet web registry -> NuGet (any local tool) -> MSBuild PackageReference (will restore assemblies only) -> Bin 2) NuGet local registry -> NuGet (any local tool) -> MSBuild PackageReference (will restore assemblies only) -> Bin

NuGet web registry - requires new service index SymbolDownload standardization

NuGet (any local tool) - can be implemented right now

MSBuild PackageReference - requires additional logic for symbols restore

NuGet local registry - can be implemented right now

So right now we can implement at least 1 case(v2.1) end to end. In case of implementation NuGet protocol will be able to use snupkg from local folders and process cache.

nkolev92 commented 3 years ago

So right now we can implement at least 1 case(v2.1) end to end. In case of implementation NuGet protocol will be able to use snupkg from local folders and process cache.

I just want to set expectations here because the challenges are not just technical. We'd need to validate the experience is the needed/worth it before we go ahead with implementation.

That being said, I love the write-up! Thanks for getting this conversation going!

BlackGad commented 3 years ago

Will describe my vision of symbols restore process for clients.

What to add

Options that will force to restore snupkg as well as nupkg.

New -Symbols option to nuget.exe restore command that will force to restore all available symbols for all installed packages.
New -Symbols option to nuget.exe install command will force to restore specific package symbols.
New --symbols option to dotnet restore command that will force to restore all available symbols for all installed packages.
New --symbols option to dotnet add package command will force to restore specific package symbols.

For proper integration into real project - symbols restore configuration must be placed in new file (similar to nuget.config for example nuget.config.user) on solution level. It is crucial to have ability to exclude it from subversion.

Inside Visual Studio in the NuGet Package Manager on Installed tab for every package add option for symbols install as well (configuration placed in nuget.config.user). Built-in Restore mechanism will respect this option in same way as common package miss situation.

What it will do

nuget.exe restore and dotnet restore commands will do their job with symbols restore option only for configured packages.

Based on source project version above commands will:

For any project structure will download and place package and symbols content into the cache folders (if cache allowed). Default place for package content is c:\Users\<user>\.nuget\packages\. Default place for symbols content may be c:\Users\<user>\.nuget\symbols\.
In old project structure will merge package and symbols content into destination packages folder (in reverse to split pack operation).
In new project structure will do same the operation if packages folder set in nuget.config file.

Further symbols transport will be handled by MSBuild. In old project structure symbols will be copied as assembly satellites to output folder and in new project structure this action will be handled by PackageReference (copy satellites from external packages folder or gather assemblies and symbols from the cache).

Also it is important to implement symbols discovery operation in registries independent from package registries. This will allow to place .snupkg files in local NuGet folder (local file based registry).

Debug flow example

Disposition:

nupkg lives in super secured internal corporate feed based on unknown engine.
snupkg lives only as artifacts in CI
Application dependent from nupkg from registry with defect.

In order to debug application properly developer will:

Open application source code
Configure Visual Studio local NuGet packages folder.
Download required snupkg files and place them to it.
Setup symbols restore for required NuGet packages
Restore packages and symbols
Build application
Debug it.

BlackGad commented 3 years ago

@nkolev92 what do you think about above flow?

zivkan commented 3 years ago

We should make improvements to symbols and debugging experience, so don't interpret this as avoiding any change. But if you're looking for a way to do debugging with tools available today, I just remembered that BaGet has a built-in symbol server. Use this for your company internal NuGet feed, and I believe you can get it working.

BlackGad commented 3 years ago

We should make improvements to symbols and debugging experience, so don't interpret this as avoiding any change. But if you're looking for a way to do debugging with tools available today, I just remembered that BaGet has a built-in symbol server. Use this for your company internal NuGet feed, and I believe you can get it working.

I want to improve local PC debug experience at the end :) all things above are first steps that are missed. NuGet have artifacts split functionality but have no merge. But merge process will allow a lot of additional things for proper develop experience.

You are right, somehow with third party solutions I am able to inject in some way symbols. But I want to use NuGet. I can use local folder as a registry, but I can't use well defined snupkg format to force application debugging with visual studio. It is required to host symbol server as well not just a simple folder with downloaded snupkg.

BlackGad commented 3 years ago

Any thoughts about above flow @zivkan?

Look, I know you all have user stories from your PMs and this feature is complex. Moreover it comes from community. I don't ask you to develop it. I'm only asking you to have a discussion about the concept itself. I do understand this this will take a lot of your time and effort but believe that this functionality is crucial for NuGet's future.

Initial text describes all cases and they involve teams beside NuGet. But there is one case which can bring core implementation into NuGet without impacting other teams. Later, other teams will be able to use NuGet API for their work if necessary.

Please, let me know how you feel about it.

zivkan commented 3 years ago

I'm super busy with multiple urgent issues, so I haven't had time to look into this in detail. My overview is that this proposal is basically going to change NuGet to download and extract snupks in the global packages folder & solution packages folder on restore, and then hope the rest of the build tools make debugging work nice.

This is "easy" enough for local package sources, since we own the code that checks the files on disk. HTTP sources is going to be much more difficult because it's going to need a protocol change (and eventually update our protocol docs). While we (client team) can work with the nuget.org server team to get these changes implemented for nuget.org, it's going to up to Azure Artifacts, GitHub Package Repository, myget, Artifactory, and all nuget protocol server implementations to discover that the protocol has been extended and implement it, if they wish. If any of those servers don't implement the new "snupkg download" resource, then developers won't get an improved debugging experience, but they'll be no worse off than they are now.

Also, this is likely to only benefit packages.config customers. PackageReference will probably need #5926 fixed.

A risk is any tool that expects the packages folder to contain the same contents as the nupkg, plus the two generated files (minus the generated OPC files in the nupkg that aren't extracted). There's no official tooling that does this, so it probably won't affect most customers, but anyone who does have custom tooling to detect tampered package directories will be unpleasantly surprised. An alternative would be to extract symbols in a different directory (for example, ~/.nuget/symbols, rather than ~/.nuget/packages). That would still require #5926 to be implemented to support PackageReference, but might make supporting packages.config much more difficult.

This is all the time I can spend on this issue today.

zkat commented 3 years ago

@JonDouglas this looks like something we should look at from a customer development perspective, and figure out what actions to take from there. Do you mind taking a look? Andy and Nikolche can help with context/clarification. :)

BlackGad commented 3 years ago

@zivkan thank you! I really appreciate your effort to this despite being busy!

As an intermediate conclusion:

New functionality must be transparent and independent from generic NuGet restoration process. So, any actions must be turned on manually with appropriate commands.
Symbols cache must be stored in separated folder
Actual merge operation must be performed only for solution packages folder
For now, only limited functionality can be implemented to avoid cross team interaction

Theoretically, if I publish PR which covers above conclusion will you able to review and accept it?

PR will include:

-Symbols flag to nuget.exe install command to activate functionality
snupkg cache into ~/.nuget/symbols
snupkg download from local registries of different format (v2/v3 packed/unpacked)
snupkg restore into target solution packages folder
Bunch of tests related to symbols restore process

Or this complex functionality must be done in another way?

zivkan commented 3 years ago

An internal email thread made me realize that snupkgs currently do not get signed. If NuGet is going to start reading these files (currently it's write-only), then we need to consider whether we need to make snupkgs signed as well to mitigate the same risks that signed nupkgs mitigate. I really, really don't want to add scope creep, but security is security.

BlackGad commented 3 years ago

From my point of view NuGet container (for generic packages and for symbols) must be identical (security implementation as well). Basically snupkg is the same nupkg but with limited content and limited metadata. Even more - existing sign mechanism is working with snupkg (just checked).

Download, extract and install implementations will be identical to generic nupkg.

BlackGad commented 3 years ago

An internal email thread made me realize that snupkgs currently do not get signed. If NuGet is going to start reading these files (currently it's write-only), then we need to consider whether we need to make snupkgs signed as well to mitigate the same risks that signed nupkgs mitigate. I really, really don't want to add scope creep, but security is security.

My short opinion: It is not theme for consideration. snupkg MUST be safe

zivkan commented 3 years ago

Ok, hopefully this just means that the internal team's CI pipeline has a bug where they're not signing their snupkgs.

BlackGad commented 3 years ago

Ok, hopefully this just means that the internal team's CI pipeline has a bug where they're not signing their snupkgs.

Currently snupkgs are common zip archive with pdb inside. There are no suggestions how to work with them. All NuGet tools have no additional functionality to work with so nobody signing snupkg because it useless (for proper work you need to download it, unpack content and manually add symbols to output folder). One place that can validate signature is NuGet.org (hope they do this) before Symbols server populate.

@zivkan what about PR?

nkolev92 commented 3 years ago

Theoretically, if I publish PR which covers above conclusion will you able to review and accept it?

The burden here is not technical, but UX.

We appreciate your willingness to do the work here, but I'd encourage you to wait for a confirmation from us before spending time on the implementation.

BlackGad commented 3 years ago

@nkolev92 what about UX that I described above?

BlackGad commented 3 years ago

@nkolev92 can you comment above question?

nkolev92 commented 3 years ago

We are not ready to make a call at this point. Sorry about that.

Someone from the team will loop back when ready.

JonDouglas commented 3 years ago

Hey @BlackGad,

Firstly, thank you so much for the extremely thought out scenario & interest in contributing to NuGet! We are trying to move our proposal process to a more unified & streamlined model so we can take ample time to review as a community & team to decide whether we can support the proposal.

If you're willing to condense this issue into a proposal, we can dedicate some time to review it and amplify it within the .NET community for others to comment on.

https://github.com/NuGet/Home/tree/dev/meta#how-do-i-create-a-proposal

BlackGad commented 3 years ago

@JonDouglas here you are https://github.com/NuGet/Home/pull/10899 . Hope I've created it in proper manner. Let me know if something missed or described badly.

JonDouglas commented 3 years ago

@BlackGad Thank you kindly. I'll comment over there now!

LowCarbonGuy commented 3 years ago

I'd like to tag onto this issue for a related problem.

I'd like the "dotnet publish" command to be able to include the debug symbols of the project's dependent NuGet packages, so that the .NET exception callstack would include the line number. Here's my scenario:

1) Team A publishes a NuGet package (LibraryA) including debug symbol to a NuGet registry (it is internal to the company, but it probably doesn't matter). 2) Team B, who builds ServiceB, consumes LibraryA as a NuGet package by adding it as a "PackageReference" element in ServiceB.csproj. 3) Member of Team B builds a Docker image for ServiceB, which involves packaging ServiceB into a directory using the command "dotnet publish -o publish-output ServiceB.csproj".

Wish: LibraryA.pdb would get copied to the publish-output directory (because LibraryA.pdb is in the NuGet package and is also available in the local machine's NuGet cache). Actual: LibraryA.pdb does not get copied to the publish-output directory.

The reason I'd like to bring this up is that it seems like "dotnet publish" does not automatically copy all the files available in the build output directory. I've tried the following steps:

1) Run: dotnet build -c Release ServiceB.csproj 2) Manually copy LibraryA.pdb to where LibraryA.dll is in ServiceB's build output directory (e.g. bin\Release\net5.0). 3) Run: dotnet publish --no-build -c Release -o publish-output ServiceB.csproj

Wish: LibraryA.pdb would be copied to publish-output, given that it is in ServiceB's build output directory. Actual: LibraryA.pdb does not get copied to the publish-output directory.

Thanks for your consideration!

BlackGad commented 3 years ago

@LowCarbonGuy

If team A pack pdb into generic nupkg then 2 cases

For old project structure (dependencies inside looks like below example) msbuild will automatically copy Library.pdb and Library.xml files to output folder


<ItemGroup>
<Reference Include="Library, Version=0.0.0.42, Culture=neutral, PublicKeyToken=aaaaaaaaaaaaaaa, processorArchitecture=MSIL">
    <HintPath>..\..\packages\Library.0.0.0.42\lib\net45\Library.dll</HintPath>
</Reference>
</ItemGroup>

2. For new project structure (dependencies inside looks like below example) satellites files will not be copied do a [issue](https://github.com/NuGet/Home/issues/5926)
```xml
<ItemGroup>
    <PackageReference Include="Library" Version="0.0.0.42" />
</ItemGroup>

If team A pack pdb into snupkg package then 2 cases for team B
1. For old project structure download and extract snupkg to packages directory manually (see 1.1 above).
2. For new project structure download and extract snupkg directly to output folder (but choose right platform)
3. If snupkg was downloaded to official nuget.org feed you can use built in symbols debug server

But seems you are missed snupkg feature at all :) Current issue and this PR are all about this.

Read explanation. Is it looks like described scenario?

LowCarbonGuy commented 3 years ago

@BlackGad Thanks for your detailed explanation! Truly appreciate it!

My scenario is 1. ii. (team A pack pdb into generic nupkg, team B uses new project structure) because:

The teams use GitLab's NuGet registry, which does not yet support snupkg. Fortunately, the GitLab team seems to be actively working on supporting snupkg
Team B uses .Net 5.0, and team members are used to creating and editing the new project structure with PackageReference .

For now, team B's been working around the problem by copying the relevant .pdb from the NuGet cache directory to the publish-output directory (the output dir of dotnet publish ServiceB.csproj), before building the Docker image.

Glad to hear that the long-term solution will be implemented for snupkg!

BlackGad commented 3 years ago

@LowCarbonGuy BTW for your scenario it is possible to use lifehack

<Target Name="Copy assemblies satellite symbols" AfterTargets="ResolveAssemblyReferences">
    <ItemGroup>
      <SymbolsSatelliteFiles Include="@(ReferenceCopyLocalPaths->'%(RootDir)%(Directory)%(Filename).pdb')" Condition="$([System.IO.File]::Exists('%(RootDir)%(Directory)%(Filename).pdb'))"/>
      <ReferenceCopyLocalPaths Include="@(SymbolsSatelliteFiles)"/>
    </ItemGroup>
  </Target>

Paste this to your *.csproj file ;)

NuGet / Home

Full development cycle with Symbols #10793

Details about Problem

Current implementation state

Create and publish

Restore and build

Launch and debug

Issues

Purposed implementation

Create and publish

Restore and build

Launch and debug

Purpose

Existing covered issues

Discovered problems

V2 project

Cases:

V3 project

Cases:

What to add

What it will do

Debug flow example