Skip to content

Conversation

ccastanedaucf
Copy link
Contributor

@ccastanedaucf ccastanedaucf commented Sep 12, 2025

Context

LogMessagePacketBase provides a type-safe way to wrap BuildEventArgs into a node packet for IPC serialization.

For the RAR node to replay log events, it makes sense to reuse existing serialization wrappers instead of introducing another shim & reimplementing serialization for each event type.

However, right now LogMesssagePacketBase has a bit of a complex dependency chain - depending on preprocessor directives, certain events will either use a cached delegate of the base serialize methods, add additional properties to be serialized, or fully ignore the base implementation and provide a custom serializer. Another point is that many types in this path only exist in Microsoft.Build.dll (ArrayDictionary, ItemDictionary, PropertyDictionary, ect.).

One key here is that this additional logic is only relevant for Microsoft.Build.dll. Since MSBuild.dll, MSBuildTaskHost.dll, and (next) Microsoft.Build.Tasks.dll all use the default serialization, this custom logic can be refactored out, which removes the need for any conditional compilation.

Changes made

  • Remove compile directives from LogMessagePacketBase.
  • Move custom logic for Microsoft.Build.dll into its LogMessagePacket implementation.
  • Add virtual EventCanSerializeItself to mimic compile directive logic.
  • Virtualize WriteEventToStream and ReadEventFromStream to allow hooking in custom serializers.
  • Replace TargetFinishedTranslator delegate with virtual TranslateAdditionalProperties to allow serializing additional properties to the defaults.
  • De-abstract LogMessagePacketBase, as other assemblies only need the default implementation.

Note that this doesn't touch any of the actual serialization logic or build event types, just the packet wrapper - so this shouldn't require a version rev or introduce compatibility breaks.

@Copilot Copilot AI review requested due to automatic review settings September 12, 2025 22:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This refactoring removes Microsoft.Build.dll dependency from LogMessagePacketBase by eliminating preprocessor directives and moving custom serialization logic to the derived LogMessagePacket class. The change enables sharing the base serialization wrappers across assemblies (MSBuild.exe, MSBuildTaskHost.exe, and Microsoft.Build.Tasks.dll) while maintaining assembly-specific customizations.

Key changes:

  • Replaced compile-time directives with virtual methods for runtime customization
  • Moved Microsoft.Build.dll-specific serialization logic to LogMessagePacket class
  • Made LogMessagePacketBase concrete instead of abstract for default usage

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Shared/LogMessagePacketBase.cs Removed preprocessor directives and made class concrete with virtual methods for customization
src/Build/BackEnd/Components/Communications/LogMessagePacket.cs Added Microsoft.Build.dll-specific serialization logic moved from base class
src/MSBuild/OutOfProcTaskHostNode.cs Updated to use base class directly instead of derived LogMessagePacket
src/MSBuild/MSBuild.csproj Removed LogMessagePacket.cs from compilation
src/MSBuildTaskHost/MSBuildTaskHost.csproj Removed LogMessagePacket.cs link

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.

2 participants