Source-build symbols repackaging #17454

NikolaMilosavljevic · 2023-10-03T18:07:10Z

Also adds a new symbols package for SDK, that contains PDBs in proper folder hierarchy - dotnet/source-build#3621 (comment)

Instead of 20 Symbols.<repo>.* tarballs, we'll have the following 2:

dotnet-sdk-symbols-8.0.100-rtm.23477.1-fedora.38-x64.tar.gz
sourcebuilt-symbols-8.0.100-rtm.23477.1-fedora.38-x64.tar.gz

First tarball contains SDK symbols that match those binaries. It enables developers to extract dotnet-sdk and dotnet-sdk-symbols tarballs to the same destination folder, and be able to debug every single .NET assembly, as PDBs will be placed right next to binaries.

Second tarball contains all symbols from all repos. This tarball introduces new naming schema for artifacts produced by source-build infra. In 9.0 we should use the new naming schema for all other source-build artifacts, i.e. various Private.Sourcebuilt.* tarballs.

mthalman · 2023-10-03T18:25:28Z

src/SourceBuild/content/build.proj

+    </ItemGroup>
+  </Target>
+
+  <Target Name="ExtractSymbolsTarballs"


This should have a DependsOnTargets for DiscoverSymbolsTarballs.

I don't think it's necessary due to target order in DependsOnTargets attribute of RepackageSymbols target.

But, it won't hurt to have it here as well.

Fixed in abc74a8

mthalman · 2023-10-03T18:32:37Z

src/SourceBuild/content/build.proj

+            DiscoverSymbolsTarballs;
+            ExtractSymbolsTarballs">
+    <PropertyGroup>
+      <UnifiedSymbolsTarball>$(OutputPath)sourcebuilt-symbols-$(MicrosoftSourceBuildIntermediateInstallerVersion)-$(TargetRid).tar.gz</UnifiedSymbolsTarball>


I feel like this should have dotnet in the name.

Suggested change

<UnifiedSymbolsTarball>$(OutputPath)sourcebuilt-symbols-$(MicrosoftSourceBuildIntermediateInstallerVersion)-$(TargetRid).tar.gz</UnifiedSymbolsTarball>

<UnifiedSymbolsTarball>$(OutputPath)dotnet-sourcebuilt-symbols-$(MicrosoftSourceBuildIntermediateInstallerVersion)-$(TargetRid).tar.gz</UnifiedSymbolsTarball>

Good idea. I expected a discussion on this naming as it would influence the naming of other source-built artifacts. cc @MichaelSimons

Fixed in abc74a8

src/SourceBuild/content/build.proj

MichaelSimons · 2023-10-05T17:20:12Z

...d/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs

+                    string guid = $"{id.Guid:N}";
+                    if (!string.IsNullOrEmpty(guid))
+                    {
+                        if (!AllPdbGuids.ContainsKey(guid))


Consider consolidating this if with the previous. Nested if's don't seem to be adding value.

True - will fix this.

Fixed in b9f64fd

MichaelSimons · 2023-10-05T17:21:24Z

...d/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs

+                        }
+                    }
+                }
+                catch(Exception)


Are there specific scenarios you expect here or is this a general purpose safegaurd?

Just general purpose. We would catch any symbol for which we could not obtain the guid. Originally I was logging this, but there is no value in that.

My question was more along the lines if there was a specific exception type(s) we should be catching instead of all. Catching all can potentially mask problems. This comment applies to the previous catch statement as well.

I have considered handling just the specific exceptions... Perhaps we can improve this later.

It won't get fixed later 😄 so do it now or never.

OK - fixing now 😄

Fixed in afd434a

MichaelSimons · 2023-10-05T17:23:04Z

...d/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs

+
+        public void IndexAllSymbols(string path)
+        {
+            AllPdbGuids = new Hashtable();


From a design perspective, what is the reason for tracking AllPdbGuids as an instance versus returning this from this method?

Not much now honestly. The code got much simpler as I was revising it, so we don't need this class member anymore.

Will fix this.

Fixed in b9f64fd

MichaelSimons · 2023-10-05T17:27:10Z

...d/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs

+        public override bool Execute()
+        {
+            IndexAllSymbols(AllSymbolsPath);
+            IList<string> filesWithoutPDBs = GenerateSymbolsLayout(SdkLayoutPath, SdkSymbolsLayoutPath);


Are the parameters necessary? They are instance members and could be accesses directly within the method.

Thanks for catching this. Originally used for testing, but didn't refactor in the end.

Wil fix.

Fixed in b9f64fd

MichaelSimons · 2023-10-05T17:30:29Z

src/SourceBuild/content/build.proj

+      <SdkSymbolsLayout>$(ArtifactsTmpDir)SdkSymbols</SdkSymbolsLayout>
+      <SdkSymbolsTarball>$(OutputPath)dotnet-sdk-symbols-$(MicrosoftSourceBuildIntermediateInstallerVersion)-$(TargetRid).tar.gz</SdkSymbolsTarball>
+      <SdkLayout>$(ArtifactsTmpDir)Sdk</SdkLayout>
+      <SdkTarballPath>%(SdkTarballItem.Identity)</SdkTarballPath>


The Path suffix here introduces inconsistency -e.g. how is this different that the SdkSymbolsTarball?

Fixed in b9f64fd

Co-authored-by: Michael Simons <msimons@microsoft.com>

NikolaMilosavljevic · 2023-10-05T18:29:28Z

...d/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs

+
+        public override bool Execute()
+        {
+            IndexAllSymbols(AllSymbolsPath);


The parameter isn't needed here - will fix.

Fixed in b9f64fd

...d/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs

…eBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs Co-authored-by: Michael Simons <msimons@microsoft.com>

MichaelSimons

IMO, ideally the code wouldn't need the catch (Exception) blocks. but I don't have a strong objection.

NikolaMilosavljevic · 2023-10-05T22:44:18Z

...d/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs

+
+            foreach (string file in Directory.GetFiles(AllSymbolsPath, "*.pdb", SearchOption.AllDirectories))
+            {
+                try


Try-catch block isn't needed here - will remove it.

Fixed in afd434a

NikolaMilosavljevic · 2023-10-06T02:09:38Z

PR is stuck in license/cla - trying to close/reopen.

Symbols repackaging

e7882bb

NikolaMilosavljevic requested a review from a team as a code owner October 3, 2023 18:07

mthalman reviewed Oct 3, 2023

View reviewed changes

Addressing review comments

abc74a8

mthalman approved these changes Oct 4, 2023

View reviewed changes

mthalman requested a review from a team October 4, 2023 19:05

MichaelSimons added the Area-SourceBuild label Oct 5, 2023

MichaelSimons reviewed Oct 5, 2023

View reviewed changes

Update src/SourceBuild/content/build.proj

292b853

Co-authored-by: Michael Simons <msimons@microsoft.com>

NikolaMilosavljevic commented Oct 5, 2023

View reviewed changes

Addressing more review comments

b9f64fd

MichaelSimons reviewed Oct 5, 2023

View reviewed changes

...d/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs Outdated Show resolved Hide resolved

Update src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.Sourc…

fe99927

…eBuild.Tasks.XPlat/CreateSdkSymbolsLayout.cs Co-authored-by: Michael Simons <msimons@microsoft.com>

MichaelSimons approved these changes Oct 5, 2023

View reviewed changes

NikolaMilosavljevic commented Oct 5, 2023

View reviewed changes

Exception handling fixes

afd434a

NikolaMilosavljevic closed this Oct 6, 2023

NikolaMilosavljevic reopened this Oct 6, 2023

NikolaMilosavljevic merged commit 77a7628 into dotnet:release/8.0.1xx Oct 6, 2023

NikolaMilosavljevic mentioned this pull request Oct 6, 2023

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

Closed

tmds mentioned this pull request Oct 12, 2023

Managed symbols tarball seems to be missing files dotnet/source-build#3656

Closed

NikolaMilosavljevic mentioned this pull request Oct 12, 2023

Rename symbols packages #17521

Merged

	<UnifiedSymbolsTarball>$(OutputPath)sourcebuilt-symbols-$(MicrosoftSourceBuildIntermediateInstallerVersion)-$(TargetRid).tar.gz</UnifiedSymbolsTarball>
	<UnifiedSymbolsTarball>$(OutputPath)dotnet-sourcebuilt-symbols-$(MicrosoftSourceBuildIntermediateInstallerVersion)-$(TargetRid).tar.gz</UnifiedSymbolsTarball>

Source-build symbols repackaging #17454

Source-build symbols repackaging #17454

Uh oh!

Conversation

NikolaMilosavljevic commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaelSimons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikolaMilosavljevic commented Oct 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NikolaMilosavljevic commented Oct 3, 2023 •

edited

Loading