Skip to content

Conversation

@MichalPavlik
Copy link
Member

@MichalPavlik MichalPavlik commented Mar 28, 2024

Fixes #9290

Context

TaskStartedEventArgs now contains AssemblyName of the assembly that implements the task.

Changes Made

Adding full assembly name to event and propagating the value from upper layers. ITaskExecutionHost interface was removed.

Testing

Manual testing for setting the value. Unit test for serialization

Notes

@MichalPavlik
Copy link
Member Author

@ladipro, is this comment still valid? I would remove the interface as part of this PR
https://github.com/dotnet/msbuild/pull/9948/files#diff-ddd6688a0412c50705a77043f780edaec5bbb785f71dbbb1c4126a7aa8c9fcb7L45

@ladipro
Copy link
Member

ladipro commented Apr 3, 2024

The interface is internal and has only one internal implementation. So it looks like the comment is still valid.

@MichalPavlik
Copy link
Member Author

MichalPavlik commented Apr 3, 2024

I had same opinion, but I wanted to check if there is a plan to implement additional execution host(s)... for whatever reason :)

@MichalPavlik MichalPavlik marked this pull request as ready for review April 4, 2024 12:01
@MichalPavlik MichalPavlik requested a review from ladipro April 4, 2024 13:00
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

I've left a couple of inline comments.

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Overall looks good.

I'd want the BuildEventArgsReader to be backwards compatible before I signoff.
Plus don't forget to update the same class in Viewer repo as well (or create task in viewer repo for that)

Having the diverging assembly path logging handled here or tracking item created would be nice as well.

@MichalPavlik
Copy link
Member Author

MichalPavlik commented Apr 8, 2024

... Plus don't forget to update the same class in Viewer repo as well (or create task in viewer repo for that)

Having the diverging assembly path logging handled here or tracking item created would be nice as well.

@JanKrivanek, I created tracking issue for the viewer: KirillOsenkov/MSBuildStructuredLog#771

@KirillOsenkov
Copy link
Member

I think we need the viewer support ready and merged first, then merge this PR.

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.

Enhance logging to enable correct tracking of task assemblies

6 participants