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

Don't move Properties and Items to ProjectEvaluationFinished if legacy loggers present #6520

Merged
merged 1 commit into from Jun 8, 2021

Conversation

KirillOsenkov
Copy link
Member

@KirillOsenkov KirillOsenkov commented Jun 4, 2021

Fixes #6498

Summary

Switch from the "use the new logic if any logger is present that supports it" to the more conservative "use the old logic if any logger doesn't support the new logic".

Customer impact

Customers who use MSBuild binary logs in conjunction with a legacy logger like one Azure DevOps provides for you would see a crash

MSBUILD : error MSB4017: The build stopped unexpectedly because of an unexpected logger failure. 
Microsoft.Build.Exceptions.InternalLoggerException: The build stopped unexpectedly because of an unexpected logger failure. ---> System.NullReferenceException: Object reference not set to an instance of an object. 
at MSBuild.Logger.BuildConfiguration.Equals(Object obj) 
at System.Collections.Generic.ObjectEqualityComparer`1.Equals(T x, T y) 
at System.Collections.Generic.List`1.Contains(T item) 
at MSBuild.Logger.ProjectTrees.AddTopLevelProject(ProjectStartedEventArgs startedEvent, BuildConfiguration platformConfiguration) 
at MSBuild.Logger.CentralLogger.HandleProjectStarted(Object sender, ProjectStartedEventArgs e) 
at Microsoft.Build.BackEnd.Logging.EventSourceSink.RaiseProjectStartedEvent(Object sender, ProjectStartedEventArgs buildEvent) 

Without this fix, there are two available workarounds:

  1. Disable the crashing logger, or
  2. Set an MSBuild environment-variable escape flag

Regression?

Yes, #6287 caused 16.10 to regress against 16.9 (and former).

Changes Made

Switch from the "use the new logic if any logger is present that supports it" to the more conservative "use the old logic if any logger doesn't support the new logic". Effectively the new logic will now only take place when the binary logger is the only logger.

Testing

Unit tests, inspection of state in debugger.

Risk

Low. Makes the validate escape-hatch codepath more common.

@KirillOsenkov
Copy link
Member Author

/asp run

…y loggers present

Switch from the "use the new logic if any logger is present that supports it" to the more conservative "use the old logic if any logger doesn't support the new logic".

There are legacy loggers such as the Azure DevOps logger that crash if ProjectStartedEventArgs.Properties is null.

Both console loggers also need more work to properly support the new logic.

Effectively the new logic will now only take place when the binary logger is the only logger.
@rainersigwald rainersigwald changed the base branch from main to vs16.10 June 7, 2021 20:46
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Retargeted to 16.10 for servicing; applying Tactics label for servicing consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure Devops pipelines crashing in MSBuild logger, as of 5/25 VS2019 image
4 participants