MSBuild is reusing project instances

[davkean]

This was "fixed" here: #1955, but I'm still repro'ing this.

1. git clone https://github.com/dotnet/project-system/
2. cd project-system
3. git checkout a9e5252cb89e45bc9c5202789bc7effc61b7531c
4. build.cmd
5. open src\ProjectSystem.sln

-- Wait for 1 minute for restore/design-time builds to catch up (when all the errors go away) --

6. While the solution is open, make a whitespace only change to build\Targets\VSL.Imports.targets and Save

Expected: No errors
Actual:

Severity	Code	Description	Project	File	Line	Suppression State
Error		SkipCompilerExecution and ProvideCommandLineArgs should be set before calling the CompileDesignTime target	Microsoft.NetStandard.CSharp.ProjectTemplates	C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\Microsoft\VisualStudio\Managed\Microsoft.Managed.DesignTime.targets	262	
Error		SkipCompilerExecution and ProvideCommandLineArgs should be set before calling the CompileDesignTime target	Microsoft.VisualStudio.AppDesigner	C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\Microsoft\VisualStudio\Managed\Microsoft.Managed.DesignTime.targets	262	
Error		SkipCompilerExecution and ProvideCommandLineArgs should be set before calling the CompileDesignTime target	Microsoft.VisualStudio.ProjectSystem.CSharp.VS	C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\Microsoft\VisualStudio\Managed\Microsoft.Managed.DesignTime.targets	262	
Error		SkipCompilerExecution and ProvideCommandLineArgs should be set before calling the CompileDesignTime target	Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests	C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\Microsoft\VisualStudio\Managed\Microsoft.Managed.DesignTime.targets	262	
Error		SkipCompilerExecution and ProvideCommandLineArgs should be set before calling the CompileDesignTime target	Microsoft.NetCore.FSharp.ProjectTemplates	C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\Microsoft\VisualStudio\Managed\Microsoft.Managed.DesignTime.targets	262	
Error		SkipCompilerExecution and ProvideCommandLineArgs should be set before calling the CompileDesignTime target	Microsoft.VisualStudio.ProjectSystem.VisualBasic.VS	C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\Microsoft\VisualStudio\Managed\Microsoft.Managed.DesignTime.targets	262	
Error		SkipCompilerExecution and ProvideCommandLineArgs should be set before calling the CompileDesignTime target	Microsoft.VisualStudio.ProjectSystem.VisualBasic	C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\Microsoft\VisualStudio\Managed\Microsoft.Managed.DesignTime.targets	262	

The errors come and go.

[cdmihai]

Editing VSL.Imports.targets twice in the repo from the description also triggers the bug.

[davkean]

It looks like you have to wait a while between edits of VSL.Imports.targets to wait for design-time builds to catch up - it seems like only the second edit causes the issue.

[cdmihai]

OK, I think I know what happens here.

• Let's say there are two projects, A->B (A depends on B)
• Both A and B are sent as ProjectInstance objects to the BuildManager in async mode (via BuildSubmission.ExecuteAsync). This eventually leads to A building once, and B building twice, once as a p2p under A (p2p B) and again as a top level project (top B)
• Let's say that top B has an in-memory mutation of its state. This means that the evaluation state (properties, items) of top B differs from the evaluation state of p2p B.
• Let's say that A and top B both need to build target Foo
• Let's say that p2p B builds a different target than top B, so p2p B builds Bar and top B builds Foo. This mimics VS design time builds where the targets between p2p and top are also non-overlapping
• A and top B both get a BuildRequestConfiguration upon submission, on the main node
• A starts building on the child node and gets blocked on p2p B
• the child node (who knows no configuration so far) asks the master for a configuration id for p2p B and master responds with the configuration id of top B. This is incorrect, because the two Bs have different states. Conceptually they cannot be represented by the same configuration inside the engine. This happens because MSBuild only considers global properties, toolsversion, and the project path as unique identifiers for projects, as opposed to considering the entire state.
• the child node then evaluates p2p B from file inside the same configuration id as top B. This leads to the master node and the child node having a configuration with the same id but different ProjectInstance states.
• p2p B correctly executes target Bar and A correctly executes target Foo
• the main node requests the child node to build top B
• the child node uses the state of p2p B to execute target Foo on top B. This leads to an incorrect build execution.

Here's how this looks from the Scheduler's point of view:
SchedulerState_18884.txt

Probable fix is to always add a dummy unique global property to ProjectInstance based builds coming from the outside. Unfortunately some tests are failing with this fix. If this turns out to break engine invariants, then another fix would for CPS to build each project instance in its own separate Begin / End Build session.

[davkean]

@cdmihai That's sounds like a fairly big change to CPS and not something we should tackle lightly. @lifengl @AArnott Thoughts on this?

[cdmihai]

There's about 10 very cryptic tests failing, I'll have to go over each one and see what they mean and imply. For now I don't see other solutions though :(

[AArnott]

This comes from 3+ years of distant memory, but here goes...

CPS's build scheduler supports 'bundles' of build requests being built together in a single build session of MSBuild. That is, between BuildManager.BeginBuild() and EndBuild(), there can be multiple top-level build requests. This allows the MSBuild scheduler to skip targets that were already built within that same build session and overall increases build throughput, which is a good thing.
Some of these build requests simply specify a project file, while others provide the ProjectInstance that must be built.
CPS recognizes that some build requests are "compatible" with each other and can thus share the same build session, while others are incompatible. Two build requests that simply specify the project file by its path are compatible. Two build requests that specify a ProjectInstance are incompatible because of this P2P issue that @cdmihai calls out. As CPS prepares a bundle of requests to include in a single build session, it starts at the top of the CPS build request queue and selects as many compatible requests as it can, then submits that all together.

From @cdmihai's analysis, it sounds like CPS is perhaps considering build requests to be compatible that should not be considered compatible. That would hopefully be a very small fix to that test logic rather than a big change to CPS.

CPS's design-time build manager
CPS' build request compatibility check

[davkean]

Awesome, we should have pulled you in months ago. :)

Let me do a little digging...

[davkean]

Actually, too heads down in some other performance work to looking into this. @lifengl looks like your team might be able to make a change here to fix this?

[lifengl]

Yes, please open a bug and send to us. On Jun 15, 2017, at 7:16 PM, David Kean <notifications@github.com<mailto:notifications@github.com>> wrote: Actually, too heads down in some other performance work to looking into this. @lifengl<https://github.com/lifengl> looks like your team might be able to make a change here to fix this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2123 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ALGWwlFtbdXmMYpZNOwTWEQtPC4Y7KPfks5sEeVdgaJpZM4Ni_Gy>.

[lifengl]

Based on the build error comes out and goes away, the bug is currently moved to 15.5. If your team think it is more impactful, please let us know.

[davkean]

@lifengl Did we get an internal bug for this?

[davkean]

I filed: https://devdiv.visualstudio.com/DevDiv/Managed%20Project%20System/_workItems?id=457737&fullScreen=false&_a=edit.

[fschmied]

@davkean Was this issue closed because it has been fixed or because it is now tracked by devdiv internally? If the latter, could you maybe keep it open until it's really fixed so that the community can track it's status? Thanks!

[davkean]

This issue has been fixed in internal builds. It will appear in the 15.5 milestone called out this in this roadmap: https://github.com/dotnet/project-system/blob/master/docs/repo/roadmap.md#roadmap.