SDK upgrade causes compile failure due to not generating VB.NET WPF code behind

[drewnoakes]

Repro:

Clone git@github.com:dotnet/project-system.git.

• Build with SDK 3.1.400-preview-015178 ✔️
• Build with SDK 5.0.100-preview.6.20318.15 ❌

Error:

src\Microsoft.VisualStudio.Editors\OptionPages\GeneralOptionPageControl.xaml.vb(45,13): error BC30451: 'InitializeComponent' is not declared. It may be inaccessible due to its protection level.

The project file defines the item as Page but the Properties pane shows it as None.

Potentially bad globs?

Note that this is a VB.NET file.

Also note that this worked with an earlier version of 5.0.100-preview*, but I don't know which as it was uninstalled when I installed preview 6. I think it was preview 4.

[drewnoakes]

This is now breaking lots of folks who are dogfooding VS.

• .NET 5.0 SDK breaks SDK-based projects building WPF #12739
• Force import WinFX targets project-system#6458
• https://devdiv.visualstudio.com/DevDiv/_git/CPS/pullrequest/264887
• The unit testing team contacted me about this bug as they were blocked too.

[benvillalobos]

To keep all info in one place:
Original Issue
Original MSBuild PR that does not import framework WinFx.targets when ImportFrameworkWinFXTargets is false.
SDK PR that opted out of ImportWinFXTargets by setting ImportFrameworkWinFXTargets to false.
SDK PR that made ImportWinFXTargets optionally disabled on the sdk side (provided opt-out behavior).

My understanding of the issue was that we didn't want framework's copy of winfx.targets for wpf core apps, so we disabled the import. Clearly we were mistaken if this is breaking many people. Should this have been opt-in instead of opt-out?

[dsplaisted]

If I understand correctly, the projects getting broken by this change are ones targeting .NET Framework. While there was discussion in the initial issue of different behavior for dotnet vs msbuild in that case, I think our primary goal was to prevent framework winfx.targets from being imported for projects targeting .NET Core (or .NET 5+).

So I think probably what we would do to fix the regression is to set ImportFrameworkWinFXTargets to false only when the TargetFrameworkIdentifier is not .NETFramework.

If we do want to take a regression and fix the dotnet / msbuild inconsistency, then hopefully our guidance for projects would be that they should just set UseWPF to true instead of ImportFrameworkWinFXTargets. Does that work?

[chucker]

(For those wondering: as a workaround other than uninstalling .NET 5 preview, you can make a global.json at the top level of your solution, filled with:

{
  "sdk": {
    "version": "3.1.302"
  }
}

)

[benvillalobos]

If I understand correctly, the projects getting broken by this change are ones targeting .NET Framework. While there was discussion in the initial issue of different behavior for dotnet vs msbuild in that case, I think our primary goal was to prevent framework winfx.targets from being imported for projects targeting .NET Core (or .NET 5+).

This is also my understanding.

So I think probably what we would do to fix the regression is to set ImportFrameworkWinFXTargets to false only when the TargetFrameworkIdentifier is not .NETFramework.

I agree and am attempting the fix on this repro but I'm unable to test the fix as I can't remote into my work computer at the moment.

If we do want to take a regression and fix the dotnet / msbuild inconsistency, then hopefully our guidance for projects would be that they should just set UseWPF to true instead of ImportFrameworkWinFXTargets. Does that work?

I'll talk to Marc about this. I believe we can fix this regression with your proposed solution.

I opened this PR, can someone verify this fix?

[benvillalobos]

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]

[benvillalobos]

I've updated my PR to import framework winfx targets by default.

[dsplaisted]

As I understand it, we broke SDK-style WPF projects which targeted .NET Framework, without setting UseWPF to true or using the WindowsDesktop SDK. Such projects were likely created before there was any WPF support for .NET Core. They would build with the .NET Framework version of MSBuild due to the implicit winfx.targets import, but would have failed to build with dotnet.

We should avoid breaking such projects, but we should also avoid importing multiple copies of the WPF targets. So I think the default value for ImportWinFXTargets in the SDK should be true ONLY when the TargetFrameworkIdentifier is .NETFramework, and UseWPF is not set to true. That will avoid importing the legacy winfx targets for .NET Core projects, where they won't work, and for .NET Framework projects which are importing the newer WPF targets (which should still work when targeting .NET Framework).

[joeloff]

This should be in RC1