Move the working BinaryFormatter implementation to a NuGet package #103255

bartonjs · 2024-06-10T23:16:41Z

This change makes System.Runtime.Serialization.Formatters.dll build for both NetCoreAppMinimum and NetCoreAppCurrent.

The NetCoreAppCurrent version has a copy of BinaryFormatter that unconditionally throws PNSE from calls to Serialize or Deserialize.
- It is included in the shared runtime
- It is permanently AssemblyVersion locked to 8.1 ("bigger than 8.0, smaller than 9.0")
- It is not included in the new NuGet package.
The NetCoreAppMinimum version(s) are exactly what this library was in .NET 8
- The AppContext switch is still required to use it
- These TFMs are included in a new System.Runtime.Serialization.Formatters nupkg
- The AssemblyVersion floats with the repository (so 9.0 now, and we could stop producing the package in 10... or make a 10.0 then)
- Because the net8 build can't reference System.Private.Corelib (it only builds for the current version), it can't call the SerializationGuard APIs without using reflection.

Points of contention:

It still uses the AppContext switch. (Some wanted it removed, some wanted it to remain... and remaining was a smaller diff, so I left it in)
We can't currently run tests for both varieties, as we can't force a downlevel runtime in tests (Support testing previous non-current .NETCoreApp targeted libraries #54639)
- Removing the $(NetCoreAppCurrent) TFM from the src.csproj allows one to run the tests for the DLL(s) that go into the nupkg
Because of the ILLink Substitutions file that's in the mobile varieties, I can't figure out how to collapse all of the NetCoreAppMinimum builds down into one build with a runtime check. If someone tells me how to do this, or we can agree to get rid of the AppContext opt-in, the package size will drop by 80%.

Contributes to #98245.

...s/System.Runtime.Serialization.Formatters/ref/System.Runtime.Serialization.Formatters.csproj

...s/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj

ViktorHofer · 2024-06-11T13:58:30Z

Because of the ILLink Substitutions file that's in the mobile varieties, I can't figure out how to collapse all of the NetCoreAppMinimum builds down into one build with a runtime check. If someone tells me how to do this, or we can agree to get rid of the AppContext opt-in, the package size will drop by 80%.

Yes, I think we need to do this. We shouldn't ship a new package with RID specific assets in it. Either option sounds fine to me.

...lization.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryObjectReader.cs

jkotas · 2024-06-11T15:41:14Z

Because of the ILLink Substitutions file that's in the mobile varieties,

The mobile was special-cases since the BF was unconditionally disabled inbox for mobile, while the non-mobile platforms allowed opt-in. We should get rid of all special casing of mobile now that the BF is unconditionally disable inbox for all RIDs:

Delete all RID-specific builds
Delete all Condition="'$(TargetPlatformIdentifier)' == ''" and `Condition="'$(TargetPlatformIdentifier)' != ''" conditions
Delete System\Runtime\Serialization\Formatters\Binary\BinaryFormatter.PlatformNotSupported.cs
If you are going to keep AppContext opt-in, rename ILLink.Substitutions.NonMobile.xml to ILLink.Substitutions.xml

...zation.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatter.Core.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

stephentoub · 2024-06-13T12:52:58Z

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

+
+            if (IsNativeAot)
+            {
+                Console.WriteLine("BinaryFormatter is disabled in NativeAOT");


Did you intend to leave these Console.WriteLines in?

I do/did. When a test wants to use BinaryFormatter it leaves a place where it becomes clear why it's there or not.

But, if there's objection, I can certainly remove them.

stephentoub · 2024-06-13T12:54:15Z

...s/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj

@@ -1,20 +1,24 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppCurrent)-browser;$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos;$(NetCoreAppCurrent)-android</TargetFrameworks>
+    <TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppMinimum)</TargetFrameworks>


Why do we need to build this for .NET 8?

It's the best/only way I could find for having two builds for the DLL, one where BF works (net8/NuGet) and one where it doesn't (net9/shared runtime).

Where are the problems with building the non-working inbox version via independent project?

Hm. The original plan was to typeforward BinaryFormatter out into a new library and do the package trick there (which would still have required making two copies), but it turns out that System.Runtime.Serialization.Formatters.dll is itself basically the minimum closure of BinaryFormatter.

It sounds like your suggestion is to sort of do that again, making something like

src/libraries/System.Runtime.Serialization.Formatters/src/ : IsPackable, net9 src/libraries/System.Runtime.Serialization.Formatters.Inbox/src/ : !IsPackable, net9, also claim to be System.Runtime.Serialization.Formatters.dll, <Compile Include=../../System.Runtime.Serialization.Formatters/src/AllTheThingsThatArentBinaryFormatter />

I don't know how well our build system would like that. And since the preview snap date is Tuesday, I don't know that I have the time to find out before then. We could explore it after.

Yes, something like that. The current scheme where the build for NetCoreAppMinimum is meant for NetCoreAppCurrent is not easy to understand. It is fine to explore something better later. It is possible that what you have put together is the best out of all bad solutions.

NuGet limits us here. It doesn't support specifying multiple TFMs that map to the same TargetFrameworkMoniker+TargetPlatformMoniker tuple in a project. There's a proposal up to fix that eventually but until then, separate projects need to be used in such scenarios.

We have multiple source generator project files per rdifferent roslyn targeting pack version because of the exact same reason.

I would agree with Jan that the current solution might be good enough for what's proposed.

...zation.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatter.Core.cs

stephentoub · 2024-06-13T12:58:01Z

...lization.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryObjectReader.cs

+            MethodInfo? targetMethod = typeof(SerializationInfo).GetMethod(
+                "StartDeserialization",
+                BindingFlags.Public | BindingFlags.Static,
+                Type.EmptyTypes);


Worth caching (or caching a delegate created to it)?

It would be even better to use UnsafeAccessor - no reflection, no delegates, no caching. UnsafeAccessor
shipped in 8.0 that is the current NetCoreAppMinimum.

StartDeserialization returns a public type that isn't in the reference assembly; it looks like UnsafeAccessor requires an exact signature match. So, unless there's an unsafe typeref I don't think we can use it here.

CreateDelegate is similarly not an option, it refuses to loose-bind the return type (struct).

Slow invoke works, or we can add another method in corelib that does the boxing to IDisposable there so it can be bound with UnsafeAccessor. I'm guessing we don't like that option, so I'm guessing we're sticking with slow-invoke. But either way I've moved this to a different type, and am at least saving the MethodInfo.

Should we rather get rid of this (it would be my preference), or turn it into a public API?

The non-public dependencies of OOB nuget packages like this one are always factory for problems.

src/libraries/System.Runtime.Serialization.Formatters/tests/TestConfiguration.cs

GrabYourPitchforks · 2024-06-16T18:39:53Z

...stem.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/SerializationGuard.cs

+{
+    internal sealed class SerializationGuard
+    {
+        private static readonly MethodInfo? s_startDeserialization =


This is ok for now but should probably be removed before RTM. Serialization Guard has been a contentious feature and there's no guarantee it will retain its current API shape (or even exist at all!) going forward. Private reflection from an OOB package back into the shared framework adds unnecessary risk.

I agree. Can we just remove the whole thing, both from here and anywhere that's enlisting in serialization guard?

I'll remove the invocation from here, since Levi's already cool with that. I'll let the "can we delete SG altogether" fight continue elsewhere, since BinaryFormatter isn't the only thing activating it.

adamsitnik

LGTM. There are some imperfections, but I am fine with them.

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

…ion.cs Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

…kage

bartonjs · 2024-06-17T23:42:54Z

/ba-g The one reported test failure is known issue #101409, so the Build Analysis should be green.

Move the working BinaryFormatter implementation to a NuGet package

c508a3c

bartonjs added area-System.Runtime binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it labels Jun 10, 2024

bartonjs added this to the 9.0.0 milestone Jun 10, 2024

bartonjs requested review from GrabYourPitchforks and ViktorHofer June 10, 2024 23:16

bartonjs self-assigned this Jun 10, 2024

build-analysis bot mentioned this pull request Jun 11, 2024

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

ViktorHofer reviewed Jun 11, 2024

View reviewed changes

...s/System.Runtime.Serialization.Formatters/ref/System.Runtime.Serialization.Formatters.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Jun 11, 2024

View reviewed changes

...s/System.Runtime.Serialization.Formatters/ref/System.Runtime.Serialization.Formatters.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Jun 11, 2024

View reviewed changes

...s/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj Show resolved Hide resolved

ViktorHofer reviewed Jun 11, 2024

View reviewed changes

...s/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj Show resolved Hide resolved

jkotas reviewed Jun 11, 2024

View reviewed changes

...lization.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryObjectReader.cs Outdated Show resolved Hide resolved

Respond to feedback

f3b7ba8

jkotas reviewed Jun 12, 2024

View reviewed changes

...zation.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatter.Core.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 12, 2024

View reviewed changes

...zation.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatter.Core.cs Outdated Show resolved Hide resolved

bartonjs and others added 3 commits June 12, 2024 08:35

Apply suggestions from code review

3dd94ff

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Make the ILLink substitution conditional

ad7974f

Teach PlatformDetection.IsBinaryFormatterSupported about asmver 8.1

d69488f

stephentoub reviewed Jun 13, 2024

View reviewed changes

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Show resolved Hide resolved

stephentoub reviewed Jun 13, 2024

View reviewed changes

...zation.Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatter.Core.cs Show resolved Hide resolved

stephentoub reviewed Jun 13, 2024

View reviewed changes

src/libraries/System.Runtime.Serialization.Formatters/tests/TestConfiguration.cs Show resolved Hide resolved

bartonjs added 2 commits June 13, 2024 14:20

Disable System.Resources.Extensions.BinaryFormat.Tests

eaaa177

Move StartDeserialization wrapper to SerializationGuard.cs

72c9197

build-analysis bot mentioned this pull request Jun 14, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

build-analysis bot mentioned this pull request Jun 14, 2024

SslStreamTlsResumeTests.ClientDisableTlsResume_Succeeds failed in CI #103449

Open

bartonjs added 2 commits June 14, 2024 10:13

Fix more tests that used BinaryFormatter without checking support

9c68be5

Fix nuspec generation

a5c760d

GrabYourPitchforks reviewed Jun 16, 2024

View reviewed changes

GrabYourPitchforks approved these changes Jun 16, 2024

View reviewed changes

adamsitnik approved these changes Jun 17, 2024

View reviewed changes

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Outdated Show resolved Hide resolved

bartonjs and others added 3 commits June 17, 2024 11:23

Update src/libraries/Common/tests/TestUtilities/System/PlatformDetect…

be4a980

…ion.cs Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

Remove call to SerializationGuard

abdae6f

Merge branch 'bf_package' of github.com:/bartonjs/runtime into bf_pac…

5bb3345

…kage

build-analysis bot mentioned this pull request Jun 17, 2024

ExplicitConversion_FromSingle failing due to NaN != NaN #103347

Open

bartonjs mentioned this pull request Jun 17, 2024

System.Text.RegularExpressions.Tests - Fatal error. Internal CLR error. (0x80131506) #101409

Closed

bartonjs merged commit 4a7ac0e into dotnet:main Jun 17, 2024
82 of 84 checks passed

bartonjs deleted the bf_package branch June 17, 2024 23:57

tmds mentioned this pull request Jun 19, 2024

DisableBitTests.EnabledThroughFeatureSwitch/DisabledAlwaysInBrowser failures #103683

Closed

ellahathaway mentioned this pull request Jun 21, 2024

Update Source-Build SDK Diff Tests Baselines and Exclusions dotnet/sdk#41716

Merged

adamsitnik mentioned this pull request Jun 28, 2024

NrbfReader tests not working in WASM/mobile #104142

Open

MagicAndre1981 mentioned this pull request Jul 11, 2024

Epic- Update BinaryFormatter usage dotnet/winforms#6267

Open

10 tasks

github-actions bot locked and limited conversation to collaborators Jul 18, 2024

jeffhandley added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jul 25, 2024

dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the working BinaryFormatter implementation to a NuGet package #103255

Move the working BinaryFormatter implementation to a NuGet package #103255

bartonjs commented Jun 10, 2024

ViktorHofer commented Jun 11, 2024

jkotas commented Jun 11, 2024

stephentoub Jun 13, 2024

bartonjs Jun 13, 2024

stephentoub Jun 13, 2024

bartonjs Jun 13, 2024

jkotas Jun 14, 2024

bartonjs Jun 14, 2024

jkotas Jun 14, 2024 •

edited

Loading

ViktorHofer Jun 14, 2024 •

edited

Loading

stephentoub Jun 13, 2024

jkotas Jun 13, 2024

bartonjs Jun 13, 2024

bartonjs Jun 13, 2024

jkotas Jun 14, 2024

GrabYourPitchforks Jun 16, 2024

stephentoub Jun 16, 2024 •

edited

Loading

bartonjs Jun 17, 2024

adamsitnik left a comment

bartonjs commented Jun 17, 2024

Move the working BinaryFormatter implementation to a NuGet package #103255

Move the working BinaryFormatter implementation to a NuGet package #103255

Conversation

bartonjs commented Jun 10, 2024

ViktorHofer commented Jun 11, 2024

jkotas commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

ViktorHofer Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

bartonjs commented Jun 17, 2024

jkotas Jun 14, 2024 •

edited

Loading

ViktorHofer Jun 14, 2024 •

edited

Loading

stephentoub Jun 16, 2024 •

edited

Loading