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

PDB file should never be a required dependency to run app #111

Closed
CyberSinh opened this issue Nov 22, 2020 · 6 comments
Closed

PDB file should never be a required dependency to run app #111

CyberSinh opened this issue Nov 22, 2020 · 6 comments

Comments

@CyberSinh
Copy link

CyberSinh commented Nov 22, 2020

I don't think it's a good idea 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.
So, the optional PDB file became a mandatory dependency for the .NET 5.0, and the app will crash without it.

Description: A .NET application failed.
Message: Error:
  An assembly specified in the application dependencies manifest (XXX.deps.json) was not found:
    package: 'LibGit2Sharp.NativeBinaries', version: '2.0.306'
    path: 'runtimes/win-x64/native/git2-106a5f2.pdb'

There are already discussions related to this issue :
dotnet/runtime#3807
dotnet/sdk#3685

Why don't you put symbols in a dedicated symbol packages (.snupkg)?

@CyberSinh CyberSinh changed the title Is there a way to prevent the PDB file from being copied to the output folder? PDB should not be marked as a required dependency in the package? Dec 6, 2020
@CyberSinh CyberSinh changed the title PDB should not be marked as a required dependency in the package? PDB file should never be a required dependency to run app Dec 6, 2020
@rcdailey
Copy link

Even in the latest prerelease builds this is still an issue. I really feel like this should get a higher priority from the developers.

rcdailey added a commit to recyclarr/recyclarr that referenced this issue Jul 30, 2021
This is only required because LibGit2Sharp bundles PDB files in its
nuget package. See the following github issues for more info.

- dotnet/runtime#3807
- libgit2/libgit2sharp.nativebinaries#111
rcdailey added a commit to rcdailey/libgit2sharp.nativebinaries that referenced this issue Jul 31, 2021
Nuget supports two types of symbol packages: [legacy][1] and
[snuget][2]. The latter happens to be the newer implementation. Legacy
symbol packages only exist for compatibility.

It should be noted that, unfortunately, snuget symbol packages do not
support Windows PDBs (the kind you get from C++ code). They only support
portable PDBs (managed). As such, this patch provides support for legacy
symbol package format. Note the following information from the symbol
package MSDN documentation page:

> Native projects, such as C++ projects, produce Windows PDBs instead of
> Portable PDBs. These are not supported by NuGet.org's symbol server.
> Please use Legacy Symbol Packages instead.

Fixes libgit2#111

[1]: https://docs.microsoft.com/en-us/nuget/create-packages/symbol-packages
[2]: https://docs.microsoft.com/en-us/nuget/create-packages/symbol-packages-snupkg
rcdailey added a commit to recyclarr/recyclarr that referenced this issue Jul 31, 2021
This is only required because LibGit2Sharp bundles PDB files in its
nuget package. See the following github issues for more info.

- dotnet/runtime#3807
- libgit2/libgit2sharp.nativebinaries#111
rcdailey added a commit to rcdailey/libgit2sharp.nativebinaries that referenced this issue Jul 31, 2021
Nuget supports two types of symbol packages: [legacy][1] and
[snuget][2]. The latter happens to be the newer implementation. Legacy
symbol packages only exist for compatibility.

It should be noted that, unfortunately, snuget symbol packages do not
support Windows PDBs (the kind you get from C++ code). They only support
portable PDBs (managed). As such, this patch provides support for legacy
symbol package format. Note the following information from the symbol
package MSDN documentation page:

> Native projects, such as C++ projects, produce Windows PDBs instead of
> Portable PDBs. These are not supported by NuGet.org's symbol server.
> Please use Legacy Symbol Packages instead.

Fixes libgit2#111

[1]: https://docs.microsoft.com/en-us/nuget/create-packages/symbol-packages
[2]: https://docs.microsoft.com/en-us/nuget/create-packages/symbol-packages-snupkg
@bording
Copy link
Member

bording commented Aug 10, 2021

I'm not immediately inclined to make changes here. Having the native symbols available aids in debugging scenarios. I also am not a fan of the various symbol packages formats that Microsoft has produced over the years. They make getting the symbols far more complicated than I care for, when it's quite simple to just include the symbols directly in the packages. For managed symbols I've even started preferring the embedded option to just include the symbol information right in the assembly.

There is definitely value in shipping managed assemblies as part of your publish output. Without them, exceptions thrown do not have line numbers in the stack traces. The value proposition of native symbols in production might not be as clear, but the tradeoff of easily having access to them when debugging vs. having to worry about configuring a symbol server locations, etc. makes me think the tradeoff is worth it.

It is unfortunate that NuGet currently puts the native PDBs in the deps.json file, but as mentioned in the linked issues, until they fix that, there is a workaround: setting <IncludeSymbolsInSingleFile>true</IncludeSymbolsInSingleFile> true in your project file.

@rcdailey
Copy link

I'm not immediately inclined to make changes here.

I have done the work for you. Please see PR #119

Having the native symbols available aids in debugging scenarios.

Moving symbols to a symbol package does not mean you don't get symbols. In other words, you still get symbols they are just delivered in a way that does not interfere with dotnet publish.

I also am not a fan of the various symbol packages formats that Microsoft has produced over the years. They make getting the symbols far more complicated than I care for

I definitely understand your frustration, but if we expect to utilize Microsoft's package management system we unfortunately must follow their rules. As much as I'd love to ignore their rules, when you do so, you end up with bug reports like these.

when it's quite simple to just include the symbols directly in the packages.

As you can unfortunately see, even though it's simple in terms of build pipeline implementation, you end up with this bug report when you package the symbols in your main package. So in my opinion, what matters here is not what is simpler but what is correct as defined by Microsoft. I get that you don't like the way MS is doing this, but I think that since 1) I've done the work for you and 2) this issue is observed by so many people, I think it would be fair and reasonable to put aside personal feelings about this in favor of a correct solution. I mean that with the most respect.

There is definitely value in shipping managed assemblies as part of your publish output. Without them, exceptions thrown do not have line numbers in the stack traces. The value proposition of native symbols in production might not be as clear, but the tradeoff of easily having access to them when debugging vs. having to worry about configuring a symbol server locations, etc. makes me think the tradeoff is worth it.

I'm not sure what you're saying here. My understanding is that the native debug symbols (libgit2.pdb) is the root cause of the issue here. I have not seen any strictly managed symbols relating directly to this bug. If I'm misunderstanding, I apologize. Perhaps some clarification would help.

It is unfortunate that NuGet currently puts the native PDBs in the deps.json file, but as mentioned in the linked issues, until they fix that, there is a workaround: setting <IncludeSymbolsInSingleFile>true</IncludeSymbolsInSingleFile> true in your project file.

Sadly this solution won't work in some cases. For example if I set PublishSingleFile to true, IncludeSymbolsInSingleFile to true, and I'm using .NET 5, you get an error:

"Including symbols in a single file bundle is not supported when publishing for .NET5 or higher"

See here for more information (I haven't found official docs on this, but you can see it in the dotnet/sdk source). The only solution that works in all cases is my proposed solution in #119.

I hope that you will reconsider. Again I am empathetic to your feelings about the legacy symbol package and I share your frustration. But at the end of the day, I need your package to work.

@bording
Copy link
Member

bording commented Aug 12, 2021

I have done the work for you. Please see PR #119

That's not really relevant if the PR is taking the project in a direction that the maintainers aren't interested in going in. In fact, it's usually a good idea on an open source project to open an issue and discuss before just going ahead and spending time on a PR that might not be accepted.

I know that there hasn't been a ton of response here lately, but that's a side effect of me basically being the only maintainer at this point, and I just haven't had the free time to devote to this lately.

I think it would be fair and reasonable to put aside personal feelings about this in favor of a correct solution.

There's nothing actually wrong with shipping symbols in a NuGet package, and you can see in many places that even Microsoft developers aren't in alignment around the need for symbol packages. On the managed side of things, they've gone so far as to support embedding the PDBs directly into the assemblies, which is about as far away as you can get from shipping symbols in separate packages! It's not about personal feelings but an evaluation of the trade-offs involved and deciding to avoid symbol packages in light of that evaluation? Have you ever actually tried to use symbol packages before, especially older format? It leads to an an error-prone, inferior debugging experience.

The fact that the native PDB in the package causes problems with single file deployment is definitely a bug on Microsoft's part, and you can see that being acknowledged in the link issues. The last time I investigated this, there was indeed a valid workaround (IncludeSymbolsInSingleFile) so I was not inclined to make changes since there was a way to make things work while also keeping the debug symbols easily accessible.

Sadly this solution won't work in some cases. For example if I set PublishSingleFile to true, IncludeSymbolsInSingleFile to true, and I'm using .NET 5, you get an error:

The fact that Microsoft removed the workaround in .NET 5 while also not fixing the bug is not something I was previously aware of, and there was no mention of it in the linked issues that I reviewed again before commenting on this issue, so thank you for bringing that to my attention. I have verified that the only way to make it work would be to ship the git.dll next to the exe. I don't think that is particularly problematic, but I can see the appeal of it indeed being a single file (assuming you use IncludeNativeLibrariesForSelfExtract).

But at the end of the day, I need your package to work.

It does work, it just doesn't work in the exact way you want it to, with the exact set of features you want to use it with. The thing you need to keep in mind is that for many open source projects (including this one) the people working on them are volunteering their spare time and aren't being compensated for them. That means there is no obligation for those people to make changes just because you want them!


That being said, I have just done some needed maintenance here to get the CI systems working again, moving over to GitHub Actions in the process. While doing that work, I made some changes to how the package is built, and I decided that rather than try to incorporate separate symbol packages, I instead just stopped bundling the PDBs in the package.

That means I should be able to publish a new version of the native binaries package soon, and then shortly after I can have a new LibGit2Sharp preview package out that uses it.

@rcdailey
Copy link

I have done the work for you. Please see PR #119

That's not really relevant if the PR is taking the project in a direction that the maintainers aren't interested in going in. In fact, it's usually a good idea on an open source project to open an issue and discuss before just going ahead and spending time on a PR that might not be accepted.

With respect, I don't think it's fair to complain about PR etiquette when we have an open issue that hasn't gotten any developer attention for nearly a year. So in fact what we have here are multiple issues about this topic, with very little activity, followed by a pull request. So it does seem like I've followed things the way you prefer them.

Also I don't mind spending time on providing a change for you in a pull request:

  1. I fully acknowledge there is a risk my change is not satisfactory, perhaps not even acceptable at all.
  2. If I have an opportunity to make your life easier (for the very reasons you mentioned), I will gladly help.
  3. This is an issue I am affected by and passionate about, so naturally I want to help.
  4. Many times a PR is itself an open discussion and where a PR starts looks nothing like where it ends. What's important, in my view, is that a PR serves as a mechanism to engage with the repository maintainers, show some involvement and level of effort to help, and work with them towards an ideal (or compromise) solution.

While you and I may disagree on our philosophies about pull requests, I think at the end of the day we're both just trying to help out.

Lastly, I want to say that I fully recognize you are spending your free time on project and I respect that. I work with many different open source developers, some more accommodating than others. Some have more free time to spend than others. There's a lot of variation between projects. Also don't forget that I'm just like you. I'm an open source developer devoting my free time to you (and others). But I don't bring that up, I don't expect people to treat me differently because of that. Honestly this is a hobby for me, so I enjoy it.

From my standpoint, I'm already aware of and am respectful of the fact that you are likely not paid to work on this. The fact that I proactively opened a pull request is a reflection of that awareness. I see many developers bring up how they aren't paid, or spend their free time. I personally find those statements not very constructive, especially to the overall progress and culture of a FOSS community. This isn't a complaint specifically targeting you, it's more of a general trend I see that I personally dislike and I feel like we would all be better off if we each took this point for granted and leveled with each other.

Again I mean this in the most respectful way possible. I was a little put off by the way you handled your response to this issue and so I felt it appropriate to share my thoughts. But I'm not upset and I fully respect your decision on this issue.

The fact that Microsoft removed the workaround in .NET 5 while also not fixing the bug is not something I was previously aware of, and there was no mention of it in the linked issues that I reviewed again before commenting on this issue, so thank you for bringing that to my attention. I have verified that the only way to make it work would be to ship the git.dll next to the exe. I don't think that is particularly problematic, but I can see the appeal of it indeed being a single file (assuming you use IncludeNativeLibrariesForSelfExtract).

I do indeed use that option. For completeness, here is my entire publish command:

dotnet publish src\Trash `
    --output publish\$runtime `
    --configuration Release `
    --runtime $runtime `
    --self-contained true `
    -p:PublishSingleFile=true `
    -p:PublishTrimmed=true `
    -p:IncludeNativeLibrariesForSelfExtract=true `
    -p:PublishReadyToRun=true `
    -p:PublishReadyToRunShowWarnings=true

For my command line application, what I am for is a truly single file distributable. However, at the moment there's a PDB file for libgit2 sitting next to my executable which is a symptom of the issue reported here.

It does work, it just doesn't work in the exact way you want it to, with the exact set of features you want to use it with.

Worth clarifying a few things here. First, when I said it needs to work, what I'm really saying is that I need this specific issue to be resolved. Of course libgit2sharp itself works just fine. Also I'm not sure why you are bringing up features, I don't recall having any issue or complaints about the features of libgit2sharp itself.

The thing you need to keep in mind is that for many open source projects (including this one) the people working on them are volunteering their spare time and aren't being compensated for them. That means there is no obligation for those people to make changes just because you want them!

I already discussed this earlier, but this is a very passive aggressive statement that doesn't add any value to the conversation. Again, I already fully respect your time and I do not feel like you are obligated to accept my change.

That being said, I have just done some needed maintenance here to get the CI systems working again, moving over to GitHub Actions in the process. While doing that work, I made some changes to how the package is built, and I decided that rather than try to incorporate separate symbol packages, I instead just stopped bundling the PDBs in the package.

That means I should be able to publish a new version of the native binaries package soon, and then shortly after I can have a new LibGit2Sharp preview package out that uses it.

Thank you very much for taking the time to consider solutions to this issue. I remember in your first response you stated how important it is for you to have debug symbols, so I thought my proposal was respectful of that, but I suppose the burden of a separate legacy symbol package is greater than not having the PDB files in there at all. I love that there are multiple solutions to this problem and the fact that my PR was not accepted is not upsetting at all. On the contrary, I like to think it did accomplish its goal because we are here with some solution :)

Thank you again for your contributions and time on this!

@bording
Copy link
Member

bording commented Aug 13, 2021

LibGit2Sharp 0.27.0-preview-0116 has been released, and it includes the new native binaries package that fixes this problem.

@bording bording closed this as completed Aug 13, 2021
Viir added a commit to pine-vm/pine that referenced this issue Aug 16, 2021
…tch to newer .NET versions

For discussion of the errors we got with earlier versions, see libgit2/libgit2sharp#1857 and libgit2/libgit2sharp.nativebinaries#111
Viir added a commit to pine-vm/pine that referenced this issue Aug 16, 2021
For discussion of the errors we got when trying .NET 5 with earlier versions of libgit2sharp, see libgit2/libgit2sharp#1857 and libgit2/libgit2sharp.nativebinaries#111
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 and libgit2/libgit2sharp.nativebinaries#111
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
andrei-balint added a commit to UiPath/libgit2sharp.nativebinaries that referenced this issue Nov 22, 2023
otherwise they will appear as hard dependencies for studio
libgit2#111
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 a pull request may close this issue.

3 participants