Fix System.Diagnostics.StackTrace version in stack trace helper code #7383
Conversation
|
@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please |
| { | ||
| s_symbolsType = Type.GetType( | ||
| "System.Diagnostics.StackTraceSymbols, System.Diagnostics.StackTrace, Version=4.0.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", | ||
| "System.Diagnostics.StackTraceSymbols, System.Diagnostics.StackTrace, Version=4.0.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", |
There was a problem hiding this comment.
Is this the right change? I know you are fixing the version to be the one that Roslyn brought in but is using the version of a Roslyn dependency the right thing to do?
There was a problem hiding this comment.
Typically we want to probe for the lowest version and let the loader pick a newer version if that's what is available. That's what will happen here, right @gkhanna79?
There was a problem hiding this comment.
@ericstj I am trying to understand what is the right policy/approach here and not dictate one :) Is S.D.ST expected to come only with Roslyn? If so, then this looks like the right approach (assuming we want to keep the lowest version). Otherwise, this needs to be thought through more.
There was a problem hiding this comment.
As long as Roslyn isn't removed from the shared framework (though I've heard it has been discussed), it shouldn't matter. With this change coreclr doesn't depend on the version of S.D.ST because even if Roslyn references a newer version, the coreclr support will still work.
/cc: @weshaggard
There was a problem hiding this comment.
I agree with @ericstj that we should always try to load the lowest version of the assembly that actually contains this type. As for whether or not it is in the shared framework or not that is a different question, and the code here seems to correctly no-op if the assembly isn't found.
|
The shared framework/Microsoft.NETCore.App package doesn’t explicitly reference System.Diagnostics.StackTrace. It only gets pulled in because of Roslyn. I’m not really sure what other version it should be if not this one (4.0.1). FYI, the reason this bug has made it this far is that my manual testing is flawed and there are no StackTrace related tests (which includes unhandled exception stack traces). It has been on my list but will become my number one priority. mikem |
|
Yes, that is right and 4.0.1 does seem to be the lowest version so it should always succeeded if S.D.ST exists at all. |
|
LGTM |
|
Yes, 4.0.1 is the first version. |
Issue #7381