I downloaded latest bits from core-setup -- these have a hash of 281aba0f2cfcf9e8198145cbda73d8fc9dc2c45e within System.Runtime.dll which is from Wed Dec 5 01:08:52 2018 +0100 and would include my fix 24db8490eb6bdeeb2c4353d5b7f9b786546cfa2b from Nov 28. I got matching symbols both in the VS debugger and also downloading the symbols package.
Apparently https://github.com/dotnet/corefx/pull/33721 did not fix it. There are two problems
1) Some pdb's still have the internal path eg
c:\bak\sym>\t\strings Microsoft.CSharp.pdb | find /i "git"
{"documents":{"E:\\A\\_work\\7\\s\\corefx\\*":"https://devdiv.visualstudio.com/DevDiv/_apis/git/repositories/DotNet-CoreFx-Trusted/items?api-version=1.0&versionType=commit&version=281aba0f2cfcf9e8198145cbda73d8fc9dc2c45e&path=/*"}}
2) Some pdb's do not have sourcelink at all - I did not notice this before. Perhaps CCI is removing the sourcelink. Example: System.Runtime.pdb. It may be the ones where CCI has to inject type forwards.
There are presumably different causes and it may be that (2) has existed for some time. (1) is the priority.
danmosemsft
All 29 comments
Starting with (1), @tmat can you suggest which properties to check? I have looked at build logs, etc and it is not clear to me how I get it to rewrite the URL properly, as it does in other repos.
danmosemsft
on 12 Dec 2018
@danmosemsft This doesn't seem like it's produced by build that's on Arcade, since that would be built from dnceng repo, not DotNet-CoreFx-Trusted.
tmat
on 12 Dec 2018
@tmat ah - you're right, we are on the verge of converting CoreFX to AzureDevOps builds. Let's give it a few days and see whether that magically fixes it.
danmosemsft
on 12 Dec 2018
We can meanwhile focus on 2)?
ViktorHofer
on 12 Dec 2018
It is likely that CCI does not understand SourceLink and erases it.
tmat
on 12 Dec 2018
@tmat when CCI rewrites the assembly to use type forwards, does it invalidate the PDB? If so, does it attempt to update the PDB at all?
If it leaves the PDB invalid, then maybe source link info wouldn't be much use anyway. And we just have a broken debug experience when debugging code in an assembly that CCI has messed with.
Which repo is CCI in?
danmosemsft
on 12 Dec 2018
Ah I see https://github.com/Microsoft/cci. If GenFacades used S.R.Metadata instead, would that do it correctly? CCI looks deprecated,
cc @ericstj
danmosemsft
on 12 Dec 2018
Yes, CCI definitely is deprecated, or rather has actually never been supported.
Anyways, I'd suggest to write out .cs files and run them thru csc.exe. That's much easier than write metadata directly.
tmat
on 12 Dec 2018
S.R.M doesn鈥檛 really round trip, it鈥檚 missing writing functionality.
@tmat鈥檚 suggestion to replace GenFacade with source generation is actually something on the backlog. @weshaggard and I were discussing how we could do this while still not checking in the source. We could do an early call to CSC to see what types are missing (maybe with /refonly or analyzer) then gen forwards in intermediate before the real compile.
Alternatively if we are willing to check in the forwards we could make it a manual target that runs on demand (eg: apicompat fails, dev runs target).
ericstj
on 12 Dec 2018
@jaredpar :p
danmosemsft
on 12 Dec 2018
As an FYI, NuGet Package Explorer will parse and display sourcelink information in pdb's and dll's with embedded pdb's. Just double-click the file inside a nuget package.
That can be helpful for seeing the mappings.
clairernovotny
on 14 Dec 2018
To clarify, my recollection is that S.R.M can't roundtrip IL but that info might be old. @tmat or @nguerrera are better to say for sure.
Another thing that could break the source-link info is the ILLink-er. That uses Cecil to rewrite the IL. I would imagine we'd want to fix that if it had a problem.
@onovotny that's good to know. In this case I think it'd be better to examine the PDBs in intermediates as they makes their way through the build.
@danmosemsft did you root cause any of this yet or are you just suggesting theories?
ericstj
on 14 Dec 2018
As a hack, you can create a new blank package in NPE and drag the intermediate files to it, then it鈥檒l let you see the data. A side effect as it鈥檚 not directly a pdb viewer, but I don鈥檛 know of an easier way 馃檪
clairernovotny
on 14 Dec 2018
S.R.M supports writing, roslyn writes with it. :)
What it doesn't have (by design) is a high level "DOM" so that you can easily read/edit/write. So it is not a direct replacement for something like Cecil or CCI, which do offer that. Rather, it is the low level piece that you'd build higher models over.
nguerrera
on 14 Dec 2018
I agree with generating .cs for type forwards.
nguerrera
on 14 Dec 2018
For (1), it looks like the move to Azure DevOps may have fixed this. I pulled down Microsoft.CSharp.pdb for Linux and Windows out of the first passing build and both have source link info in, both by grep and expanded in Package Explorer (nice feature @onovotny). I'll not consider this fixed though until we have an install out of core-setup that I can try in the debugger.
For (2), have not investigated.
danmosemsft
on 20 Dec 2018
cc @safern fyi
danmosemsft
on 20 Dec 2018
If @danmosemsft confirms that 1) is fixed, perhaps we should then close this as a dupe of https://github.com/dotnet/buildtools/issues/705 since the fix would be external? (by the fix, I mean changing GenFacades to write .cs files with the forwarders instead of using CCI to rewrite the assembly.)
joperezr
on 4 Jan 2019
I downloaded the SDK for Windows from https://github.com/dotnet/core-setup/ for 3.0.0-preview-27304-2 and stepped into hello world and confirmed it pulled down sources for both corelib and (eg) System.Diagnostics.Process.dll.
I also pulled down the tar.gz of symbols for Mac and Linux x64 and confirmed that the github link was present in the pdb's of both the above.
So (1) is fixed. I'll close this in favor of dotnet/buildtools#705 for (2)
danmosemsft
on 4 Jan 2019
Actually tracked by https://github.com/dotnet/arcade/issues/1584
danmosemsft
on 4 Jan 2019
Reopening to track https://github.com/dotnet/arcade/issues/1584 with a milestone set and assigned to a project.
ViktorHofer
on 16 Jan 2019
We should use this to make sure that once https://github.com/dotnet/arcade/issues/1584 is fixed that all shipping implementation assemblies have matching symbols with source-link enabled. Maybe it should even add validation to our build to ensure that's true. For example here: https://github.com/dotnet/corefx/blob/master/pkg/frameworkPackage.targets#L109
ericstj
on 16 Jan 2019
As we are not the only ones that ship assemblies with source link support, would it makes to streamline such a validation and move it to arcade instead? I believe such a validation would be nice but definitely not high-prio in comparison to other things that are on the plate right now.
ViktorHofer
on 16 Jan 2019
Validation in arcade is reasonable but I don鈥檛 think we should resolve this until we have a comprehensive (possibly manual) check to ensure the issue is fixed. I wonder if there are other cases missing source link that we might care about... (ilproj, vbproj, redistributed assemblies)
ericstj
on 16 Jan 2019
Arcade issue to validate: https://github.com/dotnet/arcade/issues/1873
danmosemsft
on 25 Jan 2019
We (semi-)committed to fixing this for 3.0.
danmosemsft
on 20 Feb 2019
danmosemsft
on 5 Mar 2019
This is in progress. Please make sure to check for sourcelink info in official builds once the genfacades work is done.
ericstj
on 27 Mar 2019
I verified sourcelink with the latest sdk pdbs(preview 5) and it is working fine
Anipik
on 12 Apr 2019
Related issues
noahfalk
路
3Comments
omajid
路
3Comments
jzabroski
路
3Comments
iCodeWebApps
路
3Comments
btecu
路
3Comments