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

msbuild /t:Pack always creates seperate symbols package #4142

Closed
onovotny opened this issue Dec 21, 2016 · 48 comments
Closed

msbuild /t:Pack always creates seperate symbols package #4142

onovotny opened this issue Dec 21, 2016 · 48 comments

Comments

@onovotny
Copy link

@onovotny onovotny commented Dec 21, 2016

When using the SDK builds to create a package, if you have <IncludeSymbols> set to true, then NuGet always creates a separate .symbols.nupkg. This needs to be configurable. Sometimes we want to put our symbols in the main NuPkg as it makes it far easier to have source debugging "just work" without extra symbol server configuration.

Even if the current behavior is the default, there needs to be an option not to create a separate .symbols.nupkg and just leave the symbols in the main package.

@rohit21agrawal

This comment has been minimized.

Copy link
Contributor

@rohit21agrawal rohit21agrawal commented Dec 28, 2016

@rrelyea : this seems like a feature request, do we want it for rc3 ?

To the best of my knowledge , dotnet pack never had this ability

@rrelyea rrelyea modified the milestones: Future-1, 4.0 RC3 Dec 30, 2016
@rrelyea

This comment has been minimized.

Copy link
Contributor

@rrelyea rrelyea commented Dec 30, 2016

We'll consider this work in the future. Yes, we shouldn't take time before RTM to land this one way or the other.

@onovotny

This comment has been minimized.

Copy link
Author

@onovotny onovotny commented Dec 30, 2016

@rohit21agrawal

This comment has been minimized.

Copy link
Contributor

@rohit21agrawal rohit21agrawal commented Dec 30, 2016

@onovotny as a workaround , can't you just add a custom target that runs after pack, deletes the .nupkg package and renames the .symbols.nupkg package to .nupkg ? I can help you write the target if you think this will unblock you.

@onovotny

This comment has been minimized.

Copy link
Author

@onovotny onovotny commented Dec 30, 2016

@rohit21agrawal

This comment has been minimized.

Copy link
Contributor

@rohit21agrawal rohit21agrawal commented Dec 30, 2016

we'll have to design it appropriately, but thinking off the top of my head - we would have a msbuild property like $(CreateSeparateSymbolsPackage) which would default to true in Pack.targets . We will then use this property in MSBuildProjectFactory.cs (passed to it via PackTargetArgs) and write the logic to determine the extension of the symbols package and stop it from writing out the non-symbols package to disk.

One thing to consider here is that having too many properties/command-line switches is not always a good thing, especially for scenarios which are not blocking.

@onovotny

This comment has been minimized.

Copy link
Author

@onovotny onovotny commented Dec 30, 2016

I think this ties into #1696 and dotnet/roslyn#3. By default, especially with PortablePdb's being much smaller, symbol files should not always be going to their own packages by default.

I think there's a clear difference between the flag to include symbols in the package at all, IncludeSymbols and whether it should be in a separate file or not.

In addition, FWIW @yishaigalatzer said here #1696 (comment) that including symbols would be on by default in the new Pack targets. I didn't take that to mean it'd be a separate file as that's mostly useless with today's tooling. Having it in the main NuPkg is useful because then the debugger will easily find and load it.

@rohit21agrawal

This comment has been minimized.

Copy link
Contributor

@rohit21agrawal rohit21agrawal commented Dec 30, 2016

@yishaigalatzer, @rrelyea I wasn't aware of the plan to include symbols by default in the nupkg . Did i miss anything?

@ctaggart

This comment has been minimized.

Copy link

@ctaggart ctaggart commented Feb 26, 2017

@rohit21agrawal, yes, that plan that I hashed out with @tmat & @yishaigalatzer at the MS MVP conference in 2015-11 is finally coming together. I added some comments in #1696 (comment), but to expand on that, here is what Visual Studio 2017 now offers:

image

source server support

This was done with source indexing large Windows PDB files. There was valid concern for including them in nupkg files, especially when the nuget client didn't share them. Source indexing was done by modifying the pdb file after the compile.

source link support

The new source link support is done with Portable PDB files which are cross platform and many times more compact. The source link json is built before the the compile and the compiler embeds it in the pdb.

Essentially, things have changed. Including Portable PDB files in order to get source link support is important and doable now.

@ctaggart

This comment has been minimized.

Copy link

@ctaggart ctaggart commented Feb 26, 2017

I think the new dotnet pack option should be --include-portable-pdbs to clearly indicate that this is for Portable PDB files and the new source link support.

@ctaggart

This comment has been minimized.

Copy link

@ctaggart ctaggart commented Mar 3, 2017

@onovotny The solution that we are going forward with to enable source link is to embed the portable pdb files in the .NET Core dll files. That achieves getting them in the nupkg without people having to change anything else about their build process. https://github.com/ctaggart/SourceLink#dotnet-sourcelink

@onovotny

This comment has been minimized.

Copy link
Author

@onovotny onovotny commented Mar 3, 2017

@ctaggart that's a really, really, really bad idea....sorry. It's bad for mobile devices where the physical image size and memory is tight. We don't want pdb's to be loaded into memory because they're embedded in the dll. This is a complete non-starter.

It's not just mobile, but other constrained devices like IoT and embedded (think Tizen).

@ctaggart

This comment has been minimized.

Copy link

@ctaggart ctaggart commented Mar 3, 2017

Ugh. @tmat help

@tmat

This comment has been minimized.

Copy link

@tmat tmat commented Mar 3, 2017

@onovotny It's not a bad idea. It might not be feasible for all libraries (e.g. as you mention those potentially deployed on small devices), but many customers are fine with 10-20% dll size increase. Note that the PDB data pages are actually not committed into memory until the PDB data is accessed.

We are currently also figuring out whether the debugger could fetch Portable PDBs from packages published on NuGet.org. Stay tuned.

@tmat

This comment has been minimized.

Copy link

@tmat tmat commented Mar 3, 2017

In any case I'd recommend either embed PDBs into dll/exe, if size is not an issue, or include it in a separate file in the nuget package next to the dll/exe.

@onovotny

This comment has been minimized.

Copy link
Author

@onovotny onovotny commented Mar 3, 2017

@tmat my concern, and why I called it out as "bad", is that the creators of the libraries are often not the end-users and are not likely going to be thinking about where their libraries may be used.

So people will think they're doing the right thing, being helpful, providing symbols, etc, but then adversely affecting their users by generating larger libraries.

We are currently also figuring out whether the debugger could fetch Portable PDBs from packages published on NuGet.org. Stay tuned.

That would be a much nicer idea, IMO.

In any case I'd recommend either embed PDBs into dll/exe, if size is not an issue, or include it in a separate file in the nuget package next to the dll/exe.

Key issue is that as mentioned, the producer of the library cannot know if size is an issue for the end user. And for the end user, the choice would have been prematurely made for them. I'd venture to say that in this case, size of dll relates to perf (loading, memory, size) and thus should strive to be as small as possible under all circumstances.

@tmat

This comment has been minimized.

Copy link

@tmat tmat commented Mar 3, 2017

@onovotny If you find such a library that you absolutely need to be smaller you could let the producer know to change the settings and publish PDB separately. In case the author doesn't respond we can provide a tool that removes the PDB data from the dll. And you could then publish such modified library + pdb in your own feed. The dll would still link to the PDB so the debugger would be able to find it.

I'm still not convinced that 10-20% extra size on disk is a deal breaker. The load time should not be impacted since the loader creates a memory mapping for the file. It doesn't read each byte of the dll.

@tmat

This comment has been minimized.

Copy link

@tmat tmat commented May 2, 2017

How about changing the default behavior of <IncludeSymbols> to be:

  • if building Portable PDBs include them into .nupkg, do not create .symbols.nupkg
  • if building Windows PDBs create .symbols.nupkg, do not include them in the main nupkg

In other words symbol packages are for legacy PDBs. Portable PDBs are small and go directly to the main package where nuget server can index them.

Also, the value of <IncludeSymbols> could be true by default for Portable PDBs.

@ctaggart

This comment has been minimized.

Copy link

@ctaggart ctaggart commented May 3, 2017

Here is a working example of adding the pdb using the current tooling that I did today for fsprojects/Paket#2313:

  <ItemGroup Label="dotnet pack instructions">
    <Content Include="$(OutputPath)Paket.Core.pdb">
      <Pack>true</Pack>
      <PackagePath>lib/netstandard1.6</PackagePath>
    </Content>
  </ItemGroup>

This is based on the dotnet extensibility article. This works, but it should be easier.

@kzu

This comment has been minimized.

Copy link

@kzu kzu commented Aug 23, 2017

Yes please. That would make it compatible with NuGetizer property without "compat mode" required: https://github.com/NuGet/NuGet.Build.Packaging/blob/dev/src/Build/NuGet.Build.Packaging.Tasks/NuGet.Build.Packaging.Inference.targets#L30

@tmat

This comment has been minimized.

Copy link

@tmat tmat commented Aug 23, 2017

@kzu Any idea why there is '$(Configuration)' == 'Debug' on the line you pointed out?

@kzu

This comment has been minimized.

Copy link

@kzu kzu commented Sep 11, 2017

Because I thought it was more intuitive to enable it by default only for debug builds? I guess I should probably check for DebugType instead? Or maybe not even that and just rely on the DebugSymbolsProjectOutputGroupOutput having something (or not)?

@kzu

This comment has been minimized.

Copy link

@kzu kzu commented Sep 11, 2017

Makes sense to completely remove that from the condition. PR sent: NuGet/NuGet.Build.Packaging#147

@jnm2

This comment has been minimized.

Copy link

@jnm2 jnm2 commented Nov 4, 2017

In the meantime this is my workaround:

<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>
@thomaslevesque

This comment has been minimized.

Copy link

@thomaslevesque thomaslevesque commented Nov 5, 2017

@jnm2 nice, thanks for the tip!

@ladeak

This comment has been minimized.

Copy link

@ladeak ladeak commented Nov 16, 2017

@jnm2
Your suggestion works perfectly in VS 2017, but when I deploy to VSTS, it does not seem to have any effects.
<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>

Do you know what the reason could be for this?

@jnm2

This comment has been minimized.

Copy link

@jnm2 jnm2 commented Nov 16, 2017

@ladeak I don't know, sorry. To diagnose you'd have to get the VSTS deploy task to pass CLI arguments to MSBuild which make it give you back a log file. See http://msbuildlog.com/.

@bording

This comment has been minimized.

Copy link

@bording bording commented Nov 16, 2017

@ladeak AllowedOutputExtensionsInPackageBuildOutputFolder requires VS 2017 15.4 / .NET Core 2.0.2 SDK to work. Perhaps VSTS hasn't updated yet?

@ladeak

This comment has been minimized.

Copy link

@ladeak ladeak commented Nov 17, 2017

It say .Net Core 2.* (preview), log says Version 2.1.8 for dotnet pack.
The build step uses VS 2017 with MSBuild version 1.125.0. It seems to generate the pdb, just not included to the package.

@bording

This comment has been minimized.

Copy link

@bording bording commented Nov 17, 2017

The build step uses VS 2017 with MSBuild version 1.125.0. It seems to generate the pdb, just not included to the package.

That seems to be pretty good indication that it's not using the version of the Pack targets included with the 2.0.2 SDK then.

If you want to get it working without waiting for the 2.0.2 SDK to be available, you can use this workaround instead:

<PropertyGroup>
  <TargetsForTfmSpecificContentInPackage>$(TargetsForTfmSpecificContentInPackage);IncludePDBsInPackage</TargetsForTfmSpecificContentInPackage>
</PropertyGroup>
<Target Name="IncludePDBsInPackage" Condition="'$(IncludeBuildOutput)' != 'false'">
  <ItemGroup>
    <TfmSpecificPackageFile Include="$(OutputPath)\$(AssemblyName).pdb" PackagePath="lib\$(TargetFramework)" />
  </ItemGroup>
</Target>
@KellyR-STCU

This comment has been minimized.

Copy link

@KellyR-STCU KellyR-STCU commented Dec 7, 2017

What is the currently best solution for this? 2.0.3

@rohit21agrawal

This comment has been minimized.

Copy link
Contributor

@rohit21agrawal rohit21agrawal commented Dec 7, 2017

@KellyR-STCU
use : <AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>

@ctaggart

This comment has been minimized.

Copy link

@ctaggart ctaggart commented Jan 1, 2018

As of SourceLink 2.7, it sets that property so that pdb files are included by default. I consider this issues resolved by AllowedOutputExtensionsInPackageBuildOutputFolder.

@jnm2

This comment has been minimized.

Copy link

@jnm2 jnm2 commented Jan 16, 2018

Couldn't we have a nicer bool property like @davidfowl suggested?

This is a lot to type/paste and it shows its implementation details.

<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>

Also, it's missing .mdb. AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder takes AllowedOutputExtensionsInPackageBuildOutputFolder and adds .pdb and .mdb.

What we really need is for the target to do this after it sets the defaults:

<AllowedOutputExtensionsInPackageBuildOutputFolder Condition="'$(IncludeSymbolsInPackage)' == 'true'">$(AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder)</AllowedOutputExtensionsInPackageBuildOutputFolder>
@jnm2

This comment has been minimized.

Copy link

@jnm2 jnm2 commented Jan 23, 2018

Since this issue was closed without action, I moved my proposal to NuGet/NuGet.Client#1971.

@txwizard

This comment has been minimized.

Copy link

@txwizard txwizard commented May 2, 2019

$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb

Where in the .csproj file does the above go?

@eltjo-k

This comment has been minimized.

Copy link

@eltjo-k eltjo-k commented May 2, 2019

$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb

Where in the .csproj file does the above go?

Example of a complete project file:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netstandard1.1</TargetFramework>
    <VersionPrefix>1.0.2</VersionPrefix>
    <Description>Example</Description>
    <LangVersion>latest</LangVersion>
    <AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="System.ComponentModel.Annotations" Version="4.5.0" />
  </ItemGroup>
</Project>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.