Enable Importing Framework's WinFX Targets By Default While Allowing Opt-Out

[benvillalobos]

Should fix #12324.

We're running into MANY cases where disabling the default-import of framework's winfx.targets is breaking internal partners. This PR reverses that behavior.

Now it imports framework winfx.targets by default (as this seems to be the normal case), while allowing for "opt-out" of this behavior by setting ImportFrameworkWinFXTargets to false.

[benvillalobos]

#12324 (comment)

Doing some quick testing (thanks @Forgind) it doesn't look like that suggestion works @dsplaisted . I'm thinking at this point we make this opt-in instead of opt-out. So if ImportWinFXTargets isn't set, SDK sets it to true by default. This reverts to functionality before the original issue and provides a workaround for those that run into that. How does this sound?

An interesting observation is that when we force ImportFrameworkWinFXTargets to true, we get a different issue in this repro. @davkean is this something you've seen before? This error happens on 16.8.0 Preview 2.0 [30404.33.master]. It looks unrelated to project system since it works in 16.6.5.

Error:

C:\Users\namytelk\Documents\GitHub\project-system\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\ProjectSystem\VS
\Waiting\VisualStudioWaitIndicator.cs(101,24): error CS8619: Nullability of reference types in value of type '(WaitInd
icatorResult Canceled, T?)' doesn't match target type '(WaitIndicatorResult, T)'. [C:\Users\namytelk\Documents\GitHub\
project-system\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\Microsoft.VisualStudio.ProjectSystem.Managed.VS_q44
qb5gw_wpftmp.csproj]
C:\Users\namytelk\Documents\GitHub\project-system\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\ProjectSystem\VS
\Waiting\VisualStudioWaitIndicator.cs(108,24): error CS8619: Nullability of reference types in value of type '(WaitInd
icatorResult Canceled, T?)' doesn't match target type '(WaitIndicatorResult, T)'. [C:\Users\namytelk\Documents\GitHub\
project-system\src\Microsoft.VisualStudio.ProjectSystem.Managed.VS\Microsoft.VisualStudio.ProjectSystem.Managed.VS_q44
qb5gw_wpftmp.csproj]

[dsplaisted]

I do not think we should ever import these by default if targeting .NET Core. If I'm understanding the regressions correctly, they are all for projects that target .NET Framework. Does that make sense?

[Nirmal4G]

@benvillalobos This is already allowing opt-out. We could remove the entire section in the SDK and replace it with changes from dotnet/msbuild#5425 and dotnet/wpf#2976. This will surely fix a host of problems down the road.

Having this specified in multiple places creates confusion and more regressions. I propose we move them to MSBuild and the Desktop SDK targets (i.e. near to where the WinFX targets are actually imported).

@dsplaisted @rainersigwald Can we reopen (dotnet/msbuild#5425) and discuss this?

[chucker]

I propose we move them to MSBuild and the Desktop SDK targets (i.e. near to where the WinFX targets are actually imported).

Unless I'm missing something: the Desktop SDK targets are for using WinFX (WPF) on top of .NET Core / .NET 5. This issue, however, affects apps that are on .NET Framework, but use Sdk-style projects, e.g. with the MSBuild.Sdk.Extras SDK, not the WindowsDesktop one.

[Nirmal4G]

Unless I'm missing something

The .NET SDK Extras already uses Desktop SDK props/targets if it is present. So, it'll be a cascading effect, if we fix them in MSBuild/WindowsDesktop SDKs.

[dsplaisted]

#12786 updates the logic from this PR.