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

Bring back ProjectStarted.GlobalProperties #6342

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

KirillOsenkov
Copy link
Member

Some loggers depended on ProjectStartedEventArgs.GlobalProperties being not null and set. It will take a long time to move them to ProjectEvaluationFinished (needs to bump MSBuild dependency to 16.10).

For now log GlobalProperties in both places (ProjectStarted and ProjectEvaluationFinished). Hopefully the deduplication will save us from any significant increase in binlog size.

Fixes #6341

Some loggers depended on ProjectStartedEventArgs.GlobalProperties being not null and set. It will take a long time to move them to ProjectEvaluationFinished (needs to bump MSBuild dependency to 16.10).

For now log GlobalProperties in both places (ProjectStarted and ProjectEvaluationFinished). Hopefully the deduplication will save us from any significant increase in binlog size.

Fixes #6341
Copy link
Contributor

@AndyGerlicher AndyGerlicher left a comment

Choose a reason for hiding this comment

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

Thanks! I verified that this does fix the crash in our 1ES telemetry logger.

@KirillOsenkov
Copy link
Member Author

No discernible impact on build times or binlog size. Dedup hid this overhead under the carpet.

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 13, 2021
@AndyGerlicher
Copy link
Contributor

FYI this fixes a crash in the 1ES telemetry logger. It would be great if we could get this in asap before people start hitting this. Fortunately it only happens on repos with the logger enabled (a few hundred) and using msbuild /bl.

@Forgind Forgind merged commit 8d57dfc into main Apr 13, 2021
@KirillOsenkov KirillOsenkov deleted the dev/kirillo/globalProperties branch April 13, 2021 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProjectStarted.GlobalProperties is expected to be not-null
5 participants