Publishing v3 doesn't publish converted portable PDBs

[hoyosjs]

• This issue is blocking
• This issue is causing unreasonable pain

Symuploader used to do PDB conversion, then it was handled by Symstore.targets + PDB2PDB in publishing v2. I don't think there's anything doing this conversion right now.

This issue is blocking insertions. All Portable PDBs need to be converted to Windows PDBs and indexed.

This is also a gap in validation - it verifies a file is available, not that all appropriate assets are indexed.

cc: @epananth and @mmitche as this is affecting timely insertions.

[epananth]

In the call to publishSymbolHelper we always default this to false

arcade/src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs

Line 490 in c682530

await PublishSymbolsHelper.PublishAsync(

We will need to make sure that value can be updated by repo owners...

@mmitche not sure if there is more to this change..

[epananth]

One thing I noticed this feature was turned off even in V2

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/SdkTasks/PublishToSymbolServers.proj

Line 101 in c682530

ConvertPortablePdbsToWindowsPdbs="false"

[hoyosjs]

That's because in-repo the conversion was done not by the uploader, but in-build:

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/SymStore.targets

Line 70 in c682530

<Exec Command='"$(_PdbConverterPath)" $(_PdbConverterCommandLineArgs)' IgnoreExitCode="false" />

[hoyosjs]

That design had an issue in the mono world too (we can't really do it at build time) - as we now have components that build not on windows but get inserted into VS, so PDB2PDB never ran. cc: @directhex

[hoyosjs]

So this is really 3 issues:

• SymStore.targets + Publishing v3 don't allow for publishing the Windows PDBs needed on Watson.
• There's no conversion mechanism for VS insertions that are build on non-Windows OSs.
• Symbol validation is brittle - only verifies one item and not all necessary assets.

[mmitche]

@epananth @hoyosjs Can we just flip that switch to always be true? Are there downsides? Publishing v3 always runs on Windows so we have a point at which we can convert all PDBs.

[hoyosjs]

More CPU + time. You'd need to retune the timeouts of publishing for something like runtime

[mmitche]

@epananth Can you run some tests on runtime with the proposed changes?

[epananth]

Yes will do!

[epananth]

I'm seeing this error in promotion build -> https://dev.azure.com/dnceng/internal/_build/results?buildId=1404592&view=results

@alexperovich is helping fix this one..

[epananth]

was able to test it against runtime -> https://dev.azure.com/dnceng/internal/_build/results?buildId=1407043&view=logs&j=ba23343f-f710-5af9-782d-5bd26b102304&t=6e277ba4-1c1e-552d-b96f-db0aeb4be20a

[mmitche]

Looks reasonable to me, time-wise, assuming it did what it should.

[epananth]

I spoke to @hoyosjs he asked me to test a build that is not being published before. So I am running runtime test now.. Will update the results later today..

[epananth]

Got this one working too https://dev.azure.com/dnceng/internal/_build/results?buildId=1407513&view=results

[epananth]

Will get this PR out first thing in the morning

[riarenas]

Reopening since the changes for this had to be reverted.

[riarenas]

Perhaps we need to make the property opt-in instead of opt-out?

[hoyosjs]

Anything that can potentially end up in VS needs it. It's more likely it should be on by default.

[riarenas]

Then I guess we need to make fixes to pdb2pdb so that it doesn't result in sourcelink errors in aspnetcore?

[hoyosjs]

This indirectly uses PDB2PDB - I wonder if we are using a very old version. Can you show me the ASP errors?

[riarenas]

https://dev.azure.com/dnceng/internal/_build/results?buildId=1418496&view=results

[riarenas]

Someone had filed #7388 which is the same error before.

[hoyosjs]

I see - that error is not what I was worried about. The one that I was more concerned about is https://dev.azure.com/dnceng/internal/_build/results?buildId=1417687&view=logs&j=ba23343f-f710-5af9-782d-5bd26b102304&t=6e277ba4-1c1e-552d-b96f-db0aeb4be20a&l=504. This one is basically the DLL and PDB are not co-located, which is a requirement for conversion.

The one you mentioned gets fixed by dotnet/symreader-converter#162, I'll need to check if we need to update the converter.

[epananth]

@hoyosjs any update on this?

[hoyosjs]

Took a while as these were... gnarly to figure out and lacking logging.

From the original report in #8033:

• EFCore has some framework exe's that PDB conversion didn't handle.
• Runtime: On 5.0 there is Microsoft.NETCore.App.Internal which only contains symbols.
• ASP:
  • Microsoft.AspNetCore.Razor.Internal.Transport is a symbol only package... This feels odd. If this is inserted into VS it needs to get converted. The conversion needs the PDBs.
  • Microsoft.Extensions.ApiDescription.Server ships a framework exe.
  • Looks like the map list did not make cshtml paths into cannonical deterministic forms. Looking at it:
    I don't know exactly, but it looks like the cshtml is not getting the path reassigned early enough for it to be embedded. The CI build has no binlog, and locally RazorCoreCompile target invokes CSC task:
    
    
    And the sourcelink output is as expected.

Maybe @tmat might know what to do here as by the time the symbol uploader gets it, it's way too late.

• Installer - false alarm. The issue was a transient error in blob storage when publishing non-symbol packages.

A lot of the symbol packages are missing PDBs, but that's a separate issue.

Additionally a kusto query showed the following issues too:

TimelineIssues
| where Message contains "for portable PDB not found"
| summarize count() by BuildId, RecordId
| join TimelineBuilds on BuildId
| extend Repo=split(BuildNumber, " ")[1]
| join TimelineRecords on BuildId, RecordId
| project BuildId, count_, Repo, LogUri
| summarize by tostring(Repo)

• SDK - For some reason they are indexing and uploading all their tests. Some of them are framework exes.
• Roslyn - Packages had Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator, which is an FX exe.
• Windows SDK - on 5.0 it ships Microsoft.WindowsDesktop.App.5.0.*.symbols.nupkg, which is a symbols only package.
• Diagnostics - ships a symbol only symbol package.

Improvements to symuploader out of this:

• Handle FX exes as possible scenarios.
• Undo overly constraining paths the symuploader had hardcoded. Now it will properly convert all files that might be in a package (needed for VS insertion).
• Log the offending package in this case (I'd like all errors to have some correlation, but that's a bigger work item. For now I made all warning and errors have the package path).

Among all these there's issues on both sides:

• If the symbol conversion fails for any reason, we'll fail. This will happen with the symbol only packages (which are somewhat not common - it's usually the package layout + symbols), and it will happen with snupkg files as they don't have the PE streams. It is not tenable to have warn as error for conversion because of all the adhoc packages and the CSHTML issue. If we get requested to convert, it makes sense to at least warn of the issue. Should we treat these warnings as non-fatal?
• The original patch has issues for PdbArtifacts (aspnet uses it) here:
  https://github.com/dotnet/arcade/pull/8007/files#diff-716e74c0c24a454d2c2b3d08566c5c79d62a037bee929a70e53c3f73b62cae2aR695-R708
  They never seem to include the DLLs which are needed to do conversion. This case shouldn't try to do conversion.
• Conversion is slow. It's actually made the runtime publishing time out EVERY TIME while this was turned on.

[epananth]

After discussion with @mmitche and @hoyosjs we came to a conclusion that

ConvertToPdbs has to be done in staging pipeline instead of arcade.
There are some issues with the current symbol validation we are doing in release pipeline, so need to address that as a part of this change. Will use this issue to address that.

[epananth]

Tracking symbol validation here -> https://github.com/dotnet/core-eng/issues/14755

[epananth]

This has been enabled in release pipeline. So going to close this.