-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Capture code snippet from diagnostic compiler output #9362
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
base: main
Are you sure you want to change the base?
Conversation
* Built tentative test class SwiftBuildSystemOutputParser to handle the compiler output specifically * Added a handleDiagnostic method to possibly substitute the emitEvent local scope implementation of handling a SwiftBuildMessage diagnostic
* the flag `appendToOutputStream` helps us to determine whether a diagnostic is to be emitted or whether we'll be emitting the compiler output via OutputInfo * separate the emitEvent method into the SwiftBuildSystemMessageHandler
a20f020 to
f3aaabf
Compare
1dcaaeb to
c48e606
Compare
|
@swift-ci please test |
owenv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thie generally lgtm but I have some concerns about the regex-based parsing when we emit the textual compiler output.
- Perf - It's important this is fast so that it doesn't block the end of the build if a command produces huge quantities of output. It's hard to say if this will be a real issue without some testing
- We're re-parsing information which we're already getting from the compiler in structured form. I see the appeal of not reporting a diagnostic twice if multiple compile jobs report it though
I also pointed this out inline, though I'm not sure why the re-parsing relates to deduplication? We should be able to deduplicate diagnostics whether we parse them again or use the existing ones. |
(@jakepetroules @owenv -- tagging for visibility, GitHub notifications can be weird) Re-parsing doesn't affect the de-duplication -- when tracking the data buffer per task, I also track whether we've emitted the associated output for a given task (using its signature) and guard against this before emitting for that task again. We only go down this path (emitting for a given task) once we've received the task completed event. I mentioned this inline as well but for visibility: the re-parsing just addresses the fact that for a given task signature, we have an accumulated data buffer that contains all possible diagnostic messages coming from the compiler. I find that the user ergonomics aren't great when simply emitting the entire string blob through the observability scope, since these diagnostics can have varying severities and we'll have to decide up-front which severity to choose to emit the entire string of all diagnostics. I do also maintain a list of the Perhaps some more discussion is needed here. 😄 |
* Remove taskStarted outputStream emissions * Propagate exception if unable to find path
bkhouri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work, but we should also include automated tests as part of this change to ensure we won't regress the behaviour.
Sources/Basics/Observability.swift
Outdated
| public protocol DiagnosticsHandler: Sendable { | ||
| func handleDiagnostic(scope: ObservabilityScope, diagnostic: Diagnostic) | ||
|
|
||
| func printToOutput(message: String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's the DiagnosticHandlers responsibility to print the diagnostic here. We should instead have a "LoggingHandler" which would know where to emit the "message", whether it's to stdout, stderr, to a file, or some other place.
| /// Tracks the task IDs for failed tasks. | ||
| private var failedTasks: [Int] = [] | ||
| /// Tracks the tasks by their signature for which we have already emitted output. | ||
| private var tasksEmitted: Set<String> = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (possibly-blocking): what is the difference between tasksEmitted and taskIDsEmitted? If they are tracking the same thing, can we remove one? If we need both, could you please provide an explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many taskIDs can map to a singular taskSignature, and so I initially thought it could be helpful to track both separately. However, I think simply tracking by taskSignature may be sufficient here, so I removed taskIDsEmitted in the latest change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many taskIDs can map to a singular taskSignature
I think that shouldn't be true within a single build operation. Are you seeing that to not be the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen this behaviour locally, usually between two tasks that have an execution description of Emit Swift module (arm64) and Compile <filename>.swift (arm64) respectively; they'll share a task signature but have differing task IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a bug. The signatures are supposed to be globally unique.
cc @owenv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we should avoid using the signatures at all right now, and use only the numeric task IDs for safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, these two tasks also produce the same output (particularly the diagnostics) which helped to reduce duplicate logs. I'll try some alternative approaches locally but I may have to track both ID and signature for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an issue to investigate this in a separate PR: #9485
* Defer emission of diagnostics to completed task event * Remove global data buffer from TaskDataBuffer * Remove emits to observabilityScope for taskStarted events * Move SwiftBuildSystemMessageHandler to its own file * Rename printToOutput to print(_ output:verbose:) * Remove unprocessedDiagnostics parameter from message handler * Remove taskIDsEmitted in favour of using tasksEmitted
Modified the map to track these by targetID rather than name; since the `BuildResult` is expecting a map by targetName, another computed property representing the same map by targetName has also been added
|
@swift-ci please test |
|
@swift-ci please test windows |
PhantomInTheWire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @bripeticca just a quick one do you think your changes might fix #8877?
|
@PhantomInTheWire I'll take a look and see if this addresses the issue, thanks for the heads up! |
|
@swift-ci please test |
|
@swift-ci please test windows |
|
Errors seem unrelated to this change, re-triggering tests @swift-ci please test linux |
|
@swift-ci please test macOS |
|
@swift-ci please test linux |
|
@PhantomInTheWire It looks like this change will definitely affect possible solutions for #8877 , but I am skeptical that it will resolve it entirely. If you're interested in taking a look at it still, I would highly recommend doing so once this PR is merged 😄 |
|
sure i'll take a look once this is merged, @bripeticca |
…t stream Some minor fixes to how task output is handled in the SwiftBuildSystemMessageHandler.
|
@swift-ci test |
This PR refactors diagnostic handling in the Swift build system by introducing a dedicated message handler and per-task output buffering to properly parse and emit compiler diagnostics individually.
Key Changes
SwiftBuildSystemMessageHandler
SwiftBuildMessageevents from the build operationPer-Task Data Buffering
taskDataBufferstruct inBuildStateto capture compiler output per task signatureTaskDataBufferstruct allows for usingLocationContextorLocationContext2as a subscript key to fetch the appropriate data buffer for a task, defaulting to the global buffer if no associated task or target can be identified..outputmessages arrive