-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Net9 Reference Assemblies #73423
Net9 Reference Assemblies #73423
Conversation
eng/Directory.Packages.props
Outdated
<PackageVersion Include="Basic.Reference.Assemblies.Net80" Version="1.7.2" /> | ||
<PackageVersion Include="Basic.Reference.Assemblies.Net461" Version="1.7.2" /> | ||
<PackageVersion Include="Basic.Reference.Assemblies.NetStandard13" Version="1.7.2" /> | ||
<PackageVersion Include="Basic.Reference.Assemblies.Net90" Version="1.7.2" /> |
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.
@jcouv this new version also has the XML doc comments that you asked for in the last update. #Closed
@dotnet/roslyn-compiler PTAL |
eng/Directory.Packages.props
Outdated
<PackageVersion Include="Basic.Reference.Assemblies.Net80" Version="1.4.5" /> | ||
<PackageVersion Include="Basic.Reference.Assemblies.Net461" Version="1.6.0" /> | ||
<PackageVersion Include="Basic.Reference.Assemblies.NetStandard13" Version="1.6.0" /> | ||
<PackageVersion Include="Basic.Reference.Assemblies.NetStandard20" Version="1.7.2" /> |
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.
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.
Yeah that would make sense now that I've unified them all.
@@ -91,7 +91,8 @@ public enum TargetFramework | |||
Net50, | |||
Net60, | |||
Net70, | |||
Net80 | |||
Net80, | |||
Net90, |
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.
Consider adding a test or upgrading an existing one to exercise this new TFM #Closed
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.
All the places for the new tests are in the ref generic interfaces branch. For merge purposes it's better to have the initial commit here. Once Aleksey merges this in we'll move to this TFM. If there are any issues I will deal with the fallout.
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.
Done with review pass (iteration 3)
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.
LGTM Thanks (iteration 4)
@@ -292,6 +293,7 @@ static TargetFrameworkUtil() | |||
TargetFramework.Net60 => ImmutableArray.CreateRange<MetadataReference>(LoadDynamicReferences("Net60")), | |||
TargetFramework.NetCoreApp or TargetFramework.Net70 => ImmutableArray.CreateRange<MetadataReference>(Net70.References.All), | |||
TargetFramework.Net80 => ImmutableArray.CreateRange<MetadataReference>(LoadDynamicReferences("Net80")), | |||
TargetFramework.Net90 => ImmutableArray.CreateRange<MetadataReference>(LoadDynamicReferences("Net90")), |
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.
Why are tests failling if this isn't used anywhere?
Example test failure in Microsoft.CodeAnalysis.CSharp.UnitTests.CollectionExpressionTests.SpanArgument_01(targetFramework: Net80):
++> IL_001d: newobj "System.ReadOnlySpan<int?>..ctor(ref readonly int?)"
--> IL_001d: newobj "System.ReadOnlySpan<int?>..ctor(in int?)"
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.
Hmm ... that is odd.
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.
The tests all pass when run individually but fail when the entire test class is executed. We must be holding onto state somewhere
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.
I do not see a single line in CollectionExpressionTests that starts with IL_001d: newobj
. What am I missing?
Regarding the diff, is it possible that we are updated to ref assembly for 8.0 that use ref readonly
in the constructor?
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.
The tests all pass when run individually but fail when the entire test class is executed. We must be holding onto state somewhere
It is likely that CI failures are expected given the changes to ref assemblies that you are making. It looks to me like your branch is behind main enough to make the difference between the local state and the merged state the CI is running against. That is why CI failures do not make sense in context of your local branch. Consider merging main into you branch, then running tests locally.
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.
Figured out what is happening here. When I updated the reference assemblies recently I missed updating the net80
set. That means the NuPkg remained at 1.4.5 which is based off of the 8.0.0-preview.7.23375.6 version of .NET. The ref readonly
parameter changes came in after that. Now that I unified the versions into a single MSBuild prop that dragged the net80
version forward to RTM and we're seeing the signature differences.
Kind of tangential, but how to avoid building the project for every TFM when building locally? Is there a quick way to limit tests/build to only one framework by default? |
@jaredpar I think we should change this to Refers to: src/Compilers/CSharp/Test/Emit3/Microsoft.CodeAnalysis.CSharp.Emit3.UnitTests.csproj:7 in 7c5510e. [](commit_id = 7c5510e, deletion_comment = False) |
Or did you plan to do this later, in RefstructInterfaces branch? In reply to: 2111024335 Refers to: src/Compilers/CSharp/Test/Emit3/Microsoft.CodeAnalysis.CSharp.Emit3.UnitTests.csproj:7 in 53640ea. [](commit_id = 53640ea, deletion_comment = False) |
A change like that causes: "C:\Program Files\dotnet\sdk\8.0.104\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(166,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 9.0. Either target .NET 8.0 or lower, or use a version of the .NET SDK that supports .NET 9.0. Download the .NET SDK from https://aka.ms/dotnet/download" In reply to: 2111027934 Refers to: src/Compilers/CSharp/Test/Emit3/Microsoft.CodeAnalysis.CSharp.Emit3.UnitTests.csproj:7 in 53640ea. [](commit_id = 53640ea, deletion_comment = False) |
This. Moving to a new |
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.
LGTM (commit 5)
This adds the .NET 9 reference assemblies into our test harness.