Bring back PortablePdbReader - #242
Conversation
| <PropertyGroup> | ||
| <TargetFrameworks>netcoreapp2.0;net461</TargetFrameworks> | ||
| <TargetFrameworks Condition=" '$(OS)' != 'Windows_NT' ">netcoreapp2.0</TargetFrameworks> | ||
| <DebugType>portable</DebugType> |
There was a problem hiding this comment.
The SDK generates a full pdb for cross-compiled applications (on Windows?). CoreCLR can't seem to read a full pdb and our pdb reader only works one way and we intend to keep it so. Forcing it to portable since that's what most users would have.
There was a problem hiding this comment.
It's xunit shenanigans (https://github.com/xunit/xunit/blob/master/src/xunit.core/build/xunit.core.props#L14) and not the SDK. Maybe a bug in it. Either way, forcing this to portable ensures we get the stack trace correctly both on desktop and coreclr. In the absence of this, coreclr is missing the stacktrace for the test file.
| using System.Reflection.Metadata.Ecma335; | ||
| using System.Reflection.PortableExecutable; | ||
|
|
||
| namespace Microsoft.Extensions.StackTrace.Sources |
There was a problem hiding this comment.
Why have the namespace differentiation? Here and for the moved files.
There was a problem hiding this comment.
I guess that's how it was. It'll be a pain to change it without breaking the consumer. I'd prefer leaving it just to create less work :)
There was a problem hiding this comment.
Hu? Shouldn't there be 0 consumers currenlty?
There was a problem hiding this comment.
we use StackTraceHelper from our package in Hosting and Diagnostics and that's in the same namespace. Changing the ns for that type would be the troublesome part.
| <PropertyGroup> | ||
| <TargetFrameworks>netcoreapp2.0;net461</TargetFrameworks> | ||
| <TargetFrameworks Condition=" '$(OS)' != 'Windows_NT' ">netcoreapp2.0</TargetFrameworks> | ||
| <DebugType>portable</DebugType> |
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\ThrowingLibrary\ThrowingLibrary.csproj" /> |
There was a problem hiding this comment.
Regardless of what kind of pdb you generate, exceptions seem preserve stack trace as long as they are produced and caught in the same assembly. I had to cause it to be produced in a different assembly for us to actually use the portable pdb reader. I'll make a note of this in the test.
| Exception exception = null; | ||
| try | ||
| { | ||
| // Throwing an exception in the current assembly always seems to populate the full stack |
There was a problem hiding this comment.
Given the discussion elsewhere in this PR, it would be good to put this same comment in the Thrower class itself.
|
🆙 📅 |
NTaylorMullen
left a comment
NTaylorMullen
left a comment
There was a problem hiding this comment.
All seems fine. Will have more context once I see it used in hosting/diagnostics.
|
@Eilon bump? |
ghost
locked as resolved and limited conversation to collaborators
4 participants
Fixes #235