Documents node in embedded PDB different in .NET Core 3 #1
Comments
|
@daveaglick thanks for digging into this and pointing out the interesting changes that might be important here. I'm gonna let this sit for a few days and let others chime in about ways to address this. |
daveaglick
changed the title
|
Looks like a change in the compiler. I can pass the test using .NET Core SDK diff --git a/dotnetcore-sourcelink-test-bug.csproj b/dotnetcore-sourcelink-test-bug.csproj
index f4f552a..5634149 100644
--- a/dotnetcore-sourcelink-test-bug.csproj
+++ b/dotnetcore-sourcelink-test-bug.csproj
@@ -7,6 +7,10 @@
</PropertyGroup>
<ItemGroup>
+ <PackageReference Include="Microsoft.Net.Compilers" Version="3.3.0">
+ <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+ <PrivateAssets>all</PrivateAssets>
+ </PackageReference>
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All"/>
</ItemGroup>
All that does is force using an older C# compiler. Maybe the new one has different behavior with respect to generated files? |
Can confirm that. I was able to get it work with your patch on 3.1.1. |
|
@daveaglick related to untracked sources: there is an option to embed untracked sources in the pdb as described at https://github.com/dotnet/sourcelink/blob/master/docs/README.md#embeduntrackedsources That will allow the debugger to also debug "into" sources that are not in version control (e.g. build time generated code). |
|
@rainersigwald @poke is there a way to validate that for non-Windows usage? I tried the same but it errors on macOS and it fails to build while mentioning |
|
@japj Cool, TIL Confirmed that setting Also doesn't address the underlying problem of compiler/sourcelink compatibility regarding compiler generated files and embedded PDBs, but at least gets closer to a successful |
|
@shiftkey Could you try the 3.3.0 compiler version? 3.4.0 also didn’t work for me on Windows due to some MSBuild version. |
|
@poke same issue on macOS |
|
@poke @rainersigwald nevermind, I should have spotted this clue from NuGet:
I'll dust off my Windows VM |
|
Paging @tmat |
|
Can you share the binaries? |
|
@tmat the binary is generated from the csproj file in the repository root You should be able to run |
|
I tested this on Windows with .NET Core SDK |
|
For non-Windows use this package instead diff --git a/dotnetcore-sourcelink-test-bug.csproj b/dotnetcore-sourcelink-test-bug.csproj
index f4f552a..edaa520 100644
--- a/dotnetcore-sourcelink-test-bug.csproj
+++ b/dotnetcore-sourcelink-test-bug.csproj
@@ -7,6 +7,10 @@
</PropertyGroup>
<ItemGroup>
+ <PackageReference Include="Microsoft.NETCore.Compilers" Version="3.3.0">
+ <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+ <PrivateAssets>all</PrivateAssets>
+ </PackageReference>
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All"/>
</ItemGroup>SDK 3.0.100 shipped with Roslyn 3.3.0 (coordinated with VS 16.3), and SDK 3.1.100 shipped with Roslyn 3.4.0 (coordinated with VS 16.4). |
|
Note: you shouldn't downgrade the compilers for production use. I'm suggesting it here to narrow down the problem. |
|
@rainersigwald all good, I walked through the published versions and NuGet and tested whether they passed:
That seems to narrow the range of changes to somewhere in here:
Comparison Link (sadly too big for GitHub to render 😢 ) - dotnet/roslyn@154af84...b8a5611 |
|
I setup a script to building Roslyn from source and apply it to the repro, and bisected the resulting commits above and stumbled upon this commit that seems to introduce the problem - This was merged as part of dotnet/roslyn#39136 which fixed dotnet/roslyn#38954. I'm not familiar with the "sequence points" feature mentioned above, but I'd guess that the impact to Sourcelink is an unexpected side-effect. |
|
@shiftkey Thanks for root-causing the issue. It's now immediately obvious to me what's going on. First of all, the root cause of the issue is dotnet/msbuild#1479 and a workaround is mentioned in the description. dotnet/roslyn#39136 works as designed. Prior to this change the compiler did not include source file entries in the document table of the PDB that did not contain any method bodies. The PR changed the compiler to always include all source files passed to the compiler. There was no good reason for the compiler to behave the way it did before, it was pretty much a result of internal implementation detail leaking through. We have found this behavior problematic in several ways as it prevented us from making other features reliable. Unfortunately, we did not anticipate dotnet/roslyn#39136 exacerbating the effect of dotnet/msbuild#1479. |
|
Fix for dotnet/msbuild#1479: dotnet/msbuild#5101 |
|
Another thing - even when dotnet/msbuild#5101 is fixed you'll need to set |
|
@tmat thanks for the tip about |
|
@shiftkey btw as a workaround you can set the property Tomas is setting in your own codebase (Directory.Build.props). You don't have to wait for the MSBuild fix to go in. |
|
@KirillOsenkov the |
|
no, you also need this: |
|
this should be visible to all projects, one way to do it is have a Directory.Build.props at the root of your repo |
|
@KirillOsenkov is that the entire fix? I tested the change in the project here: diff --git a/dotnetcore-sourcelink-test-bug.csproj b/dotnetcore-sourcelink-test-bug.csproj
index f4f552a..5752d9c 100644
--- a/dotnetcore-sourcelink-test-bug.csproj
+++ b/dotnetcore-sourcelink-test-bug.csproj
@@ -4,6 +4,8 @@
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<RootNamespace>dotnetcore_sourcelink_test_bug</RootNamespace>
<DebugType>embedded</DebugType>
+ <EmbedUntrackedSources>true</EmbedUntrackedSources>
+ <TargetFrameworkMonikerAssemblyAttributesPath>$([System.IO.Path]::Combine('$(IntermediateOutputPath)','$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)'))</TargetFrameworkMonikerAssemblyAttributesPath>
</PropertyGroup>
<ItemGroup>And while the behaviour changes, I now see two failing files: I think I also need diff --git a/dotnetcore-sourcelink-test-bug.csproj b/dotnetcore-sourcelink-test-bug.csproj
index f4f552a..8b71d31 100644
--- a/dotnetcore-sourcelink-test-bug.csproj
+++ b/dotnetcore-sourcelink-test-bug.csproj
@@ -4,6 +4,9 @@
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<RootNamespace>dotnetcore_sourcelink_test_bug</RootNamespace>
<DebugType>embedded</DebugType>
+ <EmbedAllSources>true</EmbedAllSources>
+ <EmbedUntrackedSources>true</EmbedUntrackedSources>
+ <TargetFrameworkMonikerAssemblyAttributesPath>$([System.IO.Path]::Combine('$(IntermediateOutputPath)','$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)'))</TargetFrameworkMonikerAssemblyAttributesPath>
</PropertyGroup>
<ItemGroup>And we have success: Does that sound right? |
|
@KirillOsenkov looks like only |
|
@shiftkey |
|
Setting So I think the best would be to set it in You can see from the |
|
@tmat this project pre-dates .NET Core, so it hasn't really caught up to these sorts of habits. I'll try adding one in and see how that goes. |
|
@tmat @KirillOsenkov I realise I've been told differing advice about which file to add with these settings:
I ended up going with the |
|
I think msbuild might not be importing the file. Since this is on Linux casing matters and it needs to be |
|
No change: |
|
Oh, if you're not using the SDK-style project format the Directory.Build.props/targets won't get picked up. Unfortunately in that case you'll need to resort to manually setting the properties directly in the .csproj file (at the very end, after all imports). And Tomas was right earlier, these properties should go in Directory.Build.targets file, since it gets imported almost at the end. Directory.Build.props is being imported very early on (if you're using an SDK-style project). Try using a binlog and http://msbuildlog.com to see if the properties are being set correctly for your project. |
|
I've been following this as well, and it's not clear to me why the With my own minimal project to repro the issue (https://github.com/bording/SourceLinkProblem) The problem is that despite having This is the same thing that you can see from @shiftkey's output in #1 (comment) It doesn't seem like dotnet/msbuild#5101 fixes this problem. |
|
There is another issue -
This means Source Link won't see the file. The workaround is to specify it manually in |
|
Fix: dotnet/sdk#10613 |
|
@tmat I can confirm that the
|
…uild.targets" Per following suggestion by @tmat > Setting `TargetFrameworkMonikerAssemblyAttributesPath` needs to be > done after importing SDK targets because `IntermediateOutputPath` is > set in `Microsoft.Common.CurrentVersion.targets` and not before that. > > So I think the best would be to set it in `Directory.Build.targets` > in the repo root. Source: shiftkey/dotnetcore-sourcelink-test-bug#1 (comment)
This applies the following suggestion from @tmat: shiftkey/dotnetcore-sourcelink-test-bug#1 (comment)
|
Note that this line may also be necessary: |
This intrigued me and I've been meaning to dig into how SourceLink works anyway, so here we are. I think I know what's going on...or at least why it's failing. I just don't know how to fix it.
For starters, here's the embedded PDB metadata of a build on .NET Core SDK 2.2.401:
And here's the embedded PDB metadata of a build on .NET Core SDK 3.1.101:
There's so much more! But most importantly, check out that "Documents" node. It doesn't even exist in the .NET Core 2.2 build.
This can be verified by the sourcelink tool - here it is listing documents for the 2.2.401 build:
Well that was anticlimactic. Here it is for the 3.1.101 build:
I'm pretty sure that confirms the sourcelink tool is using the Documents node in the PDB. Just to be sure, I peeped the sourcelink tool code (which isn't in the DNF repo, but in a repo from @ctaggart directly):
Here's the actual code for the
testcommand: https://github.com/ctaggart/SourceLink/blob/be8dcdaad09a2891bc0e1d04e43af40a1e482158/dotnet-sourcelink/Program.cs#L203If we go back and look more closely at the output from
testwhen it fails:We can see two errors which make sense to me now because we can track them back to entries in the PDB metadata as document nodes:
TL;DR:
I'm left with the following questions:
AssemblyAttributes.cs,AssemblyInfo.cs, and other generated source files?The text was updated successfully, but these errors were encountered: