Remove workaround in tests for clearing EnableUnsafeBinaryFormatterSerialization

[elinor-fung]

This is effectively a revert of b63f588

Fixes #87811

[Copilot]

Pull Request Overview

This PR removes a workaround in tests that was previously required for clearing the EnableUnsafeBinaryFormatterSerialization configuration option. The change reverts to using MSBuild properties instead of a custom MSBuild target to control BinaryFormatter enablement/disablement behavior in tests.

Key changes:

• Replaces custom MSBuild target with standard property configuration
• Updates documentation comments to reflect the proper approach
• Simplifies the test project configuration

[dotnet-policy-service]

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

[elinor-fung]

@GrabYourPitchforks @adamsitnik I don't actually know that setting an internal-by-convention property in combination with a blank EnableUnsafeBinaryFormatterSerialization (the original) is clearer than the target to remove the RuntimeHostConfigurationOption (the workaround). I'll leave it to you to tell me which version you actually want in the tests.

[adamsitnik]

I am sorry, but this is MSBuild magic that I don't understand.

However, it seems that we enable EnableUnsafeBinaryFormatterSerialization for the whole runtime repo:

runtime/Directory.Build.props

Lines 355 to 358 in 4bbd32d

	<!-- we need to re-enable BinaryFormatter within test projects since some tests exercise these code paths to ensure compat -->
	<EnableUnsafeBinaryFormatterSerialization>true</EnableUnsafeBinaryFormatterSerialization>
	<!-- For DotNetBuildSourceOnly, only the bundled BinaryFormatter is built which does not support serialization. -->
	<EnableUnsafeBinaryFormatterSerialization Condition="'$(DotNetBuildSourceOnly)' == 'true'">false</EnableUnsafeBinaryFormatterSerialization>

And we need to have it disabled only in one place (to test that it throws by default):

runtime/src/libraries/System.Runtime.Serialization.Formatters/tests/Disabled/System.Runtime.Serialization.Formatters.Disabled.Tests.csproj

Lines 12 to 21 in 4bbd32d

	<!--
	We're testing the BinaryFormatter enablement / disablement switch, so we need to suppress any inherited behavior.
	See https://github.com/dotnet/runtime/issues/87811 for more details.
	This is a temporary solution and should be reverted back to the original once the necessary changes are made in
	shared frameworks and SDK.
	-->
	<Target Name="RemoveSerializationRuntimeOption" BeforeTargets="GenerateBuildRuntimeConfigurationFiles">
	<ItemGroup>
	<RuntimeHostConfigurationOption Remove="System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization" />
	</ItemGroup>

Perhaps a simpler solution would be to remove

runtime/Directory.Build.props

Lines 355 to 358 in 4bbd32d

	<!-- we need to re-enable BinaryFormatter within test projects since some tests exercise these code paths to ensure compat -->
	<EnableUnsafeBinaryFormatterSerialization>true</EnableUnsafeBinaryFormatterSerialization>
	<!-- For DotNetBuildSourceOnly, only the bundled BinaryFormatter is built which does not support serialization. -->
	<EnableUnsafeBinaryFormatterSerialization Condition="'$(DotNetBuildSourceOnly)' == 'true'">false</EnableUnsafeBinaryFormatterSerialization>

Move all the tests that use BinaryFormatter to one project, and enable this setting only there?

https://github.com/dotnet/runtime/tree/4bbd32d81f97096e9b51ed76ec0e202962e3a115/src/libraries/System.Runtime.Serialization.Formatters/tests

[elinor-fung]

My understanding from the comment is that we want the setting to be empty - that is, not set at all rather than set to true/false in the runtimeconfig. The SDK will explicitly set it to false if it is not already set to true and if _ProjectTypeRequiresBinaryFormatter != true:
https://github.com/dotnet/sdk/blob/64f04db4e06df420e05424ccb221c3171725367a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L200
So in order to explicitly clear it out, we either:

• Set _ProjectTypeRequiresBinaryFormatter=true so that the SDK doesn't set EnableUnsafeBinaryFormatterSerialization and
• Have a target remove the RuntimeHostConfigurationOption before the runtimeconfig is generated

Move all the tests that use BinaryFormatter to one project, and enable this setting only there?

I like the idea, but it seems a lot of different libraries (not just System.Runtime.Serialization.Formatters) have test cases for when BinaryFormatter is supported: https://github.com/search?q=repo%3Adotnet%2Fruntime%20IsBinaryFormatterSupported&type=code

I don't think it makes sense to consolidate all those, so it would be adding the enabling in each of those separate unit tests. I think that is somewhat separate from the clearing of the property that System.Runtime.Serialization.Formatters.Disabled.Tests is doing though - even if we were not explicitly setting EnableUnsafeBinaryFormatterSerialization=true for all tests, this test would still need to do something to either prevent the SDK from setting it to false or clear it out after the SDK sets it.

[adamsitnik]

PTAL at my comment, thanks @elinor-fung !

[adamsitnik]

I am sorry, but this is MSBuild magic that I don't understand.

However, it seems that we enable EnableUnsafeBinaryFormatterSerialization for the whole runtime repo:

runtime/Directory.Build.props

Lines 355 to 358 in 4bbd32d

	<!-- we need to re-enable BinaryFormatter within test projects since some tests exercise these code paths to ensure compat -->
	<EnableUnsafeBinaryFormatterSerialization>true</EnableUnsafeBinaryFormatterSerialization>
	<!-- For DotNetBuildSourceOnly, only the bundled BinaryFormatter is built which does not support serialization. -->
	<EnableUnsafeBinaryFormatterSerialization Condition="'$(DotNetBuildSourceOnly)' == 'true'">false</EnableUnsafeBinaryFormatterSerialization>

And we need to have it disabled only in one place (to test that it throws by default):

runtime/src/libraries/System.Runtime.Serialization.Formatters/tests/Disabled/System.Runtime.Serialization.Formatters.Disabled.Tests.csproj

Lines 12 to 21 in 4bbd32d

	<!--
	We're testing the BinaryFormatter enablement / disablement switch, so we need to suppress any inherited behavior.
	See https://github.com/dotnet/runtime/issues/87811 for more details.
	This is a temporary solution and should be reverted back to the original once the necessary changes are made in
	shared frameworks and SDK.
	-->
	<Target Name="RemoveSerializationRuntimeOption" BeforeTargets="GenerateBuildRuntimeConfigurationFiles">
	<ItemGroup>
	<RuntimeHostConfigurationOption Remove="System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization" />
	</ItemGroup>

Perhaps a simpler solution would be to remove

runtime/Directory.Build.props

Lines 355 to 358 in 4bbd32d

	<!-- we need to re-enable BinaryFormatter within test projects since some tests exercise these code paths to ensure compat -->
	<EnableUnsafeBinaryFormatterSerialization>true</EnableUnsafeBinaryFormatterSerialization>
	<!-- For DotNetBuildSourceOnly, only the bundled BinaryFormatter is built which does not support serialization. -->
	<EnableUnsafeBinaryFormatterSerialization Condition="'$(DotNetBuildSourceOnly)' == 'true'">false</EnableUnsafeBinaryFormatterSerialization>

Move all the tests that use BinaryFormatter to one project, and enable this setting only there?

https://github.com/dotnet/runtime/tree/4bbd32d81f97096e9b51ed76ec0e202962e3a115/src/libraries/System.Runtime.Serialization.Formatters/tests