Enable PdbGit (GitLink) on our PDB files #15256

jaredpar · 2016-11-15T18:19:57Z

No description provided.

jaredpar · 2016-11-15T18:20:06Z

CC @Pilchie, @dotnet/roslyn-infrastructure

jasonmalinowski · 2016-11-15T18:32:35Z

src/Tools/MicroBuild/run-pdbgit.ps1

+    $packagesPath = Get-PackagesPath
+    $pdbGit = join-path $packagesPath "pdbgit\$pdbGitVersion\tools\PdbGit.exe"
+
+    $signToolDataPath = join-path $sourcePath "build\config\SignToolData.json"


Could a comment be added to explain why we're involving signing configuration here? I have guesses but I'd rather not be guessing and just know.

Also wondering if "SignToolData" is now a poor name if it's being used for more things too.

jasonmalinowski · 2016-11-15T18:35:00Z

src/Tools/MicroBuild/run-pdbgit.ps1

+        $binaryPath = join-Path $binariesPath $relativePath
+        $pdbPath = [IO.Path]::ChangeExtension($binaryPath, ".pdb")
+        if (-not (test-path $pdbPath)) {
+            continue;


Why not fail instead? This means there's bad data.

There are two cases where it's not an error:

The binary is a VSIX

The PDB is a portable PDB

The former is really the only concern for our repo right now though. Will account for VSIX and error on other cases.

jasonmalinowski · 2016-11-15T18:36:22Z

src/Tools/MicroBuild/run-pdbgit.ps1

+        }
+
+        write-host "Running pdbgit on $pdbPath"
+        & $pdbGit $pdbPath -u https://github.com/dotnet/roslyn --baseDir $sourcePath 


What is this URL supposed to be? Is it the git repository (which I would expect should have a .git to be more canonical) or does it have some smarts here?

(I know GitHub currently supports clones without the .git, but unless you can get a contract with them saying they'll never break it...)

It needs to be a recognizable name for the git repo. The tool doesn't actually use this name directly but instead uses the name to determine the repo involved. It writes a different URL to the PDB. Generally in the form of https://raw.github.com/repo/project/<commit>/...

I based this argument off of the sample on the PdbGit README file.

PdbGit.exe path-to-your.pdb -u https://github.com/username/project --baseDir C:\root-of-your-project\ --commit your-commit-ID

We need to be explicit here because in the case of mulitple remotes the tool will pick one of them. When I ran locally it was actually embedding urls to my personal fork vs. the official once hence I made it an explicit argument.

OK, makes sense. I didn't know where the README was to check.

Explicitly specifying the URL to avoid forks sounds good. But you should only need the --baseDir parameter if there is no .git folder. I think you can remove that from your script here.

jaredpar · 2016-11-15T18:58:07Z

@jasonmalinowski responded to PR feedback

jasonmalinowski

Thanks for the updates. Good, but yes, we need to do unit test binaries too. Otherwise I promise to send @CyrusNajmabadi in your direction next time he has to debug a unit test crash. :-)

jasonmalinowski · 2016-11-15T19:00:03Z

src/Tools/MicroBuild/run-pdbgit.ps1

+    $pdbGit = join-path $packagesPath "pdbgit\$pdbGitVersion\tools\PdbGit.exe"
+
+    # Linking our PDBs to Git is really only interesting for PE / PDB that are 
+    # being shipped to customers.  For local developement the original PDB 


What about unit tests though? Investigating dumps of crashing unit tests is common and you'd want that too.

@jasonmalinowski can you elaborate on the scenario here? Essentially when do we get a crash dump for a unit test that we need to investigate?

When a unit test fails in Jenkins or the signed build. That's how we figured out the flaky access violation issue just last week.

...which I now realize means we need to run this in Jenkins too.

I think you need to consider the cost of this tool. PdbGit is not a fast tool, instead it's comparitevyl slow with other tools used during build. The performance seems to scale with the size of the binary involved. This means adding this to our unit test runs will add quite a bit of time to our unit tests runs.

To give a sense of how much consider that we are linking ~60 PDBs and it takes 15 minutes to do so. If we implement source linking perfectly on our source tree that will be ~200 PDBs and hence easily adding about 1 hour to our unit test time.

By perfectly I mean that we link a PDB exactly once. Given our build setup PDBs will appear multiple times in the output tree (due to build output copying). After the first link we need to copy the PDB vs. linking again as copying is faster. That's certainly doable but takes a bit more effort.

I'm currently digging into the performance a bit to see if there are any wins we could get. At this time though I don't think it's viable for unit test legs.

Wow. 15 minutes for 60 PDBs is awful. When I wrote (some of) the PdbGit code, I remember that there was non-performant code I was writing that could be improved if necessary (the whole "optimize after you measure" philosophy). So given how bad it is, can you please send an ETL trace of the tool running (perhaps the command line invocation on just one of your larger PDBs)? I bet we can fix it significantly (perhaps an order of magnitude).

Cool. Then we might even be able to use the srcsrv support in the PDB to self-extract the source. :)
I'll mention you on an issue where I'll track the work to ask you more questions. :)

hey, whatever the new thing is, I'm game. Just point me at the docs. :)

Do you mean including a command in the PDB to extract the source? Let's not do that, that's a security hole right there.

The new thing is Portable PDBs.

Note that embedding to windows PDBs via the compiler was pushed out to Update 1: #13707

The compiler supports embedding source in portable PDBs only right now.

Please tag me as well.

Pilchie

Thank you!

tmat · 2016-11-15T21:15:14Z

src/Tools/MicroBuild/run-pdbgit.ps1

@@ -40,6 +40,14 @@ try
    $count = 0

    foreach ($relativePath in $signToolData.sign | %{ $_.values }) {
+
+        # This is a DLL we sign but do not produce.  Hence it doesn't have a PDB
+        # to be source linked.


Why are we signing it?

@tmat because we were signing it 😄

Basically when I converted to SignUtil I started by doing a diagnostic run on our signed builds. That spit out the list of assemblies which were being signed. I assumed that was correct and replicated the list in the sign tool data.

If that's wrong I'm happy to remove.

I think it's wrong. We are not building it in the repo so we shouldn't be signing it. It's signed by the build queue of symreader-portable repo.

AArnott · 2016-11-21T05:33:59Z

This PR seems related to #3. Not to close it, but it satisfies some of the requirements presented there.

jaredpar · 2016-11-29T16:34:11Z

Closing as after some discussion we are switching to using GitLink.

AArnott · 2016-11-30T15:28:58Z

@jaredpar: can you share those discussions? I'm very curious what your reasoning was and what approach you will take (e.g. msbuild integration, running gitlink.exe passing in a solution file, etc.)

Enable PdbGit (GitLink) on our PDB files

f0ce100

jaredpar added the Area-Infrastructure label Nov 15, 2016

dnfclas added the cla-already-signed label Nov 15, 2016

jasonmalinowski reviewed Nov 15, 2016

View reviewed changes

Respond to PR feedback

def0b88

jasonmalinowski approved these changes Nov 15, 2016

View reviewed changes

Fixed a props issue

c057aea

Pilchie approved these changes Nov 15, 2016

View reviewed changes

Fix Microsoft.DiaSymReader.PortablePdb.dll

122cd7d

tmat reviewed Nov 15, 2016

View reviewed changes

Fix a path

8dc7de9

nguerrera mentioned this pull request Nov 21, 2016

Embed source files that do not belong to the git repo GitTools/GitLink#115

Open

jaredpar closed this Nov 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable PdbGit (GitLink) on our PDB files #15256

Enable PdbGit (GitLink) on our PDB files #15256

jaredpar commented Nov 15, 2016

jaredpar commented Nov 15, 2016

jasonmalinowski Nov 15, 2016

jasonmalinowski Nov 15, 2016

jasonmalinowski Nov 15, 2016

jaredpar Nov 15, 2016

jasonmalinowski Nov 15, 2016

jaredpar Nov 15, 2016

jasonmalinowski Nov 15, 2016

AArnott Nov 18, 2016

jaredpar commented Nov 15, 2016

jasonmalinowski left a comment

jasonmalinowski Nov 15, 2016

jaredpar Nov 15, 2016

jasonmalinowski Nov 15, 2016

jaredpar Nov 15, 2016

AArnott Nov 18, 2016

AArnott Nov 18, 2016

AArnott Nov 18, 2016

tmat Nov 18, 2016

tmat Nov 18, 2016

nguerrera Nov 18, 2016

Pilchie left a comment

tmat Nov 15, 2016

jaredpar Nov 15, 2016

tmat Nov 15, 2016

AArnott commented Nov 21, 2016

jaredpar commented Nov 29, 2016

AArnott commented Nov 30, 2016

Enable PdbGit (GitLink) on our PDB files #15256

Enable PdbGit (GitLink) on our PDB files #15256

Conversation

jaredpar commented Nov 15, 2016

jaredpar commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Nov 15, 2016

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AArnott commented Nov 21, 2016

jaredpar commented Nov 29, 2016

AArnott commented Nov 30, 2016