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

Adding spec of "consuming pdbs from packages in PackageReference" #11251

Merged
merged 10 commits into from
Dec 2, 2021

Conversation

heng-liu
Copy link
Contributor

@heng-liu heng-liu commented Sep 21, 2021

Adding spec of "consuming pdbs from packages in PackageReference"
The issue is #11350
(epic is #5926)

@heng-liu heng-liu marked this pull request as ready for review September 21, 2021 17:30
@heng-liu heng-liu requested a review from a team as a code owner September 21, 2021 17:30
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good :)

proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
@heng-liu
Copy link
Contributor Author

cc @dsplaisted @wli3
Appreciate if you could take a look.

Copy link

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍👍

proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @heng-liu

Nicely done.

proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
@heng-liu
Copy link
Contributor Author

heng-liu commented Sep 23, 2021

Hi @wli3, @rainersigwald, may I get more reviews from .NET SDK side? Thanks!

@heng-liu
Copy link
Contributor Author

Hi @NuGet/nuget-client , may I get another review from our team? Thanks!

@briantist
Copy link
Contributor

Hi @briantist, may I get more reviews from .NET SDK side? Thanks!

Hi @heng-liu ! I think I may have been tagged accidentally here. Letting you know so you can tag whomever you meant to, take care! 🙂

@heng-liu
Copy link
Contributor Author

Hi @briantist, may I get more reviews from .NET SDK side? Thanks!

Hi @heng-liu ! I think I may have been tagged accidentally here. Letting you know so you can tag whomever you meant to, take care! 🙂

Oops sorry! You're right. I meant to tag @rainersigwald Thanks for letting me know :)

Copy link

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but @dsplaisted's review counts double mine on this anyway.


## Problem Background

In the PackageReference world, the packages are consumed from the global-packages folder, .pdb, .xml files from lib and runtime folder are not copied into the build output folder.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PDB and XML documentation files are the most common related files, but RAR can find arbitrary related files. The list of possible extensions is user-configurable and the default is here: https://github.com/dotnet/msbuild/blob/2f1e9cad51097aec5d7268a4afdf7ad9bc6864b9/src/Tasks/Microsoft.Common.CurrentVersion.targets#L617-L627

It looks like the proposal covers this; the only thing is we'll have to be careful about "differs only by extension" to cover cases like .dll.config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reminding me for that!
So how about adding the following as additive description in the doc? (Haven't added yet)
The assembly extension .winmd, .dll and .exe will not be listed in "related" property. Any other extensions, like .dll.config, will be listed in "related" property.

proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved
proposed/2021/NuGetConsumePDBInPackageReference.md Outdated Show resolved Hide resolved

Cons:
* .pdb and .xml files are not on the same level as compile or runtime items.
The usage of .pdb and .xml files are SDK driven rather than package author driven.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean? It seems like either way if the package author decides to include the files they'll be consumed by the SDK and present in the output the same way, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
If rephrasing it as the following, is it better? Or worse? :D
From the view of a package author, compile or runtime items are the essential elements. While .pdb and .xml files are not on the same level in the understanding. So the way of having related files as a property of compile and runtime items might be in line with the understanding.

Hi @nkolev92 , may I know if you have any other opinions about rephrasing this cons? Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my confusion comes from this: if these files are present in the NuGet package, the package author had to choose to put them there (right?). So they clearly care about it and expect package consumers to use the files somehow.

@heng-liu
Copy link
Contributor Author

heng-liu commented Oct 4, 2021

Hi @clairernovotny , we plan to have this in .NET 7. May I know if you have any concerns? Thanks!

@heng-liu
Copy link
Contributor Author

heng-liu commented Oct 4, 2021

Hi @zivkan, could you help take a look at it? Thanks!

@JonDouglas
Copy link
Contributor

Please feel free to merge when you're comfortable with the reviews! Adding reviewers to help.

@clairernovotny
Copy link

I think this needs to be in the 6.0.200+ SDK, it cannot wait until .NET 7 as 6.x is LTS and more people will be using that for longer.

@heng-liu
Copy link
Contributor Author

Sorry for taking long to merge the PR.
I just address the comments in the meeting offline: Changed the cons of a contrast solution in "Consideration".
Pls let me know if there is any questions.
I'll merge it tomorrow if there is no comments. Thank you!

@heng-liu heng-liu merged commit ef5913a into dev Dec 2, 2021
@heng-liu heng-liu deleted the dev-hengliu-spec-consumePDB branch December 2, 2021 04:08
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.

None yet

8 participants