-
Notifications
You must be signed in to change notification settings - Fork 321
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
Upgrade to .NET 6 #1112
Upgrade to .NET 6 #1112
Conversation
@@ -112,7 +112,9 @@ internal class RDDCommandExecutor | |||
{ | |||
case CommandSerDe.SerializedMode.Byte: | |||
BinaryFormatter formatter = s_binaryFormatter ??= new BinaryFormatter(); | |||
#pragma warning disable SYSLIB0011 // Type or member is obsolete |
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 worry about the continued use of BinaryFormatter due to https://learn.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide. pragma'ing away is necessary to compile...but I worry this needs to be fixed soon.
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.
Yes we will need to fix this in upcoming PRs
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... I am worried about it, and we'll get hounded by component governance for this... but if it's a vulnerability, it's already a vulnerability now, so we may as well solve it while on .net 6
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.
#1131 to defer
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.
Since it's just a MemoryStream this one is really easy to fix. BinaryFormatter doesn't give any value over just writing the length and bytes.
We are waiting for this release as lot of our applications are in .Net 6.0 and we would like to host them in Azure Databricks, thanks for creating this PR |
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.
Any reason to include src/csharp/upgrade-assistant.clef
?
Oops, removed. |
Need to fix the build pipeline. |
@AFFogarty Thanks for working on this, can't wait to see it land! |
@AFFogarty - Would be awesome if you can please merge this PR. My team is waiting for this PR ;) |
@vibhuic: We are blocked by the build failures. |
@AFFogarty any ETA on fixing the pipeline? I don't have access, but it seems we would just need to make sure the right SDK version is installed on the build machine. |
Require the dotnet 6 SDK for development in this repository, to force the build process to download this SDK version.
A bit of debugging indicates that Further testing locally suggests that there may be further changes to make. Emulating the CI build produces the following output:
This might need an update to the Arcade version, but I don't know enough about this toolchain to know how or where. |
@AFFogarty @Niharikadutta @ericstj Is this mergeable? |
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.
Changes look good to me, just a couple non-blocking comments.
@@ -112,7 +112,9 @@ internal class RDDCommandExecutor | |||
{ | |||
case CommandSerDe.SerializedMode.Byte: | |||
BinaryFormatter formatter = s_binaryFormatter ??= new BinaryFormatter(); | |||
#pragma warning disable SYSLIB0011 // Type or member is obsolete |
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.
Since it's just a MemoryStream this one is really easy to fix. BinaryFormatter doesn't give any value over just writing the length and bytes.
...ons/Microsoft.Spark.Extensions.Delta.E2ETest/Microsoft.Spark.Extensions.Delta.E2ETest.csproj
Outdated
Show resolved
Hide resolved
Please support the .NET migration to keep this Azure Doc Page VALID! |
@Niharikadutta @suhsteve @AFFogarty Is there anything left holding this up? |
@dragorosson |
examples/Microsoft.Spark.CSharp.Examples/Microsoft.Spark.CSharp.Examples.csproj
Show resolved
Hide resolved
examples/Microsoft.Spark.FSharp.Examples/Microsoft.Spark.FSharp.Examples.fsproj
Show resolved
Hide resolved
...ons/Microsoft.Spark.Extensions.Delta.E2ETest/Microsoft.Spark.Extensions.Delta.E2ETest.csproj
Outdated
Show resolved
Hide resolved
...oft.Spark.Extensions.Hyperspace.E2ETest/Microsoft.Spark.Extensions.Hyperspace.E2ETest.csproj
Outdated
Show resolved
Hide resolved
src/csharp/Microsoft.Spark.E2ETest/Microsoft.Spark.E2ETest.csproj
Outdated
Show resolved
Hide resolved
src/csharp/Microsoft.Spark.Worker.UnitTest/Microsoft.Spark.Worker.UnitTest.csproj
Outdated
Show resolved
Hide resolved
src/csharp/Microsoft.Spark.Worker/Microsoft.Spark.Worker.csproj
Outdated
Show resolved
Hide resolved
src/csharp/Microsoft.Spark.Worker/Processor/BroadcastVariableProcessor.cs
Show resolved
Hide resolved
Co-authored-by: Steve Suh <[email protected]>
Thanks for your efforts! The following may be relevant to your Todo list to get UDF working in e.g. Notebook:
One user has a solution to the ToDo problem. However, I have not looked into it yet. Additional feedback related to UDF |
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
<ItemGroup> | ||
<PackageReference Include="Microsoft.DotNet.DependencyManager" Version="10.10.0-beta.20254.4" /> | ||
<PackageReference Include="System.Memory" Version="4.5.3" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp3.1' "> |
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 assume this isn't required in net 6?
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 @AFFogarty
It's great to see this checked in. Thank you! I would imagine that there would be a release soon [?] @AFFogarty |
@chaoticsoftware Let us wait till next week. Let us feedback to support them to get the .NET6 working RIGHT through nuget release. |
This PR addresses #1032 by updating to .NET 6.