Skip to content

Extend PropertyInitialValueSetEventArgs and PropertyReassignmentEventArgs#839

Merged
KirillOsenkov merged 8 commits intoKirillOsenkov:mainfrom
YuliiaKovalova:dev/ykovalova/initial_prop_assignment
Feb 12, 2025
Merged

Extend PropertyInitialValueSetEventArgs and PropertyReassignmentEventArgs#839
KirillOsenkov merged 8 commits intoKirillOsenkov:mainfrom
YuliiaKovalova:dev/ykovalova/initial_prop_assignment

Conversation

@YuliiaKovalova
Copy link
Contributor

@YuliiaKovalova YuliiaKovalova commented Dec 6, 2024

Context

related to dotnet/msbuild#11106
fixes: #268

Extend PropertyInitialValueSetEventArgs to include precise location details (file, line, column) and modify the formatting approach for PropertyReassignmentEventArgs.

@YuliiaKovalova YuliiaKovalova changed the title Add ExtendedPropertyInitialValueSetEventArgs that contains information about property location Refine PropertyInitialValueSetEventArgs and PropertyReassignmentEventArgs Dec 11, 2024
@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review December 11, 2024 15:51
@YuliiaKovalova YuliiaKovalova changed the title Refine PropertyInitialValueSetEventArgs and PropertyReassignmentEventArgs Extend PropertyInitialValueSetEventArgs and PropertyReassignmentEventArgs Dec 11, 2024
Copy link
Collaborator

@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!

@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/initial_prop_assignment branch from d21e881 to 24f9a6e Compare January 27, 2025 11:20
@KirillOsenkov
Copy link
Owner

@YuliiaKovalova just to confirm, I think we should test the following scenarios.

I suggest let's record two binlogs from your PR (dotnet/msbuild#11106), one with no special settings (1.binlog), and one with property tracking enabled (2.binlog). Let's also have a regular old binlog of version 24 (3.binlog).

  • this build of the viewer opens 1.binlog
  • this build of the viewer opens 2.binlog
  • this build of the viewer opens 3.binlog
  • old viewer opens 1.binlog
  • old viewer opens 2.binlog

If it all works fine, let me know and I'll merge and publish a new viewer version. After that we can merge dotnet/msbuild#11106

Thanks a lot!

@YuliiaKovalova
Copy link
Contributor Author

@YuliiaKovalova just to confirm, I think we should test the following scenarios.

I suggest let's record two binlogs from your PR (dotnet/msbuild#11106), one with no special settings (1.binlog), and one with property tracking enabled (2.binlog). Let's also have a regular old binlog of version 24 (3.binlog).

  • [*] this build of the viewer opens 1.binlog
  • [*] this build of the viewer opens 2.binlog
  • [*] this build of the viewer opens 3.binlog
  • [*] old viewer opens 1.binlog
  • old viewer opens 2.binlog NOPE

If it all works fine, let me know and I'll merge and publish a new viewer version. After that we can merge dotnet/msbuild#11106

Thanks a lot!

The old viewer opens 2.binlog, but doesn't contain info about PropertyAssignment and UninitializedPropertyRead because I moved raw messages to LogViewer

"PropertyAssignment": "Property initial value: $({0})=\"{1}\". Source: {2}",
and stopped passing a message from msbuild side for these two
https://github.com/dotnet/msbuild/blob/bdadaea8aa91de921767a5686592dfd86d62c299/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs#L589

Should I handle it somehow differently or it's ok to have this breaking change for the sake of perf savings?

@KirillOsenkov KirillOsenkov merged commit df8221f into KirillOsenkov:main Feb 12, 2025
@KirillOsenkov
Copy link
Owner

No, that's fine I think. Thanks a lot!

@KirillOsenkov
Copy link
Owner

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.

Track property assignment the same way as property reassignment

3 participants