-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add output DebugSymbolsFiles to ResolvePackageAssets #27580
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
Conversation
6c1a9d0 to
7e9e95b
Compare
04f38a1 to
75b67c4
Compare
|
Thanks @ocallesp, this is a good start. Can you rebase and retarget this PR to the Other feedback:
Here's a sample assets file: "targets": {
"net7.0": {
"LibraryTest/1.0.0": {
"type": "package",
"compile": {
"ref/net7.0/LibraryTest.dll": {
"related": ".xml"
}
},
"runtime": {
"lib/net7.0/LibraryTest.dll": {
"related": ".pdb"
}
}
},
"Newtonsoft.Json/4.5.4": {
"type": "package",
"compile": {
"lib/net40/Newtonsoft.Json.dll": {
"related": ".pdb;.xml"
}
},
"runtime": {
"lib/net40/Newtonsoft.Json.dll": {
"related": ".pdb;.xml"
}
}
}
}
}, |
4f6e5ea to
144446f
Compare
If we apply this change to |
144446f to
4611e7d
Compare
Yes, the code will eventually flow forward to later branches. |
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs
Outdated
Show resolved
Hide resolved
fd97481 to
1722738
Compare
5c7d4aa to
1722738
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far.
...estProjects/DesktopUsingPackageWithPdbWithoutXml/DesktopUsingPackageWithPdbWithoutXml.csproj
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs
Outdated
Show resolved
Hide resolved
bf35bbe to
52eb122
Compare
8eff3f6 to
25590b0
Compare
|
@dsplaisted
|
DebugSymbolsFiles outputs the list of absolute path to symbols files for package references. ReferenceDocumentationFiles outputs the list of absolute path to Xml documentation files. The change: ResolvePackagesAssets was modified to read/write DebugSymbolsFiles and ReferenceDocumentationFiles from/to the cache file. They were inserted after ContentFilesToProcess to keep the alphabetical order. The target was modified to copy the DebugSymbolsFiles to output directory only when CopyDebugSymbolFilesFromPackages is true, and ReferenceDocumentationFiles to the output directory when EnableCopySymbolsAndXmlDocsToOutputDir is true. There is some minor code cleanup not related to this bug.
25590b0 to
be82e56
Compare
|
@dsplaisted changes done as suggested. |
d9f14b4 to
0231a17
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very good, and pretty close to done I think. Thanks!
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets
Show resolved
Hide resolved
|
@baronfel, What do you think about the property names Also pinging @dotnet/dotnet-cli in case anyone else has feedback. |
c01b3fb to
eb07219
Compare
…ectory - Testing CopyDebugSymbolFilesFromPackages and CopyDocumentationFilesFromPackagesAdd unit-tests for EnableCopySymbolsAndXmlDocsToOutputDir
6902fda to
0debac3
Compare
|
I'm ok with the current names - I'd rather align with the existing terminology to stay consistent. The I also agree with the plan to not enable the flag by default in this release, due to timing. We should explore turning it on for 7.0.200 and beyond, though - I think the existence of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 minor comments, looks good otherwise.
|
|
||
| foreach (string fileExtension in relatedExtensions.Split(RelatedPropertySeparator)) | ||
| { | ||
| if (fileExtension.ToLower() == extension) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use invariant comparison
| if (fileExtension.ToLower() == extension) | |
| if (fileExtension.ToLowerInvariant() == extension) |
| } | ||
| else | ||
| { | ||
| _task.Log.LogWarning(Strings.AssetsFileNotFound, xmlFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really the right error message here. Do we need to log a warning at all here?
…FilesFromPackages (#8621) Related to a new feature (consuming pdbs/documents from packages), which was brought in in .NET 7. PR from sdk: dotnet/sdk#27580 Document the `CopyDebugSymbolFilesFromPackages` and `CopyDocumentationFilesFromPackages` properties.
fixes #1458
DebugSymbolsFilesoutputs the list of absolute path to symbols files (.pdb) from runtime assemblies.ReferenceDocumentationFilesoutputs the list of absolute path to xml files that enables debugging features.The change:
Added
DebugSymbolsFilesandReferenceDocumentationFilesandResolvePackagesAssetswas modified to read/writeDebugSymbolsFilesandReferenceDocumentationFilesfrom/to the cache file.The debug symbols files will be copied to the output directory when
CopyDebugSymbolFilesFromPackagesquals to true.The reference documentation files will be copied to the output directory when
CopyDocumentationFilesFromPackagesquals to true.For manual testing, I was using this sample project #1383 and checking that the pdb are copied to the output directory of the consumer.