libgit2 / libgit2sharp.nativebinaries

MIT License
28 stars 62 forks source link

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

Closed CyberSinh closed 3 years ago

CyberSinh commented 3 years ago

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 : https://github.com/dotnet/runtime/issues/3807 https://github.com/dotnet/sdk/issues/3685

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

rcdailey commented 3 years ago

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

bording commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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