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

Refactor "PDB distributions" section of README #1271

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

MattKotsenas
Copy link
Member

@MattKotsenas MattKotsenas commented Aug 28, 2024

Refactor the "PDB distributions" section of the README so that:

  • Add a small pros / cons table explaining the benefits and limitations of: .snupkgs, Adding to the main package, and assembly embedding
  • There's less bouncing back and forth between text (specifically shrinking the distance between introducing a .snupkg and talking about the limitations)
  • Add explanation of embedding and benefits / limitations
  • Update "main package" alternative to make it clear that it requires consumers to set CopyDebugSymbolFilesFromPackages
  • Use GitHub markdown tags for asides or secondary information

Fixes #255

@MattKotsenas
Copy link
Member Author

MattKotsenas commented Aug 28, 2024

I originally wrote this change as part of #1270, but separated it into its own PR since this one is a bit more nuanced and contains some opinions instead of just facts.

More specifically, the only downside I see for advocating for embedded pdbs over separate files included in the main package is it's easier to strip separate files for people that really care about size. I'm not convinced that requiring every consumer to opt-in via CopyDebugSymbolFilesFromPackages is a good tradeoff though, and they should instead either publish on a platform that supports snupkg or use another solution like symbol servers.

@MattKotsenas
Copy link
Member Author

@tmat, would you mind taking a look at this when you have a second? If you have any questions or concerns, please let me know. Thanks!

README.md Show resolved Hide resolved
@ViktorHofer ViktorHofer removed their request for review January 13, 2025 19:49
@MattKotsenas MattKotsenas requested a review from tmat January 13, 2025 19:50
@MattKotsenas
Copy link
Member Author

/cc @tmat, mind making another review? If you have any questions / concerns, please let me know. Thanks!

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Since Windows-only PDBs are legacy for managed code, and this section is
about managed code, the "supports all types" row isn't really relvant.
Remove it.
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 this pull request may close these issues.

guidance for embedding pdb in nupkg vs snupkg
3 participants