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

Publish Single-file: Smart settings for IncludeSymbolsInSingleFile #3685

Closed
swaroop-sridhar opened this issue Sep 27, 2019 · 12 comments
Closed
Milestone

Comments

@swaroop-sridhar
Copy link
Contributor

When publishing an app as a single file, the default setting is to not include PDB files in the output bundle by default (and instead leave them as separate files).

However, this setting is unsuitable for some apps, where the deps.json file includes a dependency on the PDB file. This causes runtime-crash of the app when the apphost validates the contents against the deps.json manifest. For example:
PowerShell/PowerShell#10266
https://github.com/Viir/bots/tree/adapt-for-single-file-exe-publish

The inclusion of PDB files as a dependency is almost always an error, but an app (and its dependencies/nuget packages) are not required to associate the same semantics with file-extensions.

So, if the deps.json writer finds a PDB file in the manifest, the SDK should default the IncludeSymbolsInSingleFile setting to true. The SDK should also issue a warning about this change, so that PDBs are not silently included -- unintentionally increasing the size of published apps.

@swaroop-sridhar
Copy link
Contributor Author

CC: @Viir

@swaroop-sridhar
Copy link
Contributor Author

CC: @dasMulli

@swaroop-sridhar swaroop-sridhar changed the title Publish Single-file: Smat settings for IncludeSymbolsInSingleFile Publish Single-file: Smart settings for IncludeSymbolsInSingleFile Sep 27, 2019
@swaroop-sridhar
Copy link
Contributor Author

@lpereira In .net 5, will the apphost validate the dependencies listed in the deps.json file at startup?
This PDB inclusion issue will be moot for most apps, but for the apphost checking.

CC: @vitek-karas

@dasMulli
Copy link
Contributor

I'm still not sure if the pdb in the deps.json isn't a bug that should be fixed in the first place. Though this likely needs Nuget involvement.. @nguerrera @dsplaisted thoughts?

@nguerrera
Copy link
Contributor

I think it is a bug. We have related discussion about being able to query for pds so that we can copy them. It's ironic that we copy them for native, but not managed. Feels like there's an exclusion of pdbs only in managed case.

@dsplaisted
Copy link
Member

I believe that technically it's a bug to put a pdb in the native files of a package. NuGet interprets anything in the folder as a native file, which is how it flows through to the deps.json.

It seems like it is natural to put pdbs in the native folder though, so if we want to support that we could update the NuGet convention, or apply a PDB convention on top of it in the SDK.

@dasMulli
Copy link
Contributor

I think it is perfectly natural for authors to include artefacts that help for debugging or error tracking.

For managed code, I usually suggest deploying PDBs with everything to get "good" stack traces. Tools like Sentry even help tracking down the GIT commit that caused error to occur based on stack trace symbolificatiton.
So authors may want to include pdbs / dsym / etc.

But "native" really means native assets so that may also mean runtime-specific scripts and assets..
I don't think there should be much introspection of the file type going on here. PDB is one special case for windows but I don't think it is good to make assumptions on files in native folders.

@dasMulli
Copy link
Contributor

To double down: Does IncludeSymbolsInSingleFile also apply to dsym files?
I think there should be parity for each runtime if that is the case, so leaving native pdb / dsym / * files alone and not applying filters to that or encoding the knowledge about all kinds of symbol files.

@nguerrera
Copy link
Contributor

nguerrera commented Sep 27, 2019

Great points. At the end of the day, we have to uniformly decide if something is a symbol file or not. If we decide it is not a symbol file, but an arbitrary native asset, then it should be in deps.json AND deployed irrespective of IncludeSymbolsInSingleFile. And if we decide it is a symbol file, then it should not be in deps.json AND included in single file if and only if IncludeSymbolsInSingleFile is true.

NuGet and the assets files should be the source of truth for what files in package are what kind of thing. If we decide to use extension heuristics for this, then they should go in nuget. SDK should respect what NuGet tells us.

I will combine this issue and the issue of not copying managed PDBs into one feature request for nuget.

@lpereira
Copy link

@lpereira In .net 5, will the apphost validate the dependencies listed in the deps.json file at startup?

I've been thinking about this, and I might open the deps in apphost so the validation is performed right at the beginning, but keep them open -- the runtime can then just use the open file descriptors instead. In a way, the check now becomes more robust while still improving the performance, by cutting a lot of unneeded stat(2) calls. (Need to sleep on this for a little while longer, though.)

@livarcocc livarcocc added this to the 5.0.1xx milestone Oct 7, 2019
SIkebe added a commit to SIkebe/gitbucket-utility that referenced this issue Jan 30, 2020
SIkebe added a commit to SIkebe/gitbucket-utility that referenced this issue Feb 1, 2020
SIkebe added a commit to SIkebe/gitbucket-utility that referenced this issue Feb 5, 2020
SIkebe added a commit to SIkebe/gitbucket-utility that referenced this issue Feb 10, 2020
SIkebe added a commit to SIkebe/gitbucket-utility that referenced this issue Feb 24, 2020
@marcpopMSFT marcpopMSFT added the untriaged Request triage from a team member label Apr 16, 2020
@dsplaisted
Copy link
Member

I believe this is related to #1458

@dsplaisted dsplaisted removed their assignment Apr 22, 2020
@dsplaisted dsplaisted removed the untriaged Request triage from a team member label Apr 22, 2020
Viir added a commit to pine-vm/pine that referenced this issue May 18, 2020
With the previous version, got this error when trying to run elm-fullstack.exe:
----
Error:
  An assembly specified in the application dependencies manifest (elm-fullstack.deps.json) was not found:
    package: 'LibGit2Sharp.NativeBinaries', version: '2.0.306'
    path: 'runtimes/win-x64/native/git2-106a5f2.pdb'

For context, see:

+ libgit2/libgit2sharp#1754 (comment)
+ dotnet/sdk#3685
rainersigwald pushed a commit that referenced this issue Jul 20, 2020
…build 20191122.1 (#3685)

- Microsoft.NET.Sdk.Razor - 3.0.2-servicing.19572.1
@marcpopMSFT marcpopMSFT modified the milestones: 5.0.1xx, 6.0.1xx Jun 21, 2021
Viir added a commit to pine-vm/pine that referenced this issue Aug 16, 2021
The single-file build functionality in .NET 5 did not work well with earlier versions of libgit2sharp. Let's see if our build works with this version and .NET 5 or .NET 6.
For discussion of the errors we got when trying .NET 5 with earlier versions of libgit2sharp, see libgit2/libgit2sharp#1857, libgit2/libgit2sharp.nativebinaries#111, dotnet/sdk#3685, and dotnet/runtime#36590
Viir added a commit to pine-vm/pine that referenced this issue Aug 16, 2021
The single-file build functionality in .NET 5 did not work well with earlier versions of libgit2sharp. Let's see if our build works with this version and .NET 5 or .NET 6.
For discussion of the errors we got when trying .NET 5 with earlier versions of libgit2sharp, see libgit2/libgit2sharp#1857, libgit2/libgit2sharp.nativebinaries#111,
libgit2/libgit2sharp.nativebinaries#119, dotnet/sdk#3685, and dotnet/runtime#36590
Viir added a commit to pine-vm/pine that referenced this issue Aug 16, 2021
The single-file build functionality in .NET 5 did not work well with earlier versions of libgit2sharp. Let's see if our build works with this version and .NET 5 or .NET 6.
For discussion of the errors we got when trying .NET 5 with earlier versions of libgit2sharp, see libgit2/libgit2sharp#1857, libgit2/libgit2sharp.nativebinaries#111,
libgit2/libgit2sharp.nativebinaries#119, dotnet/sdk#3685, and dotnet/runtime#36590
@marcpopMSFT marcpopMSFT modified the milestones: 6.0.1xx, 7.0.1xx Oct 27, 2021
@marcpopMSFT marcpopMSFT modified the milestones: 7.0.1xx, 8.0.1xx Nov 23, 2022
@marcpopMSFT
Copy link
Member

Marking completed as we now have a way of including the pdb in the nuget package. Reopen if that's not sufficient for this.

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

No branches or pull requests

7 participants