Enable PDB archives for source-build #14015

NikolaMilosavljevic · 2023-09-07T01:03:49Z

Resolves: dotnet/source-build#3547, dotnet/source-build#3609 and dotnet/source-build#3610

This change enables creation of repo-specific PDB archives, named Symbols.<repo-name>.tar.gz. Archives get included in main source-build intermediate package, and eventually extracted to artifacts/<chip>/<flavor> i.e. artifacts/x64/Release.

New infra collects all PDBs using **\obj\**\*.pdb pattern.

File hierarchy is preserved, which enables archive to contain multiple files with the same name. File hierarchy is rooted in $(CurrentRepoSourceBuildArtifactsDir) except for source-build-externals repo, which uses $(RepoRoot).

Validation VMR build is in progress: https://dev.azure.com/dnceng/internal/_build/results?buildId=2261578&view=results

Example content, from Symbols.sourcelink.tar.gz:

obj/dotnet-sourcelink/Release/net8.0/dotnet-sourcelink.pdb
obj/Microsoft.Build.Tasks.Git/Release/net8.0/Microsoft.Build.Tasks.Git.pdb
obj/Microsoft.SourceLink.AzureDevOpsServer.Git/Release/net8.0/Microsoft.SourceLink.AzureDevOpsServer.Git.pdb
obj/Microsoft.SourceLink.AzureRepos.Git/Release/net8.0/Microsoft.SourceLink.AzureRepos.Git.pdb
obj/Microsoft.SourceLink.Bitbucket.Git/Release/net8.0/Microsoft.SourceLink.Bitbucket.Git.pdb
obj/Microsoft.SourceLink.Common/Release/net8.0/Microsoft.SourceLink.Common.pdb
obj/Microsoft.SourceLink.Gitea/Release/net8.0/Microsoft.SourceLink.Gitea.pdb
obj/Microsoft.SourceLink.Gitee/Release/net8.0/Microsoft.SourceLink.Gitee.pdb
obj/Microsoft.SourceLink.GitHub/Release/net8.0/Microsoft.SourceLink.GitHub.pdb
obj/Microsoft.SourceLink.GitLab/Release/net8.0/Microsoft.SourceLink.GitLab.pdb
obj/Microsoft.SourceLink.GitWeb/Release/net8.0/Microsoft.SourceLink.GitWeb.pdb

Here are some numbers:

Repo	Previous intermediate size (MB)	Symbols archive (MB)	New intermediate size (MB)
arcade	6	0	6
aspnetcore	94	7	101
cecil	0.6	0.3	0.9
command-line-api	0.6	0.3	0.9
deplyment-tools	0.1	0.03	0.13
diagnostics	0.06	0.06	0.12
emsdk	0.04	0	0.04
format	14.4	0.07	14.5
fsharp	109	0	109
installer	203	0	203
msbuild	8.4	3.4	11.8
nuget-client	5.3	1.4	6.7
razor	1	1	2
roslyn	78	41	119
roslyn-analyzers	18.3	3	21.3
runtime - crossgen	256	0	256
runtime - ilcompiler	79	0	79
runtime - main	177	128	305
runtime - runtime	139	0	139
sdk	74.7	2.2	76.9
source-build-externals	3	3	6
source-build-reference-packages	23.5	0	23.5
sourcelink	0.7	0.1	0.8
symreader	0.3	0.06	0.4
templating	1.3	0.6	2
test-templates	0.2	0.004	0.2
vstest	3	0	3
xdt	0.2	0.1	0.3
xliff-tasks	0.1	0.04	0.15
Totals	1297	192	1489

mthalman · 2023-09-07T12:51:18Z

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildIntermediate.proj

@@ -79,6 +81,48 @@
    </ItemGroup>
  </Target>

+  <!-- Discover all repo's symbols that match **\obj\**\*.pdb, to be included in the "main" intermediate nupkg. -->
+  <Target Name="CreateSymbolsList" Condition="'$(SupplementalIntermediateNupkgCategory)' == ''">


While I appreciate the breaking down of logic into smaller chunks, it feels weird to me that CreateSymbolsList is a separate target whose output is then deleted by another target: CreateSymbolsArchive. It seems like the symbols list is just an implementation detail of the logic to create the archive and doesn't deserve a separate target.

Yes, this is true. It started as a single target which I split into two to simplify conditioning of few tasks (packaging, message, deletion of symbols list) - each task would need its own condition.

However, I see the point and will revert to my original design.

Fixed with de9bb57

NikolaMilosavljevic · 2023-09-07T18:39:56Z

Found a small issue in one of the conditions - it caused few issues: 1) no archive for source-build-externals and 2) unnecessarily longer hierarchy in most packages. Testing the fix locally...

omajid · 2023-09-07T20:55:17Z

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildIntermediate.proj

+    </PropertyGroup>
+
+    <ItemGroup>
+      <AbsoluteSymbolPath Include="$(SymbolsRoot)\**\obj\**\*.pdb" />


Apologies if this is a stupid question, but is there any risk of a not-just-built pdb file being available at this path? Is there any support for posioning pdb files to catch potential leaks?

obj folder would contain content of just compiled sources. bin folder would also contain dependencies.

MichaelSimons

Looks good. I'd be curious to see what the size impact is to the current intermediates. I know runtime has run into the 500MB limit AZDO has on individual artifact files.

MichaelSimons · 2023-09-07T21:59:25Z

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildIntermediate.proj

+      <SymbolsArchiveLocation>$(CurrentRepoSourceBuildArtifactsPackagesDir)</SymbolsArchiveLocation>
+      <SymbolsList>$(SymbolsArchiveLocation)\symbols.lst</SymbolsList>
+      <SymbolsArchivePrefix>Symbols.</SymbolsArchivePrefix>
+      <SymbolsArchiveName>$(SymbolsArchiveLocation)$(SymbolsArchivePrefix)$(GitHubRepositoryName).tar.gz</SymbolsArchiveName>


Should the symbols tarball include the rid and version like other SB artifacts?

That's a good idea - will update the name so it matches other artifacts.

Source-build intermediate package does not include the Rid, i.e. Microsoft.SourceBuild.Intermediate.sdk.8.0.100-rc.2.23423.14.nupkg

I see that infra is evaluating Rid from current OS, which produces RIDs like linux-x64, so that could be used. But, is there a value - would version be enough?

TargetRID in SourceBuildIntermediate.proj, which is called from AfterSourceBuild.proj, is set to linux-x64.

If we use both version and rid we get this: Symbols.sourcelink.8.0.0-beta.23422.2.linux-x64.tar.gz

Just version: Symbols.sourcelink.8.0.0-beta.23422.2.tar.gz

Related intermediate package: Microsoft.SourceBuild.Intermediate.sourcelink.8.0.0-beta.23422.2.nupkg

It seems that only runtime and sdk have non-portable RID set in TargetRid property. Other repos use the portable Rid, as we are doing portable source-builds for majority of the repos..

My test build with both version and rid produced the following - naming is inconsistent, but it matches the Rid used for repo's build:

Symbols.aspnetcore.8.0.0-rc.2.23423.7.linux-x64.tar.gz Symbols.cecil.0.11.4.0-alpha.23407.2.linux-x64.tar.gz Symbols.command-line-api.0.1.435601.linux-x64.tar.gz Symbols.deployment-tools.8.0.0-preview.6.23407.1.linux-x64.tar.gz Symbols.diagnostics.7.0.0-preview.23211.1.linux-x64.tar.gz Symbols.format.8.0.442201.linux-x64.tar.gz Symbols.msbuild.17.8.0-preview-23424-01.linux-x64.tar.gz Symbols.nuget-client.1.0.0-preview.23424.1.linux-x64.tar.gz Symbols.razor.7.0.0-preview.23423.3.linux-x64.tar.gz Symbols.roslyn-analyzers.3.11.0-beta1.23420.2.linux-x64.tar.gz Symbols.roslyn.4.8.0-2.23423.10.linux-x64.tar.gz Symbols.runtime.8.0.0-rc.2.23418.14.fedora.36-x64.tar.gz Symbols.sdk.8.0.100-rc.2.23423.14.fedora.36-x64.tar.gz Symbols.source-build-externals.8.0.0-alpha.1.23421.1.linux-x64.tar.gz Symbols.sourcelink.8.0.0-beta.23422.2.linux-x64.tar.gz Symbols.symreader.2.1.0-beta.23253.1.linux-x64.tar.gz Symbols.templating.8.0.100-rc.2.23423.2.linux-x64.tar.gz Symbols.test-templates.1.1.0-rc.23410.2.linux-x64.tar.gz Symbols.xdt.7.0.0-preview.22423.2.linux-x64.tar.gz Symbols.xliff-tasks.1.0.0-beta.23418.1.linux-x64.tar.gz

Fixed with e41ac2f

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildIntermediate.proj

MichaelSimons · 2023-09-07T22:13:34Z

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildIntermediate.proj

+
+    <ItemGroup>
+      <AbsoluteSymbolPath Include="$(SymbolsRoot)\**\obj\**\*.pdb" />
+      <RelativeSymbolPath Include="@(AbsoluteSymbolPath)" Condition="'@(AbsoluteSymbolPath)' != ''">


Curious if this can be achieved in one ItemGroup
$([MSBuild]::MakeRelative($(SymbolsRoot), %(FullPath)))

Will test it out - thanks!

We can do something like this:

<AbsoluteSymbolPath Include="$(SymbolsRoot)\**\obj\**\*.pdb" /> <AbsoluteSymbolPath Update="@(AbsoluteSymbolPath)" Condition="'@(AbsoluteSymbolPath)' != ''"> <RelativePath>$([MSBuild]::MakeRelative($(SymbolsRoot), %(FullPath)))</RelativePath> </AbsoluteSymbolPath>

It removes one ItemGroup.

Fixed with e41ac2f

MichaelSimons · 2023-09-07T22:14:41Z

src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildIntermediate.proj

+    </PropertyGroup>
+
+    <ItemGroup>
+      <AbsoluteSymbolPath Include="$(SymbolsRoot)\**\obj\**\*.pdb" />


I typically seem ItemGroups name in plural form as the are a type of collection. e.g. AbsoluteSymbolPaths

I've seen both plural and singular in use and can't seem to find any guidance for .NET codebase. It appears that even source-build infra is inconsistent, i.e.

Singular:

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcade.targets

Line 83 in 08a46d5

<IntermediateNupkgArtifactFile Include="$(CurrentRepoSourceBuildArtifactsPackagesDir)**\*.nupkg" />

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildIntermediate.proj

Line 60 in 08a46d5

<Content Include="@(IntermediatePackageFile->WithMetadataValue('Category', '$(SupplementalIntermediateNupkgCategory)'))" />

Plural:

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeBuild.targets

Line 115 in 08a46d5

<SourceBuildFilesToCopy Include="$(RepoRoot)/**/*" />

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/AfterSourceBuild.proj

Line 23 in 08a46d5

<SourceBuiltPackageFiles Include="$(CurrentRepoSourceBuiltNupkgCacheDir)**/*.nupkg" />

I acknowledge there are inconsistencies in the code base. I asked to see if you had reasons for using singular. I personally find the plural form can help the readability therefore tend to favor that form as a general rule.

…rchive

NikolaMilosavljevic · 2023-09-08T00:32:16Z

Looks good. I'd be curious to see what the size impact is to the current intermediates. I know runtime has run into the 500MB limit AZDO has on individual artifact files.

Added the relevant data to main comment.

NikolaMilosavljevic · 2023-09-08T00:37:22Z

Found a small issue in one of the conditions - it caused few issues: 1) no archive for source-build-externals and 2) unnecessarily longer hierarchy in most packages. Testing the fix locally...

This is now fixed. I've updated the initial comment with updated symbols paths in archive packages and added all relevant numbers for intermediate and symbols archive sizes.

mthalman

We want this for .NET 8, right? It's easier to just target release/8.0.1xx first and, once it's merged, it will then automatically be ported to main branch.

NikolaMilosavljevic · 2023-09-08T14:46:32Z

We want this for .NET 8, right? It's easier to just target release/8.0.1xx first and, once it's merged, it will then automatically be ported to main branch.

Arcade repo doesn't have any 8.0-specific branch, they flow their main branch to installer's release/8.0.1xx branch; e.g. dotnet/installer#17293

I'm going to make a small update to this PR to address 2 of @MichaelSimons comments.

Enable PDB archives

9da7213

NikolaMilosavljevic requested review from omajid, tmds, mmitche and a team September 7, 2023 01:03

mthalman reviewed Sep 7, 2023

View reviewed changes

MichaelSimons mentioned this pull request Sep 7, 2023

[smoke-tests] Sourcelink tests should verify all source-build symbols dotnet/source-build#3612

Closed

Address review comments

de9bb57

omajid reviewed Sep 7, 2023

View reviewed changes

MichaelSimons reviewed Sep 7, 2023

View reviewed changes

Fixing symbols paths in archives and missing source-build-externals a…

2e09bfe

…rchive

mthalman previously approved these changes Sep 8, 2023

View reviewed changes

Few small fixes

e41ac2f

NikolaMilosavljevic dismissed mthalman’s stale review via e41ac2f September 8, 2023 17:20

MichaelSimons approved these changes Sep 11, 2023

View reviewed changes

mthalman approved these changes Sep 11, 2023

View reviewed changes

NikolaMilosavljevic merged commit 2bcf51c into dotnet:main Sep 11, 2023
11 checks passed

This was referenced Sep 19, 2023

Move runtime symbols out of main intermediate package #14040

Merged

Consolidate symbols into a single artifact dotnet/source-build#3621

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable PDB archives for source-build #14015

Enable PDB archives for source-build #14015

NikolaMilosavljevic commented Sep 7, 2023 •

edited

Loading

mthalman Sep 7, 2023

NikolaMilosavljevic Sep 7, 2023

NikolaMilosavljevic Sep 7, 2023

NikolaMilosavljevic commented Sep 7, 2023

omajid Sep 7, 2023

NikolaMilosavljevic Sep 8, 2023

MichaelSimons left a comment

MichaelSimons Sep 7, 2023

NikolaMilosavljevic Sep 8, 2023

NikolaMilosavljevic Sep 8, 2023

NikolaMilosavljevic Sep 8, 2023

NikolaMilosavljevic Sep 8, 2023

MichaelSimons Sep 7, 2023

NikolaMilosavljevic Sep 8, 2023

NikolaMilosavljevic Sep 8, 2023

NikolaMilosavljevic Sep 8, 2023

MichaelSimons Sep 7, 2023

NikolaMilosavljevic Sep 8, 2023

MichaelSimons Sep 11, 2023

NikolaMilosavljevic commented Sep 8, 2023

NikolaMilosavljevic commented Sep 8, 2023

mthalman left a comment

NikolaMilosavljevic commented Sep 8, 2023

Enable PDB archives for source-build #14015

Enable PDB archives for source-build #14015

Conversation

NikolaMilosavljevic commented Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolaMilosavljevic commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolaMilosavljevic commented Sep 8, 2023

NikolaMilosavljevic commented Sep 8, 2023

mthalman left a comment

Choose a reason for hiding this comment

NikolaMilosavljevic commented Sep 8, 2023

NikolaMilosavljevic commented Sep 7, 2023 •

edited

Loading