Add output DebugSymbolsFiles to ResolvePackageAssets #27580

ocallesp · 2022-08-31T02:05:54Z

DebugSymbolsFiles outputs the list of absolute path to symbols files (.pdb) from runtime assemblies.
ReferenceDocumentationFiles outputs the list of absolute path to xml files that enables debugging features.

The change:
Added DebugSymbolsFiles and ReferenceDocumentationFiles and ResolvePackagesAssets was modified to read/write DebugSymbolsFiles and ReferenceDocumentationFiles from/to the cache file.

The debug symbols files will be copied to the output directory when CopyDebugSymbolFilesFromPackages quals to true.

The reference documentation files will be copied to the output directory when CopyDocumentationFilesFromPackages quals 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.

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

dsplaisted · 2022-08-31T20:42:40Z

Thanks @ocallesp, this is a good start. Can you rebase and retarget this PR to the release/7.0.1xx branch?

Other feedback:

I think there should be separate output items on the task for the PDB and XML files. The XML files should only be included in the publish output if PublishReferencesDocumentationFiles is true (I think that for the build output they should always be included).
The XML files should come from the compile assets in the project.assets.json file, not the runtime assets.

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"
          }
        }
      }
    }
  },

ocallesp · 2022-08-31T22:50:14Z

Thanks @ocallesp, this is a good start. Can you rebase and retarget this PR to the release/7.0.1xx branch?

Other feedback:

I think there should be separate output items on the task for the PDB and XML files. The XML files should only be included in the publish output if PublishReferencesDocumentationFiles is true (I think that for the build output they should always be included).

The XML files should come from the compile assets in the project.assets.json file, not the runtime assets.

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"
          }
        }
      }
    }
  },

If we apply this change to release/7.0.1xx branch, does the pipeline/insertions also apply this change to main ?

dsplaisted · 2022-09-01T00:04:56Z

If we apply this change to release/7.0.1xx branch, does the pipeline/insertions also apply this change to main ?

Yes, the code will eventually flow forward to later branches.

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

dsplaisted

Looking good so far.

...estProjects/DesktopUsingPackageWithPdbWithoutXml/DesktopUsingPackageWithPdbWithoutXml.csproj

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs

ocallesp · 2022-09-08T22:53:21Z

@dsplaisted
Copying pdb and xml to output and publish directories are now disable by default.

The debug symbols files will be copied to the output directory when CopyDebugSymbolFilesFromPackages quals to true.
The reference documentation files will be copied to the output directory when CopyDocumentationFilesFromPackages quals to true.

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.

ocallesp · 2022-09-09T02:26:35Z

@dsplaisted changes done as suggested.

dsplaisted

Looking very good, and pretty close to done I think. Thanks!

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets

dsplaisted · 2022-09-09T21:28:28Z

@baronfel, What do you think about the property names CopyDebugSymbolFilesFromPackages and CopyDocumentationFilesFromPackages? Should we consider something like CopyPdbFilesFromPackages and CopyXmlFilesFromPackages? The current names try to match the existing PublishReferencesDocumentationFiles property.

Also pinging @dotnet/dotnet-cli in case anyone else has feedback.

…ectory - Testing CopyDebugSymbolFilesFromPackages and CopyDocumentationFilesFromPackagesAdd unit-tests for EnableCopySymbolsAndXmlDocsToOutputDir

baronfel · 2022-09-12T14:44:18Z

I'm ok with the current names - I'd rather align with the existing terminology to stay consistent. The PublishDocumentationFile(s) and PublishReferencesDocumentationFiles properties don't seem to be documented yet, so when this changes I'll bundle documenting them and these new properties in a single PR to dotnet/docs so that we're up-to-date and maybe get some SEO on the change.

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 PublishReferencesDocumentationFiles property and it being set to true by default is a strong enough signal to change this mid-release-cycle rather than waiting for the 8.0.x SDK series.

dsplaisted

2 minor comments, looks good otherwise.

dsplaisted · 2022-09-12T19:25:54Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

+
+                        foreach (string fileExtension in relatedExtensions.Split(RelatedPropertySeparator))
+                        {
+                            if (fileExtension.ToLower() == extension)


This should probably use invariant comparison

Suggested change

if (fileExtension.ToLower() == extension)

if (fileExtension.ToLowerInvariant() == extension)

dsplaisted · 2022-09-12T19:27:04Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs

+                                }
+                                else
+                                {
+                                    _task.Log.LogWarning(Strings.AssetsFileNotFound, xmlFilePath);


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.

ocallesp requested a review from dsplaisted August 31, 2022 02:05

dotnet-issue-labeler bot added the Area-Infrastructure label Aug 31, 2022

ocallesp force-pushed the githubFix1458 branch 2 times, most recently from 6c1a9d0 to 7e9e95b Compare August 31, 2022 02:08

ocallesp self-assigned this Aug 31, 2022

ocallesp commented Aug 31, 2022

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs Outdated Show resolved Hide resolved

ocallesp requested a review from marcpopMSFT August 31, 2022 02:53

ocallesp force-pushed the githubFix1458 branch 7 times, most recently from 04f38a1 to 75b67c4 Compare August 31, 2022 18:34

ocallesp force-pushed the githubFix1458 branch from 4f6e5ea to 144446f Compare August 31, 2022 22:17

ocallesp force-pushed the githubFix1458 branch from 144446f to 4611e7d Compare August 31, 2022 23:43

ocallesp changed the base branch from main to release/7.0.1xx August 31, 2022 23:44

ocallesp marked this pull request as ready for review September 1, 2022 01:45

ocallesp commented Sep 1, 2022

View reviewed changes

src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs Outdated Show resolved Hide resolved

ocallesp force-pushed the githubFix1458 branch 3 times, most recently from fd97481 to 1722738 Compare September 1, 2022 19:45

ocallesp commented Sep 1, 2022

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs Outdated Show resolved Hide resolved

ocallesp force-pushed the githubFix1458 branch from 5c7d4aa to 1722738 Compare September 1, 2022 22:58

ocallesp commented Sep 1, 2022

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs Show resolved Hide resolved

dsplaisted reviewed Sep 2, 2022

View reviewed changes

ocallesp force-pushed the githubFix1458 branch from bf35bbe to 52eb122 Compare September 2, 2022 02:00

ocallesp force-pushed the githubFix1458 branch 2 times, most recently from 8eff3f6 to 25590b0 Compare September 8, 2022 22:06

ocallesp force-pushed the githubFix1458 branch from 25590b0 to be82e56 Compare September 9, 2022 02:20

ocallesp force-pushed the githubFix1458 branch 4 times, most recently from d9f14b4 to 0231a17 Compare September 9, 2022 20:28

dsplaisted reviewed Sep 9, 2022

View reviewed changes

ocallesp force-pushed the githubFix1458 branch from c01b3fb to eb07219 Compare September 10, 2022 00:41

ocallesp requested a review from dsplaisted September 10, 2022 01:27

Add unit-tests for copy debug symbols and documentation to output dir…

0debac3

…ectory - Testing CopyDebugSymbolFilesFromPackages and CopyDocumentationFilesFromPackagesAdd unit-tests for EnableCopySymbolsAndXmlDocsToOutputDir

ocallesp force-pushed the githubFix1458 branch from 6902fda to 0debac3 Compare September 10, 2022 06:40

baronfel mentioned this pull request Sep 12, 2022

Don't ship the FSharp.Core XmlDocs as Content in the nuget package. dotnet/fsharp#13838

Merged

ocallesp enabled auto-merge September 12, 2022 16:54

dsplaisted approved these changes Sep 12, 2022

View reviewed changes

ocallesp merged commit cd4dba4 into release/7.0.1xx Sep 12, 2022

ocallesp deleted the githubFix1458 branch September 12, 2022 19:30

ocallesp mentioned this pull request Sep 13, 2022

New project system doesn't copy PDBs from packages #1458

Closed

baronfel mentioned this pull request Sep 13, 2022

Document the CopyDebugSymbolFilesFromPackages and CopyDocumentationFilesFromPackages properties dotnet/docs#31140

Closed

dsplaisted mentioned this pull request Sep 27, 2022

Increase version of asset cache file format to 12 #28218

Closed

ocallesp mentioned this pull request Sep 29, 2022

Increase cache assets file format version to 11 in 7.0.1xx #28222

Merged

5 tasks

heng-liu mentioned this pull request Apr 1, 2023

Add doc for CopyDebugSymbolFilesFromPackages and CopyDocumentationFilesFromPackages properties dotnet/msbuild#8621

Merged

pepone mentioned this pull request Jun 12, 2023

Copy PDBs from NuGet packages icerpc/icerpc-csharp#3347

Merged

BretJohnson mentioned this pull request Aug 17, 2023

Default VS symbol loading mechanism does not work dotnet/maui#10654

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add output DebugSymbolsFiles to ResolvePackageAssets #27580

Add output DebugSymbolsFiles to ResolvePackageAssets #27580

ocallesp commented Aug 31, 2022 •

edited

Loading

dsplaisted commented Aug 31, 2022

ocallesp commented Aug 31, 2022

dsplaisted commented Sep 1, 2022

dsplaisted left a comment

ocallesp commented Sep 8, 2022 •

edited

Loading

ocallesp commented Sep 9, 2022

dsplaisted left a comment

dsplaisted commented Sep 9, 2022

baronfel commented Sep 12, 2022

dsplaisted left a comment

dsplaisted Sep 12, 2022

dsplaisted Sep 12, 2022

	if (fileExtension.ToLower() == extension)
	if (fileExtension.ToLowerInvariant() == extension)

Add output DebugSymbolsFiles to ResolvePackageAssets #27580

Add output DebugSymbolsFiles to ResolvePackageAssets #27580

Conversation

ocallesp commented Aug 31, 2022 • edited Loading

dsplaisted commented Aug 31, 2022

ocallesp commented Aug 31, 2022

dsplaisted commented Sep 1, 2022

dsplaisted left a comment

Choose a reason for hiding this comment

ocallesp commented Sep 8, 2022 • edited Loading

ocallesp commented Sep 9, 2022

dsplaisted left a comment

Choose a reason for hiding this comment

dsplaisted commented Sep 9, 2022

baronfel commented Sep 12, 2022

dsplaisted left a comment

Choose a reason for hiding this comment

dsplaisted Sep 12, 2022

Choose a reason for hiding this comment

dsplaisted Sep 12, 2022

Choose a reason for hiding this comment

ocallesp commented Aug 31, 2022 •

edited

Loading

ocallesp commented Sep 8, 2022 •

edited

Loading