Use Portable PDBs and turn on SourceLink #24025
Conversation
|
The corresponding coreCLR pull request is dotnet/coreclr#13964 |
| @@ -163,6 +163,12 @@ | |||
| <BuildingUAPAOTVertical Condition="'$(BuildingUAPAOTVertical)' == '' AND ('$(_bc_TargetGroup)'=='uapaot' OR '$(BuildAllConfigurations)' == 'true')">true</BuildingUAPAOTVertical> | |||
| </PropertyGroup> | |||
|
|
|||
| <!-- Set the kind of PDB to Portable and turn on SourceLink (fetching source from GitHub) --> | |||
| <PropertyGroup> | |||
| <DebugType Condition="'$(DebugType)' == ''">Portable</DebugType> | |||
There was a problem hiding this comment.
Nit: This makes the lines below that set DebugType to full/pdbonly unnecessary.
mellinoe
added
the
* NO MERGE *
|
Previously, our Release builds were using "pdbonly" rather than "full" DebugType. Is there any such distinction for portable PDB's? |
Does code coverage still work? |
|
presumably you will want to do this for Corelib as well? Do all VS features that currently work with Windows PDB's on .NET Core (eg., profiling) work as well with portable PDB's? |
|
Unfortunately this does break code coverage in corefx, e.g.
ends up failing to produce any output with: and: |
|
Can we keep the |
Currently it's expected to work on a regular build, and most folks that do coverage locally don't do a full "coverage" build, but rather just do a normal build, and then just add the /p:Coverage=true flag when running tests. But I'd be fine with a solution that allowed us to specify a flag to the build when building a src assembly to say "use a legacy PDB", either via a coverage-specific flag or via a pdb-specific flag. We'd need to update CI's coverage run to use that flag when building all assemblies, and individual devs can just specify that flag when building an individual assembly before doing coverage tests of that assembly... we'd just want to update the docs as well. Whatever we do, it'd be good to include that as part of this change. |
|
And I guess we have such a flag with DebugType, assuming specifying it on the command-line overrides the build system. I'll try... |
|
Yeah, setting the DebugType at the command-line works, e.g. so we just need to update our docs and the targets/scripts used for the coverage runs to use the flag. |
|
Adding |
|
Or we can run the PDB converter tool before invoking the coverage tool. It would make it work on regular builds. |
I'm not sure that'd significantly improve things. The Coverage prop is only relevant on the test build/target, which doesn't rebuild any src libs, so it wouldn't actually help in the case I outlined (though maybe other folks do things differently and it would help them)... I'd still need to rebuild the src lib, I'd just type /t:rebuild /p:Coverage=true instead of /t:rebuild /p:DebugType=pdbonly.
We could. Honestly I think it's fine to say the libs being tested/covered need to have been built with /p:DebugType=pdbonly. But if we can come up with something better, cool. |
https://github.com/dotnet/core-eng/issues/1132 tracks putting the converted Windows PDBs in the symbol packages, and the best way I see to do that is convert during the build. It sounds like this lines up nicely. |
|
According to OpenCover/opencover#610 portable PDB support exists if you patch it with Mono.Cecil 0.10. However the maintainer of OpenCover is waiting for 0.10 to be marked stable before he updates his dependency. I pinged @jbevain to ask when this is likely to happen. |
|
@vancem what is status and plan for this PR? |
|
@karelz - I had been preempted for a while but I have gotten back to this in the past day or so. I was trying an option where we did not switch to portable PDBs (we upgraded the C# compiler so it is now possible to use SourceLink with MS-PDBs). Unfortunately. This works for coreCLR but CoreFX uses ILLink and it is broken, and like OpenCover it needs updated Mono.Cecil to work. After some thought, I am leaning toward sticking with plan A (moving to portable). I am currently determining the fixes need not to break Coverage builds. Progress is being made... |
vancem
force-pushed
the
PortablePdbsAndSourceServer
branch
from
1bb5116 to
134e5f8
Compare
vancem
removed
the
* NO MERGE *
|
Removing NoMerge label. I want to proceed moving over to portable PDBs (and enabling SourceLink) |
|
The windows build shows the AV that @dagood worried about in dotnet/buildtools#1740. The coverage build also failed, but it is not as clear why as it timed out after an exception as thrown (but we don't know which exception). It could be the same issue. I will pursue this... |
This makes us uniform. Allows VSCode (which does not suport MSPdbs) use on Windows.
Also update docs about code coverage.
vancem
force-pushed
the
PortablePdbsAndSourceServer
branch
from
4331453 to
54cfca5
Compare
|
I believe I have fixed this reliability issue dotnet/buildtools#1740 and updating to build-tools 2.0.0-prerelease-02126-01 should bring that fix here. @dotnet-bot test code coverage please |
|
@dotnet-bot help |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
@dotnet-bot test this please |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
Coverage runs work properly. Here is the run above. There is one more update to the build tools that I would like to get in, but things look good. If anyone has a problem with the change to use portable PDBS please speak now. |
|
@dotnet-bot test code coverage please |
When dotnet/corefx#24025 goes through to change coreFX over to use portable PDBs we want to do the same for System.private.Corelib.pdb in CoreCLR. This change does this.
|
@vancem I've seen quite a few cases in build logs where your PDB converter tool is failing to find a component and throwing an exception -- without breaking the build. Is that relevant or a concern? I can find one if needed. |
|
Ah, it may be because GenerateWindowsPdbsForAssemblyBeingTested is running on Unix, which won't ever work. |
|
@danmosemsft
Yes, your bug https://github.com/dotnet/corefx/issues/24807 alerted me to this. As you have figured out. this task was not supposed to run at all except on windows platforms. The fix was easy (only run it on windows). However the fix is in the build-tools nuget package, so it is a two-step process to fix it. It has been fixed in build-tools (see dotnet/buildtools#1764) and in fact this PR will cause CoreFX to use this fixed Nuget package. Thus this PR should fix all those warnings you are getting. Note that that issue is independent of whether portable PDBS are used or not on windows (which is what this change is about). |
|
OK thanks I somehow missed your earlier reply. |
|
OK I have put in the last update to build-tools (that will address Dan's issue). Code coverage works. Here is the report that was generated by the CI system (since I asked dotnetbot for code coverage. I would like to check this in later today (along with dotnet/coreclr#14721 which makes coreclr generate portable pdbs). Any last worries? (as mentioned ASP.NET has been on the portable PDB plan for a while now, so we know that the 'basics' have to work). Note that this moves us in the direction we want, but also unblocks turning on SourceLink, which allows Visual Studio users to fetch the correct source code straight from the GitHub (this is what has been driving me to put in the effort). |
|
No concerns from me |
|
Issue dotnet/coreclr#14721 turns on portable PDBs for coreCLR |
When dotnet/corefx#24025 goes through to change coreFX over to use portable PDBs we want to do the same for System.private.Corelib.pdb in CoreCLR. This change does this.
|
This breaks ILSpy also -- fix is to rename the PDB temporarily. Taht needs updated Mono.Cecil also. JB says maybe end of the week. |
|
i am getting the exact error stephentoub mentioned while running .net 4.5,VS 2015,OpenCover.4.6.519, windows 10 and msbuild .All items shows zero as mentioned in the image above Committing... |
…rceServer Use Portable PDBs and turn on SourceLink Commit migrated from dotnet/corefx@968cfcf
This pull request, as well as its companion in the CoreClr repository are tiny (they basically set an MSBUILD variable), but the result is that we generate Portable PDBs rather than MS-PDBs during the build).
The immediate value of this change is that we can turn on SourceLink support which allows Visual Studio to 'just work' and step into source it fetches from this Github repository. This is an important step in getting this working for our shipped builds.
It is possible to get SourceLink working without generating Portable PDBs but I believe we want to move in this direction sooner rather than later because.
In short, uniformity is good, and this makes us significantly more uniform. The only issue is what 'breaks'. Because of (4), it is not clear that any important scenario is actually broken by the change. To the degree that we publish PDBs via the MS symbol server, those were converted before publication so things will work as the do now. For private builds, VS works with portable PDBs. Tests should be using portable (so they can run on Linux too). Now I am sure that we might break something, but we are likely to be close enough that it is easier/better to simply try it and clean it up (or revert it), if we run into issues.
But I have marked this NO-MERGE for the time being so that we can vet any concerns. If after several days of vetting we have addressed all concerns we would be in a position to proceed.
Please include anyone you believe may be impacted by this.
@dagood @mikem8361 @ericstj @jkotas @lt72 @Petermarcu @markwilkie @weshaggard @tmat @Eilon @karelz @danmosemsft