-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Update CoreCLR dependencies in dev/eng #14866
Conversation
@@ -12,7 +12,7 @@ public static class RuntimeDetection | |||
private static readonly string s_frameworkDescription = RuntimeInformation.FrameworkDescription; | |||
|
|||
// public static bool IsMono { get; } = Type.GetType("Mono.Runtime") != null; | |||
// public static bool IsNetFramework { get; } = s_frameworkDescription.StartsWith(".NET Framework"); | |||
public static bool IsNetFramework { get; } = s_frameworkDescription.StartsWith(".NET Framework"); |
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.
What is the connection for this?
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.
never-mind I see you use it below but I guess I'm not sure why it was ever commented out.
@@ -3,8 +3,8 @@ | |||
"{TFM}": { | |||
"dependencies": { | |||
"Microsoft.NETCore.Platforms": "1.2.0-beta-24721-02", | |||
"Microsoft.NETCore.Runtime.CoreCLR": "1.2.0-beta-24728-02", | |||
"Microsoft.NETCore.TestHost": "1.2.0-beta-24728-02", | |||
"Microsoft.NETCore.Runtime.CoreCLR": "1.2.0-beta-24904-03", |
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.
Did you have to manually update this or did the update task correctly update the json.template file? If it didn't auto-update this can you please file an issue to track that work as we want the auto updates to work here.
@@ -628,10 +628,13 @@ public static void Append_CharArray_Invalid() | |||
Assert.Throws<ArgumentNullException>("value", () => builder.Append((char[])null, 1, 1)); // Value is null, startIndex > 0 and count > 0 | |||
|
|||
Assert.Throws<ArgumentOutOfRangeException>("startIndex", () => builder.Append(new char[0], -1, 0)); // Start index < 0 | |||
Assert.Throws<ArgumentOutOfRangeException>("count", () => builder.Append(new char[0], 0, -1)); // Count < 0 | |||
Assert.Throws<ArgumentOutOfRangeException>( |
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.
These are actual bugs that we should have filed so that we can ensure the behavior matches between .NET Core and .NET Framework. We shouldn't blindly condition these, we should be very specific about making this change. If these are just about the parameter name for example I would much prefer we rename the parameter in .NET Core to match .NET Framework.
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.
Ignore this comment as I just realized you only ported this change. I just added comments the the original PR instead.
LGTM. |
…eclr Update CoreCLR dependencies in dev/eng Commit migrated from dotnet/corefx@d4d3bbd
I've also cherry-picked a commit to fix the test failures, see #14840.
@weshaggard @joperezr @chcosta