Skip to content
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

Merged
merged 3 commits into from Jun 14, 2023

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jun 13, 2023

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.

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.
@ghost
Copy link

ghost commented Jun 13, 2023

Tagging subscribers to this area: @dotnet/area-dependencymodel
See info in area-owners.md if you want to be subscribed.

Issue Details

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.

Author: eerhardt
Assignees: eerhardt
Labels:

area-DependencyModel

Milestone: -

@@ -42,7 +40,13 @@ private static DependencyContextPaths GetCurrent()

internal static DependencyContextPaths Create(string? depsFiles, string? sharedRuntime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be private?

Copy link
Member Author

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).

Copy link
Member

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 <stoub@microsoft.com>
@eerhardt eerhardt merged commit 8b7c300 into dotnet:main Jun 14, 2023
99 of 105 checks passed
@eerhardt eerhardt deleted the FixDependencyContext branch June 14, 2023 02:47
@@ -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()
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing in #87666.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants