Skip to content
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

refactor: prepare serialization infrastructure for .NET 8+ #1034

Merged

Conversation

vbreuss
Copy link
Member

@vbreuss vbreuss commented Aug 25, 2023

This fixes #1033:
As BinaryFormatter becomes obsolete, remove the tests for serialization on .NET8.0 or greater. Also move the Serialization attribute behind a feature flag, which is currently always active, but will be disabled with .NET9.

@vbreuss vbreuss changed the title refactor: disable serialization for .NET8 or greater refactor: remove Serializable attribute for .NET8 or greater Aug 25, 2023
@vbreuss vbreuss marked this pull request as ready for review August 25, 2023 12:20
@fgreinacher
Copy link
Contributor

Just a quick answer while I'm on vacation 😎

Reading https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md I don't see SerializableAttribute being deprecated for .NET 8. What's the reason that we have to exclude it?

@vbreuss
Copy link
Member Author

vbreuss commented Aug 31, 2023

Just a quick answer while I'm on vacation 😎

Reading https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md I don't see SerializableAttribute being deprecated for .NET 8. What's the reason that we have to exclude it?

@fgreinacher
Probably we don't have to exclude it, but as this attribute only applies when using the BinaryFormatter or SoapFormatter, which both are obsolete in .NET8 I would also remove the attribute (see also here).

I can of course keep the attributes also for .NET8, but there is no way to test them anymore!

@vbreuss vbreuss closed this Aug 31, 2023
@vbreuss vbreuss reopened this Aug 31, 2023
@fgreinacher
Copy link
Contributor

fgreinacher commented Sep 8, 2023

@vbreuss If I understand the upstream documentation (especially https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md#binaryformatter-disabled-by-default-across-all-project-types-net-8) right, applications can still opt in to binary serialization via the EnableUnsafeBinaryFormatterSerialization switch. As this library can be used in such scenarios I think we should not remove the attributes for .NET 8 and instead set the switch in our test project. WDYT?

@vbreuss
Copy link
Member Author

vbreuss commented Sep 8, 2023

If I understand the upstream documentation (especially https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md#binaryformatter-disabled-by-default-across-all-project-types-net-8) right, applications can still opt in to binary serialization via the EnableUnsafeBinaryFormatterSerialization switch. As this library can be used in such scenarios I think we should not remove the attributes for .NET 8 and instead set the switch in our test project.

@fgreinacher
I interpreted this
image
to mean, that the attribute should no longer be used in .NET 8 or newer for custom types.
As an upgrade of the .NET version can always result in a breaking change, I preferred to remove the SerializableAttribute altogether.

However, I see your point that the workaround via EnableUnsafeBinaryFormatterSerialization is not working anymore.
I can update the pull request, to keep the attributes. However, I don't think it necessary to explicitely update the tests, and instead I would just disable the serialization tests for .NET 8 or newer.

The attribute would then be removed with .NET 9 when the BinaryFormatter infrastructure gets removed, right?
image

@fgreinacher
Copy link
Contributor

fgreinacher commented Sep 11, 2023

However, I don't think it necessary to explicitely update the tests, and instead I would just disable the serialization tests for .NET 8 or newer.

I think we should keep them because we should still support the legacy serialization if applications explicitly opt-in.

The attribute would then be removed with .NET 9 when the BinaryFormatter infrastructure gets removed, right?

Yes, seems like that :)

@vbreuss vbreuss changed the title refactor: remove Serializable attribute for .NET8 or greater refactor: disable serialization for .NET8 or greater Sep 11, 2023
@vbreuss
Copy link
Member Author

vbreuss commented Sep 11, 2023

@fgreinacher
I moved the SerializableAttribute behind a feature flag, that is currently always active. This way it will be easy to disable for a specific framework version in future.
The tests for serialization will only run pre-.NET8.

I hope this is okay with you :-)

@fgreinacher
Copy link
Contributor

I moved the SerializableAttribute behind a feature flag, that is currently always active. This way it will be easy to disable for a specific framework version in future.

That's cool, thanks a lot!

The tests for serialization will only run pre-.NET8.

As mentioned above I'd prefer to add not exclude them and instead add this the test csprojs:

  <PropertyGroup>
    <!--
      Allow deprecated binary formatter functionality on .NET 8 so that we can test it
    -->
    <EnableUnsafeBinaryFormatterSerialization Condition="'$(TargetFramework)' == 'net8.0'">true</EnableUnsafeBinaryFormatterSerialization>
  </PropertyGroup>

@fgreinacher fgreinacher changed the title refactor: disable serialization for .NET8 or greater refactor: prepare serialization infrastructure for .NET 8+ Sep 12, 2023
Copy link
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

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

Thanks again @vbreuss 🙇

@mergify mergify bot merged commit 475b8c1 into TestableIO:main Sep 12, 2023
10 checks passed
mergify bot referenced this pull request in Testably/Testably.Abstractions Nov 16, 2023
…19.2.87 (#424)

[![Mend Renovate logo
banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[TestableIO.System.IO.Abstractions](https://togithub.com/TestableIO/System.IO.Abstractions)
| `19.2.69` -> `19.2.87` |
[![age](https://developer.mend.io/api/mc/badges/age/nuget/TestableIO.System.IO.Abstractions/19.2.87?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/nuget/TestableIO.System.IO.Abstractions/19.2.87?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/nuget/TestableIO.System.IO.Abstractions/19.2.69/19.2.87?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/nuget/TestableIO.System.IO.Abstractions/19.2.69/19.2.87?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>TestableIO/System.IO.Abstractions
(TestableIO.System.IO.Abstractions)</summary>

###
[`v19.2.87`](https://togithub.com/TestableIO/System.IO.Abstractions/releases/tag/v19.2.87)

##### What's Changed

- chore(deps): update actions/checkout action to v4 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1037](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1037)
- chore(deps): update danielpalme/reportgenerator-github-action action
to v5.1.25 by [@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1038](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1038)
- chore(deps): update dependency benchmarkdotnet to v0.13.8 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1039](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1039)
- refactor: prepare serialization infrastructure for .NET 8+ by
[@&#8203;vbreuss](https://togithub.com/vbreuss) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1034](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1034)
- chore(deps): update dependency dotnet-sdk to v7.0.401 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1040](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1040)
- chore(deps): update amannn/action-semantic-pull-request action to
v5.3.0 by [@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1041](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1041)
- chore(deps): update dependency benchmarkdotnet to v0.13.9 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1042](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1042)
- chore(deps): update danielpalme/reportgenerator-github-action action
to v5.1.26 by [@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1043](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1043)
- chore(deps): update dependency dotnet-sdk to v7.0.402 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1045](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1045)
- chore(deps): update dependency dotnet-sdk to v7.0.403 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1047](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1047)
- chore(deps): update dependency benchmarkdotnet to v0.13.10 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1048](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1048)
- chore(deps): update amannn/action-semantic-pull-request action to
v5.4.0 by [@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1050](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1050)
- chore(deps): update dependency nunit to v3.14.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1051](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1051)
- chore(deps): update dependency microsoft.net.test.sdk to v17.8.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1052](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1052)
- chore(deps): update danielpalme/reportgenerator-github-action action
to v5.2.0 by [@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1054](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1054)
- chore(deps): update dependency dotnet-sdk to v7.0.404 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1055](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1055)
- chore(deps): update dependency microsoft.sourcelink.github to v8 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1056](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1056)
- feat: enable net8.0 by [@&#8203;vbreuss](https://togithub.com/vbreuss)
in
[https://github.com/TestableIO/System.IO.Abstractions/pull/1032](https://togithub.com/TestableIO/System.IO.Abstractions/pull/1032)

**Full Changelog**:
TestableIO/System.IO.Abstractions@v19.2.69...v19.2.87

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/Testably/Testably.Abstractions).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy41OS44IiwidXBkYXRlZEluVmVyIjoiMzcuNTkuOCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Copy link

github-actions bot commented Dec 5, 2023

This is addressed in release v19.2.87.

@github-actions github-actions bot added the state: released Issues that are released label Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: released Issues that are released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BinaryFormatter becomes obsolete -> Remove it with .NET 8.0
2 participants