runtime: DesignTimeBuild fails for System.Utf8String.Experimental src project

So I put together a little test case for this and found some interesting results.

See repro here: https://github.com/ericstj/scratch/tree/master/testReeval

This project uses the same MSBuild evaluation API as design time build. I created 3 properties and set them all from the caller of the evaluation API to global. I created 3 property groups that will reset the value of the property if set to global to local. After reevaluating the project I see interesting results.

The current state has PropA in a props file as the first import of the project file. PropB is set in the project. PropC is set in targets file at the end of the project.

Running this test outputs:

PropA: global    PropB: local    PropC: local

So the property set in in the props file doesn’t get reset. Now the interesting part. Simply adding an empty <PropertyGroup /> before the first import fixes the problem.

PropA: local    PropB: local    PropC: local

I was able to debug and find the problem. When debugging the reevaluation I found that the leading imports are evaluated while none of the SetProperty’s have been set. The implementation of SetProperty is at fault. Here’s the problem: https://github.com/microsoft/msbuild/blob/06567a7988c47e0ffe1ae5ad8831b7dd783a79e0/src/Build/Construction/ProjectRootElement.cs#L1156-L1159

So this updates a property if it exists, otherwise it searches for the first unconditioned PropertyGroup in the project file to add the property to. This is always going to be after any implicit imports as well as explicitly leading imports. This appears to be the long-standing behavior of SetProperty. Contrast that to SetGlobalProperty, which will treat the property as global. Indeed if I change the test app to use SetGlobalProperty the issue goes away.

This is arguably a bug in how design time build sets the property. I believe it should be applying the property as global, to be consistent with how the SDK outer/inner builds work. In most cases this won’t matter since the SDK also supports setting the property locally. I’d recommend that both OmniSharp and VS project system use SetGlobalProperty rather than SetProperty to be consistent with the SDK.

We notice it and are impacted because we are parsing early (in order to support the use of derived properties in the body of the project). I believe our path forward is to move and/or duplicate our parsing to be after the project body. We brought this up a couple times already: @Anipik I believe we need to do this.

“TargetFramework” is supposed to be a user property, you are supposed to be able to have the following:

<TargetFramework>FooBar</TargetFramework>
<TargetFrameworkIdentifier>.NETCoreApp</TargetFrameworkIdentifier>
<TargetFrameworkVersion>v3.1</TargetFrameworkVersion>

And this is supposed to work. Unfortunately, NuGet has some bugs around this.