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

Removing last instances of the BinaryFormatter #11346

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

MichalPavlik
Copy link
Member

Fixes #11323

Context

We used BinaryFormatter in the serialization logic to support build custom events. With the VS 17.14, we can remove the last instances of the BinaryFormatter in our codebase.

Changes Made

TranslateDotNet implementation was removed and OutOfProcNode now unconditionally emits build error instead of sending packets with the custom event.

Testing

Modified existing unit test.

Notes

src/Shared/Resources/Strings.shared.resx Outdated Show resolved Hide resolved
src/Build/BackEnd/Components/Logging/LoggingService.cs Outdated Show resolved Hide resolved
src/Shared/LogMessagePacketBase.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Node/OutOfProcNode.cs Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good.

I'd just suggest to keep this warning on NETFx

src/Framework/Traits.cs Show resolved Hide resolved
src/Build/BackEnd/Node/OutOfProcNode.cs Show resolved Hide resolved
MichalPavlik and others added 3 commits January 29, 2025 11:00
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
@YuliiaKovalova
Copy link
Member

to fix the build issue you need to build msbuild locally and have the strings generated again.

@MichalPavlik
Copy link
Member Author

to fix the build issue you need to build msbuild locally and have the strings generated again.

Yeah, it's not the first time I've encountered this kind of error, but thank you :)

@MichalPavlik MichalPavlik added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 29, 2025
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

LGTM! For this kind of change I would find it quite helpful to see screenshots/console transcripts of the "after" experience with the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove BinaryFormatter
4 participants