Refactor task environment IPC to use structured data and semantic hin…#1239
Open
shrudge wants to merge 3 commits into
Open
Refactor task environment IPC to use structured data and semantic hin…#1239shrudge wants to merge 3 commits into
shrudge wants to merge 3 commits into
Conversation
shrudge
requested review from
aciidgh,
jakepetroules,
mhrawdon,
neonichu and
owenv
as code owners
jakepetroules
left a comment
jakepetroules
left a comment
Collaborator
There was a problem hiding this comment.
I think this is a good start overall; @owenv what do you think?
| let taskSpec = task.type as? Spec | ||
| let serializedDiagnosticsPaths = task.type.serializedDiagnosticsInfo(task, operation.requestContext.fs).map(\.serializedDiagnosticsPath) | ||
| let info = BuildOperationTaskInfo(taskName: taskSpec?.name ?? "", signature: .taskIdentifier(ByteString(encodingAsUTF8: task.identifier.rawValue)), ruleInfo: task.ruleInfo.quotedDescription, executionDescription: (task.execDescription ?? task.ruleInfo.quotedDescription), commandLineDisplayString: task.showCommandLineInLog ? commandLineDisplayString(task.commandLine.map(\.asByteString), additionalOutput: task.additionalOutput, workingDirectory: task.workingDirectory, environment: environmentToShow, dependencyInfo: dependencyInfo, hostOS: workspaceContext.core.hostOperatingSystem) : nil, interestingPath: interestingPath, serializedDiagnosticsPaths: serializedDiagnosticsPaths) | ||
| let info = BuildOperationTaskInfo(taskName: taskSpec?.name ?? "", signature: .taskIdentifier(ByteString(encodingAsUTF8: task.identifier.rawValue)), ruleInfo: task.ruleInfo.quotedDescription, executionDescription: (task.execDescription ?? task.ruleInfo.quotedDescription), commandLineDisplayString: task.showCommandLineInLog ? commandLineDisplayString(task.commandLine.map(\.asByteString), additionalOutput: task.additionalOutput, workingDirectory: task.workingDirectory, environment: nil, dependencyInfo: dependencyInfo, hostOS: workspaceContext.core.hostOperatingSystem) : nil, interestingPath: interestingPath, serializedDiagnosticsPaths: serializedDiagnosticsPaths, taskEnvironment: taskEnvironment, environmentDisplayHint: environmentDisplayHint) |
Collaborator
There was a problem hiding this comment.
We might want to keep 'environment' as the original value for now to give clients time to transition.
owenv
reviewed
Apr 2, 2026
|
|
||
| public init(from deserializer: any Deserializer) throws { | ||
| try deserializer.beginAggregate(7) | ||
| try deserializer.beginAggregate(9) |
Collaborator
There was a problem hiding this comment.
I think we need to find a way to stage this change in so it doesn't break IPC compatibility immediately for out of process clients
Collaborator
There was a problem hiding this comment.
You can do let count = try deserializer.beginAggregate(7...9), IIRC, and then use the count value to decide what fields to deserialize.
Contributor
Author
There was a problem hiding this comment.
thanks for these reviews, I will get a patch pushed up shortly.
Contributor
Author
There was a problem hiding this comment.
@jakepetroules I went with 7...8 here instead of 7...9 Since we completely dropped the EnvironmentDisplayHint property per the other feedback.
…mptyState for nilIfEmpty
jakepetroules
approved these changes
Apr 6, 2026
Collaborator
|
@swift-ci test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor task environment IPC to use structured data (#1211)
Motivation
I’m finally putting this up for review after getting sidetracked by some pre-existing noise in BuildOperationTests—mostly visionOS and RealityAssetsCompile errors that seem specific to my local environment.
The main goal here is to stop the server from "baking" environment strings directly into the command log. By sending structured data instead, we move the presentation logic to the client where it belongs and keep the message stream much cleaner.
Modifications
IPC Stability: Updated serialization aggregate counts for BuildOperationStarted (1→2) and BuildOperationTaskInfo (7→9).
Efficiency: The base environment is now sent once at build start to avoid repeating it for every task.
Structured Deltas: Tasks now send a [String: String] dictionary of their specific environment changes.
Semantic Hints: Replaced the old boolean with a hint (.normal vs .prominent) so the client can decide when the environment is important enough to show.
Result
The message stream is now more efficient, and the client has full control over how to merge and render environments. I’ve verified the stability with MessageSerializationTests and confirmed the client-side merging in BuildLogTests.