-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix DependencyContext splitting on semi-colon #87518
Conversation
dotnet@5e67657 introduced a bug in DependencyContextPaths where the static array is not initialized before it is being used in the Create static method. This fix reorders the fields so that s_semicolon is initialized before it is first used.
Tagging subscribers to this area: @dotnet/area-dependencymodel Issue Details5e67657 introduced a bug in DependencyContextPaths where the static array is not initialized before it is being used in the Create static method. This fix reorders the fields so that s_semicolon is initialized before it is first used.
|
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextPaths.cs
Outdated
Show resolved
Hide resolved
- Skip test on netfx since it doesn't work.
src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextPaths.cs
Outdated
Show resolved
Hide resolved
@@ -42,7 +40,13 @@ private static DependencyContextPaths GetCurrent() | |||
|
|||
internal static DependencyContextPaths Create(string? depsFiles, string? sharedRuntime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, unit tests use it through IVT. They could be refactored to use reflection instead, but I'm trying to get this through to unblock code flow - see dotnet/installer#16621 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh IVT, how you mock me.
Co-authored-by: Stephen Toub <[email protected]>
Build time outs are tracked by #76454. System.Collections.Tests failing on WASI appear to be: |
@@ -276,6 +276,19 @@ public void MergeMergesRuntimeGraph() | |||
Subject.Fallbacks.Should().BeEquivalentTo("win7-x64", "win7-x86"); | |||
} | |||
|
|||
[Fact] | |||
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "GetEntryAssembly() returns null")] | |||
public void DefaultWorksCorrectly() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eerhardt this also fails on NativeAOT on runtime-extra-platforms:
Assert.NotNull() Failure
Stack trace
at Microsoft.Extensions.DependencyModel.Tests.DependencyContextTests.DefaultWorksCorrectly() + 0x76
at Microsoft.Extensions.DependencyModel.Tests!<BaseAddress>+0x1292ea4
at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0xe6
GetEntryAssembly()
does work on NativeAOT though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing in #87666.
5e67657 introduced a bug in DependencyContextPaths where the static array is not initialized before it is being used in the Create static method.
This fix removes the static array since it is only used once.