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

[BinFmt] Custom BuildEventArgs de/serialization #8823

Closed
JanKrivanek opened this issue Jun 1, 2023 · 2 comments · Fixed by #8917
Closed

[BinFmt] Custom BuildEventArgs de/serialization #8823

JanKrivanek opened this issue Jun 1, 2023 · 2 comments · Fixed by #8917

Comments

@JanKrivanek
Copy link
Member

JanKrivanek commented Jun 1, 2023

Background

#6215
This subitem is focused on https://github.com/dotnet/msbuild/blob/main/src/Shared/LogMessagePacketBase.cs#L377

Suggested approach

Define custom build event type with de/serialization methods similar to #8779 (however without any type hierarchy/ overloading/ reflection etc - just single type and bag of strings), allow only such custom events - unles opted-out via changewave
Seal our BuildEventArgs hierarchy - to prevent subclasing (handling of subclasses would require registration of those during startup of nodes)

@JanKrivanek
Copy link
Member Author

Discussed with @rokonec:

  • Some usages derive from more derived event args (e.g. BuildWarningEventArgs) - so we need to have a way to allow preserving similar functionality
  • One way is to add a Dictionary<string, string>? CustomData property to the root (BuildEventArgs) - so that custom data can be attached to any of our events (without deriving)
  • Another way is to create single CustomBuildEventArgs that will hold the above member, plus it will hold a BuildEventArgs? AdditionalEvent property - to which the additional event can be attached.
  • The second approach would require adjusting all places in our code where we rely on type matching (e.g. specific handling of BuildWarningEventArgs)
  • Sealing our hierarchy is a breaking change - so should be conditionaly compiled only for NET (and only after transiting users from the custom events - so likely NET9 timeframe)

@JanKrivanek
Copy link
Member Author

Additional thoughts:

  • The de/serialization code handling custome event args currently should be adjusted to produce warning, and possibly somehow grace handle the disabled BinaryFormatter situation (either detect it, or catch) - so that MSBuild doesn't crash and so that users that choose EnableUnsafeBinaryFormatterSerialization (or whatever will be needed to flip this in SDK) can continue to operate their plugins, but with very explicit default warning (that should be maskable).
  • We should idealy add aka.ms links to such warnings - to point users to information about how to handle this properly
  • All this should probably be for Core only for now (FullFW will likely follow soon as well - net9?)

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

Successfully merging a pull request may close this issue.

3 participants