Don't double import WinFX targets

[Nirmal4G]

Don't import .NET Framework's WinFX targets, When the SDK's implementation is being used and vice versa.
Also, there's no straight forward way to switch between either WinFX targets as needed. This patch provides such mechanism through ImportFrameworkWinFXTargets.

Reason

We (and possibly many others like me) have a large set of NETCLR WPF projects that can't move to CoreCLR right now. I have recently converted them to sdk-style projects with the WindowsDesktop SDK which includes custom tasks that are meant for Full framework projects. We also have some silverlight projects to maintain and I recently converted them to sdk-style too. Seems they also need WinFX targets.

So, to use the official WindowsDesktop, we want to switch between either WinFX targets for our custom tasks to take effect. This patch is just allowing that. We're migrating our projects and tools to .NET CoreCLR and it'll take some time. Until then, we need this ability to maintain our projects.

Reference: dotnet/msbuild#4948

[rainersigwald]

Please don't merge this until discussion on dotnet/sdk#11606 has come to a decision whether it's the right thing to do. I suspect it won't be necessary.

[vatsan-madhavan]

Typing from memory, please verify before acting on this information.

When using WindowsDesktop SDK -

• When MSBuildRuntimeType==Core, NetFx copy of WinFx.targets is not imported anyway, IIRC.
• When MSBuildRuntimeType==Full, WindowsDesktop SDK's copy of WinFx.props (for which there is no analogue in NetFx) is imported early on and effectively supersedes settings included by NetFx's copy of WinFx.targets.

I'm not sure how this interacts with UseWpf==false && UseWindowsForms==false. My guess is that WinFx.props based supersedence would work irrespective of UseWpf/UseWindowsForms settings.

When not using WindowsDesktop SDK,

• If MSBuildRuntimeType==Core, NetFx's copy of WinFx.targets is not imported; same as before.
• If MSBuildRuntimeType==Full, NetFx's copy of WinFx.targets is likely imported, but WindowsDesktop SDK's copy of WinFx.props is not imported; thus there is no supersedence in place for ensuring that NetFx's copy of WinFx.targets gets turned off overridden.

This only matters when both UseWpf==false && UseWindowsForms==false. Setting either one without including the WindowsDesktop SDK is a warning state in the base NET.Sdk (which can be suppressed).

The net is that I'm not sure why a further change is needed in the WindowsDesktop SDK. Whenever the WindowsDesktop SDK is imported, it effectively nullifies the effect of the NetFx copy of WinFx.targets.

I can see the value of solving this in the base NET.Sdk though when neither WPF nor WinForms are in use by default, but NetFx copy of WinFX.targets comes into play anyway when MSBuildRuntimeType==Full.

[Nirmal4G]

With the .NET Sdk's change I proposed, it'll make import optional but disabled by default.

Whenever the WindowsDesktop SDK is imported, it effectively nullifies the effect of the NETFX copy of WinFX.targets.

In this PR, I just make sure that it's effectively disabled when using WindowsDesktop SDK.

I can see the value of solving this in the base NET.Sdk

They did. But I wanted to make it optional so that I can still use the inbox targets when needed.

[vatsan-madhavan]

Since dotnet/sdk#11606 seems close to being worked out, is this really needed any more?

[Nirmal4G]

We have to ensure that only when WinFX targets from sdk is used, the NETFX WinFX targets gets disabled permanently.

[vatsan-madhavan]

Why? I mean what’s the problem you are trying to solve?

As far as I can tell the current situation is benign.

[Nirmal4G]

The sdk PR solves the issue I'm facing. So, no problem there.

Just as you said, even if we import both of them, the sdk version takes over. I'm just ensuring the correctness of the imports in this PR. That's all.

This property have to be a toggle for both the imports. Like, when NETFX version is disabled, the sdk version is enabled and vice-versa. Both must not be enabled at the same time.

[vatsan-madhavan]

Fair enough!

[Nirmal4G]

PR #3155 broke mine as it removed WinFX.props which I was using it. I have resolved the conflicts by moving the logic into WindowsDesktop.props. No other changes.

@ryalanms Can we merge this now? It's been too long.

[vatsan-madhavan]

If it's indeed an update from master, then can this be merged from master cleanly?

@dotnet/wpf-developers PTAL, review and and consider for merging. There was much effort by @Nirmal4G, @dsplaisted, and myself across this PR and dotnet/sdk#11606 to get this in shape.

@dotnet/dnceng why aren't the checks reporting success correctly for this PR ?

[riarenas]

They look correct to me. Was there a large delay between the build completing and the check reporting success? Or was there a delay between all the build legs completing and the build reporting it had finished?

[Nirmal4G]

All Green for me too! I did resolve the conflicts a few days ago but forgot to push. That might be why the latest merge from master is few commits old from the current one. But there are no conflicts, so it's safe to merge.

[vatsan-madhavan]

Or was there a delay between all the build legs completing and the build reporting it had finished?

This. When I looked, all the individual checks were green at least for a couple of mins, yet the "dotnet-wpf CI" summary was showing "in-progress". A few seconds lag seems reasonable.... but if the status stayed that way throughout the amount of time it took me to compose my comment (not-very-short), take a couple screenshots, highlight them etc., then at least to me it looked non-responsive.

[riarenas]

Azdo builds have delays when reporting that an entire stage has finished, and then another delay when all stages have finished before it reports that a build has completed. This has to do with how the agents report a successful build and it can be frustrating.

Additionally, there are also sometimes delays between a build finishing, and the GitHub checks reflecting it. We had another report of delays when running the checks, so there might have been a transient issue with the communication between AzDO and GitHub. I sent email to the AzDO experts to see if there was a known outage at any point this morning.

[Nirmal4G]

Please merge this as soon as possible, the existing PRs are breaking this one.

[ryalanms]

Thank you for fixing this, @Nirmal4G.

[Nirmal4G]

Finally! 🥳