Change Waves Warnings Don't Report to Error/Warning Window in VS and Log Many Times

[benvillalobos]

The Problem

There is currently no mechanism for MSBuild to log a warning once per build.

• CLI builds and VS builds log differently.
  • The ideal location for us to throw this warning for it to happen once is in BeginBuild. This is complicated because VS has zero loggers attached at the time BeginBuild is called. So while this works for CLI builds, this doesn't work at the moment for VS (see Actual Behavior).
• Should we decide to log the warning once per project, or after MSBuild worker nodes have spawned, we will see these warnings many times unless deduplicated.

Currently, ChangeWaves warnings get logged at evaluation time when setting built-in properties. These warnings have two issues with them:

1. The warning gets logged many times per build
2. The warning does not show in the Error/Warning window in VS.

The Solution

Create a task that runs once per project that checks if the user set some invalid change wave, and warn appropriately. This should cut down the number of warnings.

Steps to Reproduce

in a dev cmd prompt:
set MSBuildDisableFeaturesFromVersion=16.5
devenv someSolution.sln
Build your project and view the output window

Expected Behavior

Error/Warning window shows the thrown warning.

Actual Behavior

---

A Path Forward (old)
The new diagnostic API is supposed to be able to condense errors/warnings of the same type. See "Identifier/Supersedes integers" in the doc linked. I don't think this is something we can implement at the MSBuild layer. Project System could consider adding this to their Error/Warning window loggers.

There is also the option of attaching a logger just before BeginBuild is called, having it only watch for warning events, and detaching it after the BeginBuild call here: https://dev.azure.com/devdiv/DevDiv/_git/VS?path=%2Fsrc%2Fenv%2Fvscore%2Fpackage%2FBuildManager%2FBuildManagerAccessor.cs&version=GBmain&line=1740&lineEnd=1741&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

[benvillalobos]

11/23/2020 Sync Up Meeting Notes

It's been a problem reported by customers that some warnings get posted in the output window that don't show in the warnings list.

The path forward we're considering is adding a logger at the CPS level here.

ideal-beautiful-scenario-thats-probably-wrong.graph

1. In BeginRealBuild()
2. Hook Up a logger
   • This logger should only listen to warnings/errors, as there is a lot of logging of regular messages at that time
3. call BeginBuild()
4. Detach logger

Note: The way the sdk reports their preview warning currently happens once per project, so figuring out a way to log these warnings once per build is also relevant for them.
/cc: @dsplaisted

[lifengl]

The solution mentioned earlier won't work, because BeginBuild is called by solution feature code, which is chained to the solution build manager initial event, and has no knowledge on how projects handle builds. At that time, the solution hasn't tell any project to build, and they have no chance to chain to the logging. After that point, the solution build manager controls which project to be built, and only then, the project to be scheduled to build would hook up logger, and send build submission.

To chain to the solution build logger, project need tell the solution its build submission id. You cannot get build submission id before creating the build submission, and you cannot create build submission before BeginBuild is called. So projects cannot hook up a logger after BegingBuild is called.

[benvillalobos]

@lifengl Could there be some "known value" for submission ID that represents an error/warning that isn't associated with a particular build submission?

[lifengl]

I think it has been discussed through emails earlier. We have two options here:

1, the warning/error must be reported for each build submission if it impacts the project, and the error need be deduplicated in the error list. That matches the current VS design. However, I don't know how the new diagnostic service supports deduplication logic. Because it is completely new, it sounds a reasonable time to design the deduplication logic for that. I think @davkean supports this solution. (It fits VS design, because each project can decide where/how to build. The per-build concept in msbuild doesn't match one solution build in VS. For example, one project wants to use 32 bit msbuild, and other one wants to use 64bit msbuild, a third one want to do build through an azure service, so they might build completely differently on different environments or even on different machines. It is only built-in C#/VB projects are using the default build manager, and share a same build session.

2, update the solution side code to manage the default build manager and handle this scenario in a special way. In that case, there will be no change in project systems.

[lifengl]

It calls MuxLogger.RegisterLogger(submissionId, logger). I think the reason is to split messages from one project to another, so it won't handle messages from building another project in the solution.

Actually BuildManagerAccessor does expose RegisterLogger without a submissionId. However, BeginRealBuild is done by listening IVsUpdateSolutionEvents4.UpdateSolution_BeginUpdateAction event. Without defining some new contract, this code has no idea on what projects will be built, and other projects have no idea on whether they will be involved as well. The code calling BeginBuild (BuildManagerAccessor) and projects are very loosely coupled. Also, it is possible that the project to be built won't use msbuild at all, so it will call BeginBuild and then EndBuild without building anything. For example, maybe all projects are up to date, or it is a single deploy project (which is not msbuild based.) I am not sure whether it makes sense to produce the warning message, because the developer doesn't expect msbuild to be involved. Only BuildManagerAccessor doesn't know that, and it expects it to be no-op originally.

[benvillalobos]

Takeaway from the sync up today: Using this case as an example of what would help other teams (notably deduping the dotnet/sdk preview warning per-project), there's interest in using the new diagnostic API to dedup warnings/errors regardless of happening in different projects.

I'll be in touch with the folks behind the Diagnostic API and see when they can jump into a sync up meeting to continue the discussion.

[benvillalobos]

Takeaway from sync up today: This warning is very specific such that:

1. Customers only hit if they incorrectly set an environment variable
2. Customers will hit once and resolve it, and if they hit it again they'll know what to do
   1.. If they hit it a second time, they'll know what to do

This issue alone isn't enough to justify CPS and project system adding implementations of the diagnostic API, a solution that ultimately is a workaround for this issue.

The core of this issue can be summed up as "There is no way for MSBuild to log a single error/warning in a multi-project solution in both CLI and VS once per build "