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

Rendering message about build cancellation (Terminal Logger) #10055

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichalPavlik
Copy link
Contributor

@MichalPavlik MichalPavlik commented Apr 22, 2024

Fixes #8983

Context

When you cancel command-line MSBuild, we send the cancellation signal through the API and also writes status to stdout. But it does it directly in XMake, and TL tends to immediately overwrite that message with status updates, so it's not obvious whether cancellation is in progress.

Changes Made

TL now has instance of cancellation token and writes message on it's own.
TODO: Do similar change in Console Logger and remove write from XMake, otherwise TL can display the message twice in some scenarios.

Testing

Manual:
image

Notes

@MichalPavlik MichalPavlik changed the title Rendering message about build canellation Rendering message about build cancellation (Terminal Logger) Apr 22, 2024
@baronfel
Copy link
Member

Follow-on question - it's always kind of bothered me that a cancelled build is considered failed for purposes of coloration/reporting. Do we know the reason why the build failed? Could we special-case the final build summary to say something like Build [red]cancelled[/red] after {elapsed_time} in that case to be more precise?

@rainersigwald
Copy link
Member

rainersigwald commented Apr 22, 2024

I like having "cancelled" logging but it's "failed" because it didn't "complete without errors", which makes sense IMO.

@MichalPavlik
Copy link
Contributor Author

MichalPavlik commented Apr 22, 2024

@rainersigwald, you mentioned that you would like to have the "build cancellation event". Could you please elaborate? Would it be just a new event or it will be used also as an indication for loggers to stop (instead of cancellation token)? Token is still IMO expected way how to cancel :)

{
if (!_cancellationMessageRendered)
{
Terminal.WriteLine(ResourceUtilities.GetResourceString("AbortingBuild"));
Copy link
Member

Choose a reason for hiding this comment

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

Are we guaranteed to get here between the point the build is cancelled and the last time DisplayNodes is called?

Copy link
Member

Choose a reason for hiding this comment

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

Or rather: between build cancellation and logger shut down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There could be a small chance. I will walk the code once again, and I will probably add another check to RenderBuildResult.

@rainersigwald
Copy link
Member

I was thinking more or less of a new BuildCancelledEventArgs that the engine would raise to indicate to loggers to emit a message like this one. That could then be logged in with precise timing in the binlog too, which may be helpful for some analyses.

@MichalPavlik
Copy link
Contributor Author

It seems there is no infrastructure for emitting events where the context is overall build (or I didn't looked properly). If no one knows how/where to send the event without node/project/target/task context, then I propose to finish this issue with cancellation token and create new issue tracking the "global events" API.

@MichalPavlik
Copy link
Contributor Author

^ @rainersigwald, @ladipro, @rokonec, do you agree?

@ladipro
Copy link
Member

ladipro commented Apr 25, 2024

I would expect the new event to use something analogous to BuildFinishedEventArgs, which is also raised without project context. Hope I'm not misunderstanding or missing anything obvious.

@MichalPavlik
Copy link
Contributor Author

Good tip. It allowed me to discover EngineLoggingServices, which is not in placed in the same directory as the rest logging related types :)

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.

TerminalLogger overwrites "attempting to cancel the build" message
4 participants