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

Added task assembly location to TaskStartedEventArgs #9948

Merged
merged 20 commits into from
Apr 19, 2024

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.

src/Build/BackEnd/Components/Logging/ILoggingService.cs Outdated Show resolved Hide resolved
src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs Outdated Show resolved Hide resolved
src/Framework/TaskStartedEventArgs.cs Outdated Show resolved Hide resolved
@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.

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.

Thanks for the fixes!

My last request is to revert the unintended changes to the .resx file

src/Build/Resources/Strings.resx Show resolved Hide resolved
@MichalPavlik MichalPavlik changed the title Added task assembly name to TaskStartedEventArgs Added task assembly location to TaskStartedEventArgs Apr 10, 2024
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.

Thank you!

@MichalPavlik
Copy link
Member Author

Do not merge yet. Viewer update should be first (I'm struggling with the test there).

@MichalPavlik MichalPavlik merged commit 10db417 into main Apr 19, 2024
9 checks passed
@MichalPavlik MichalPavlik deleted the dev/mipavlik/taskstartedevent-taskassembly branch April 19, 2024 10:28
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