DesignTimeBuild fails for System.Utf8String.Experimental src project

[eerhardt]

See the discussion here: #33357 (comment).

With #33357, the src project for System.Utf8String.Experimental does not build successfully in Visual Studio. Using the "Build Logging" window, I noticed that the netcoreapp5.0-Windows|Unix design-time builds are failing.

The project needs to use the condition:

<ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">

in order to reference the runtime. This is because it also needs to build for netcoreapp3.0, so it can't use the more typical $(TargetsNetCoreApp) property.

When looking at the binlog, I noticed an unusual condition not working:

This looks like $(TargetFramework) is still set to netcoreapp5.0-Windows_NT, but that should have been stripped before this was executed.

One potential fix for this is to disable this error during DesignTimeBuilds. I've done that locally, and it allows the src project to build successfully.

@joperezr @Anipik

[joperezr]

cc @davkean

This is the TL;DR

We are seeing this weird behavior in VS that only occurs in design-time builds so you may know what is going on. We currently kind of overload the TargetFramework property because we also embed the RID into it, for example <TargetFrameworks>netcoreapp5.0-Windows_NT</TargetFrameworks>. This way, we can benefit from MSBuild built-in logic to dispatch to Inner builds. After that, on each inner build we decompose this target framework to remove the RID specific portion which we use for other things and set TargetFramework to a regular TFM. We do that here:

<Project TreatAsLocalProperty="TargetFramework">  
  <PropertyGroup>
    <DotNetBuildTasksTargetFrameworkSdkAssembly Condition="'$(DotNetBuildTasksTargetFrameworkSdkAssembly)' == '' AND '$(MSBuildRuntimeType)' == 'core'">..\tools\netcoreapp2.1\Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.dll</DotNetBuildTasksTargetFrameworkSdkAssembly>
    <DotNetBuildTasksTargetFrameworkSdkAssembly Condition="'$(DotNetBuildTasksTargetFrameworkSdkAssembly)' == '' AND '$(MSBuildRuntimeType)' != 'core'">..\tools\net472\Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.dll</DotNetBuildTasksTargetFrameworkSdkAssembly>
    <_OriginalTargetFramework>$(TargetFramework)</_OriginalTargetFramework>
  </PropertyGroup>
  
  <PropertyGroup Condition="$(TargetFramework.Contains('-'))">
    <TargetFrameworkSuffix>$(TargetFramework.SubString($([MSBuild]::Add($(TargetFramework.IndexOf('-')), 1))))</TargetFrameworkSuffix>
    <TargetFramework>$(TargetFramework.SubString(0, $(TargetFramework.IndexOf('-'))))</TargetFramework>
  </PropertyGroup>
</Project>

This is inside a props file that should always get imported by the project since this props file comes from a package dependency of the project, and it works as expected when building inside VS and also when building on the command line. That said, it doesn't seem to work on design-time builds as the TargetFramework seems to still contain the RID part on it during the build. Is there a difference on how design-time builds evaluate projects or which packages are allowed to provide props file for evaluation of the project? We tried collecting a binlog from a designtime build that reproed this, and funny enough is that apparently the Evaluation node doesn't contain an evaluation for this project, so I wanted to see if this rang any bells to you.

[joperezr]

FYI: @ericstj

[davkean]

TreatAsLocalProperty will only be respected for the file that contains it. Elsewhere whereever TargetFramework is used it will always contain the RID.

[davkean]

"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.

[joperezr]

TreatAsLocalProperty will only be respected for the file that contains it. Elsewhere whereever TargetFramework is used it will always contain the RID.

I'm actually not very familiar with how TreatAsLocalProperty is supposed to work, these props files were written by @Anipik, but if what you are saying is true, then how does this work fine and do the right thing in a regular VS build, and when building from command line?

[davkean]

Probably because no one is passing it as a global property. Try this:

msbuild myproj.csproj /p:TargetFramework=Whatever

And this will break. This is how design-time builds and evaluations work.

[ericstj]

We pass as a global property between outer and inner builds, and have done so for years (previously with Configuration). TreatAsLocalProperty permits local reassigment and the new value remains for rest of evaluation. We can do some tests to further illustrate this.

[ericstj]

/cc @rainersigwald

[davkean]

Yep, you are right, docs state:

If the property is listed in the TreatAsLocalProperty value, the global property value doesn't override property values that are set in that file and any subsequent imports.

No idea why that isn't working in design time builds, would need to see an evaluation.

1. Are you setting it early enough?
2. Do we push CPS evaluations to build logger? @panopticoncentral?

Can you make sure the project is saved on disk and try again to grab the logs? I think that should include the evaluation in the build (maybe trigger by dropping a source file under a glob).

Either way, the approach to do this is to have a custom TargetFramework value and fill in the TFM fields, this is what it was designed for. However, the NuGet command-line restore might block this which is being fixed for .NET 5.

[davkean]

That TargetFramework split will also break in .NET 5: dotnet/designs#92

[ericstj]

We understand that, we'll either leverage the new feature or change our delimiter.

[panopticoncentral]

Unfortunately, the project system tools can't capture CPS evaluations because there isn't a hook for it. I've got it on the backlog but haven't had the time to do something about it, since it requires adding a CPS API.

[ericstj]

I believe @Anipik has a repro directly calling the MSBuild Project evaluation API and exhibiting the bad behavior. I believe it evaluates project, sets TargetFramework then calls ReevaluateIfNecessary https://docs.microsoft.com/en-us/dotnet/api/microsoft.build.evaluation.project.reevaluateifnecessary?view=netframework-4.8#Microsoft_Build_Evaluation_Project_ReevaluateIfNecessary. When doing so TargetFramework isn’t reset correctly.

[ericstj]

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.

[ericstj]

@ViktorHofer and @Anipik are looking into fixing this.

[Anipik]

Closing this as it was fixed here #34025