Skip to content

Remove workaround in tests for clearing EnableUnsafeBinaryFormatterSerialization #118510

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove workaround in tests for clearing EnableUnsafeBinaryFormatterSerialization #118510

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions ...n.Formatters/tests/Disabled/System.Runtime.Serialization.Formatters.Disabled.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,19 @@
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-freebsd;$(NetCoreAppCurrent)-illumos;$(NetCoreAppCurrent)-solaris;$(NetCoreAppCurrent)-haiku;$(NetCoreAppCurrent)-linux;$(NetCoreAppCurrent)-osx;$(NetCoreAppCurrent)-ios;$(NetCoreAppCurrent)-tvos</TargetFrameworks>
</PropertyGroup>
<PropertyGroup>
<!--
We're testing the BinaryFormatter enablement / disablement switch, so we need to suppress any inherited behavior.
The specific combination below accomplishes this. Normal apps should *NOT* set this combination of switches and
should instead set the switches documented at:
https://learn.microsoft.com/dotnet/core/compatibility/core-libraries/7.0/binaryformatter-apis-produce-errors#recommended-action
-->
<_ProjectTypeRequiresBinaryFormatter>true</_ProjectTypeRequiresBinaryFormatter>
Copy link
Member Author

@elinor-fung elinor-fung Aug 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<EnableUnsafeBinaryFormatterSerialization><!-- intentionally left blank --></EnableUnsafeBinaryFormatterSerialization>
</PropertyGroup>
<ItemGroup>
<Compile Include="DisableBitTests.cs" />
<Compile Include="..\TestConfiguration.cs" />
<ProjectReference Include="$(LibrariesProjectRoot)System.Runtime.Serialization.Formatters\src\System.Runtime.Serialization.Formatters.csproj" />
</ItemGroup>
<!--
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>
</Target>
</Project>
Loading