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

Deprecate binfmt in build event args #8917

Merged

Conversation

rokonec
Copy link
Member

@rokonec rokonec commented Jun 20, 2023

Fixes #8823 and #9008

Context

BinaryFormat deprecations is forcing us to us drop support of CustomEventArgs derived from EventArgs as those are serialized by omnipotent-but-unsecure BinaryFormatter.

Changes Made

  • making new sealed classes for implementing Custom events (message, warning, error, custom)
  • throwing exception when we detect EventArgs is about to be serialized by TranslateDotNet
  • fixing existing classes (ExternalProjectStartedEventArgs, ExternalProjectFinishedEventArgs, ...)
  • unit tests

Testing

  • unit tests
  • local debugging with temporary code changes
  • local tests

Notes

  • Consider use commits for easier review
  • Log Viewer is not handling these new events yet - recommended to do it soon in different PR sooner than later. This version does not change LogViewer event contract.

@rokonec rokonec requested a review from JanKrivanek June 20, 2023 11:55
@rokonec rokonec closed this Jun 21, 2023
@rokonec rokonec reopened this Jun 21, 2023
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.

Overall looks good!
I have couple of comments that I'd like to at least see answered or dismissed as irrelevant

src/Shared/BinaryReaderExtensions.cs Show resolved Hide resolved
src/Shared/LogMessagePacketBase.cs Show resolved Hide resolved
src/Framework/AssemblyLoadBuildEventArgs.cs Outdated Show resolved Hide resolved
src/Framework/IExtendedBuildEventArgs.cs Show resolved Hide resolved
src/Build/BackEnd/Node/OutOfProcNode.cs Outdated Show resolved Hide resolved
src/Deprecated/Engine/Resources/Strings.resx Outdated Show resolved Hide resolved
src/MSBuild/OutOfProcTaskHostNode.cs Outdated Show resolved Hide resolved
src/Shared/LogMessagePacketBase.cs Show resolved Hide resolved
@rainersigwald rainersigwald added this to the VS 17.8 milestone Jun 22, 2023
@rokonec rokonec requested a review from JanKrivanek June 23, 2023 09:21
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 to me!

As soon as we'll have consensus on the warnings target (only NET vs all)

@JanKrivanek
Copy link
Member

@rainersigwald - the warnings now apply only for core - should we mark this as candidate for 17.7 payload?

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.

Couple comments for @YuliiaKovalova

Plus we need to add ability (can be default behavior of the warning mode) of dropping the violating events - otherwise those new tests will start crashing as soon as we remove the temporary EnableUnsafeBinaryFormatterSerialization

Otherwise looks goot to go!

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Looks great! I have left a few comments inline. The most important one is about the shape of IExtendedBuildEventArgs.

src/MSBuild/OutOfProcTaskHostNode.cs Show resolved Hide resolved
src/Build/Instance/TaskFactoryLoggingHost.cs Show resolved Hide resolved
src/Build/BackEnd/Components/Logging/LoggingService.cs Outdated Show resolved Hide resolved
src/Shared/LogMessagePacketBase.cs Show resolved Hide resolved
src/Framework/IExtendedBuildEventArgs.cs Show resolved Hide resolved
src/Deprecated/Engine/Resources/Strings.resx Outdated Show resolved Hide resolved
rokonec and others added 3 commits August 9, 2023 13:31
@rokonec rokonec merged commit f121098 into dotnet:main Aug 9, 2023
@rokonec rokonec deleted the rokonec/8823-deprecate-binfmt-in-BuildEventArgs branch August 9, 2023 14:58
@YuliiaKovalova
Copy link
Member

YuliiaKovalova commented Aug 9, 2023 via email

rainersigwald pushed a commit that referenced this pull request Oct 26, 2023
Fixes #9220

CustomBuildEventArgs is deprecated. Users are advised to use ExtendedCustomBuildEventArgs instead.
See #8917

The comment change results in a public-facing doc change at
https://learn.microsoft.com/en-us/dotnet/api/microsoft.build.framework.custombuildeventargs?view=msbuild-17-netcore

The new Remarks section includes a link to a public-facing doc that explains the change:
https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/custombuildeventargs
PropertyReassignmentEventArgs propReassign = new("prop", "prevValue", "newValue", "loc", "message", "help", "sender", MessageImportance.Normal);
ResponseFileUsedEventArgs responseFileUsed = new("path");
UninitializedPropertyReadEventArgs uninitializedPropertyRead = new("prop", "message", "help", "sender", MessageImportance.Normal);
EnvironmentVariableReadEventArgs environmentVariableRead = new("env", "message", "help", "sender", MessageImportance.Normal);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing ProjectImportedEventArgs here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BinFmt] Custom BuildEventArgs de/serialization
6 participants