Scheduler should honor BuildParameters.DisableInprocNode

[cdmihai]

Context

There are two ways in MSBuild to disable the inproc node:

• via the environment variable MSBuildNoInprocNode
• by setting BuildParameters.DisableInprocNode

The implementations of these two, as you'd expect from MSBuild, are totally separate, they have nothing in common. The env var informs the Scheduler to assign all requests to out of proc nodes, regardless of their affinities. And the build parameter informs the NodeManager to just bail out on inproc node creation and return null.

This means that if you set the env var and build a traversal project (dirs.proj, or a solution metaproj file), then all is fine, the scheduler silently redirects them to out of proc nodes. But if you set the build parameter instead of the env var and build the traversal dirs.proj then MSBuild fails with: MSBUILD : error MSB4223: A node of the required type InProc could not be created.

This is causing #6386 to fail some unit tests which ensure that the project cache plugin can function with the inproc node disabled: https://dev.azure.com/dnceng/public/_build/results?buildId=1114344&view=ms.vss-test-web.build-test-results-tab&runId=33953534&resultId=101506&paneView=debug

This is a bit inconsistent and I just don't see the reason for having two separate things. So I made BuildParamters.DisableInProcNode also trigger the Scheduler's ForceAffinityOutOfProc. I doubt it would break anybody, and would actually benefit the users that want to both disable the inproc node via the API (like VS does in certain cases) and avoid node creation exceptions.

Changes Made

The scheduler now forces affinity to out of proc when either the env var is set, or the build parameter is set. It will avoid build failures when the build parameter is set.

Testing

Added / updated unit tests.

[ladipro]

The change looks good, although technically it is breaking so wondering if it shouldn't be behind a changewave.

Also, this is another place where we check the env var only:

msbuild/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs

Line 140 in b02f8de

return _host.BuildParameters.MaxNodeCount > 1 || s_onlyUseOutOfProcNodes;

Would it make sense to fix it also?

[cdmihai]

@ladipro
Which change wave should it be under?

[ladipro]

Which change wave should it be under?

Wave17_0 unless you are intending to backport to 16.x. The main branch is now targeting VS 17.0.

[rokonec]

I suppose cleaning of

msbuild/src/Build.UnitTests/ProjectCache/ProjectCacheTests.cs

Line 445 in 2be2ece

// TODO: remove this branch when the DisableInProcNode failure is fixed by https://github.com/dotnet/msbuild/pull/6400

and similar would be done in another PR, am I right?

[cdmihai]

I suppose cleaning of

msbuild/src/Build.UnitTests/ProjectCache/ProjectCacheTests.cs

Line 445 in 2be2ece

// TODO: remove this branch when the DisableInProcNode failure is fixed by https://github.com/dotnet/msbuild/pull/6400

and similar would be done in another PR, am I right?

Yes, future PR after this one gets merged in.