Import Windows Forms related props and targets into Windows Desktop

[RussKie]

Proposed Changes:

• Establish a mechanism to import Windows Forms related functionality into Windows Desktop SDK via the Windows Forms transport package.
• Add a basic validation of the content of the shipping package in .\eng\WindowsFormsImports.targets.

Enables dotnet/winforms#5183

Screenshots

Test methodology

• Manually locally modifying the installed SDK
• Building winforms -> wpf -> windowsdesktop

[ryalanms]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[vatsan-madhavan]

The props/targets changes (*.GlobalUsings.props/targets) are already merged (into dotnet/winforms) coming into the SDK via this PR is organized as a NuGet/transport-package integration. From what I can tell, those props/targets files are not part of this PR directly.

IMO reviewers curating the WindowsDesktop SDK should be evaluating functional changes made to it by paying attention to the actual changes (like the contents of the new *.GlobalUsings.props/targets files), and looking for both correctness and functional/integration problems in those changes - tasks that are made difficult to perform in this format (i.e., a format that doesn't include the functional changes to the SDK directly/inline in the PR)

I'd suggest putting the changes in *GlobalUsings.props/targets directly into WinFx.targets instead - which is already a shared WPF/WinForms file. If you foresee a lot of WinForms-specific build-logic arising in the near-future, then perhaps creating a new targets file for WinForms next to WinFx.targets (for e.g., System.Windows.Forms.targets) might do the trick.

The transport-package based approach will also lead to readability/maintainability degradation. Today, its easy to look up the sources of the WindowsDesktop SDK in one place, but with this type of forking of content, it will become much harder to look at its sources and understand how it works.

[RussKie]

Thank you for your suggestions.

The transport-package based approach will also lead to readability/maintainability degradation. Today, its easy to look up the sources of the WindowsDesktop SDK in one place, but with this type of forking of content, it will become much harder to look at its sources and understand how it works.

Equally it will also make it difficult to understand and appreciate Windows Forms related concerns if they are split across multiple repositories. It will also sound very odd "If you need to see changes for Windows Forms in Windows Desktop go and make a change in WPF".

[RussKie]

I'd suggest putting the changes in *GlobalUsings.props/targets directly into WinFx.targets instead - which is already a shared WPF/WinForms file. If you foresee a lot of WinForms-specific build-logic arising in the near-future, then perhaps creating a new targets file for WinForms next to WinFx.targets (for e.g., System.Windows.Forms.targets) might do the trick.

IIUIC, WinFx.targets is shipped with VS, which ties us to VS release cadence, and makes our functionality VS version specific. So I don't believe this is a good way forward either.

[RussKie]

We plan to add more Windows Forms specific functionality (e.g. #4605). I will refactor and create more generic System.Windows.Forms.props/targets and replace System.Windows.Forms.GlobalUsings.props/targets when the work on #4605 continues.
This work on the critical path, so I'm merging it as is.

[vatsan-madhavan]

If the .NET Core version of WinFx.targets is included by VS, then it cannot possibly be carried in isolation - that would be an odd choice. Either VS would be carrying the .NET Framework version of WinFX.targets and thus it has no bearing on the contents of WindowsDesktop SDK, or VS would be carrying the .NET Core version and it would necessarily have to carry all the contents of WindowsDesktop SDK no matter where it was produced. Either way, worrying about VS seems moot.

Equally it will also make it difficult to understand and appreciate Windows Forms related concerns if they are split across multiple repositories. It will also sound very odd "If you need to see changes for Windows Forms in Windows Desktop go and make a change in WPF".

That's fair. On balance I'm more interested in ensuring a high-quality product and SDK's like this are used commonly and inadvertent mistakes in these can cause regressions easily.

In the medium term, the appropriate solution would be to refactor the SDK into a separate repo (along with System.Xaml, PresentationBuildTasks) that builds prior to either of dotnet/winforms, dotnet/wpf, and then take a flow from it.

In the short term, there is no perfect solution - either option is somewhat imperfect and I'm inclined to suggest that you optimize towards overall quality and favor easier reviews, overall maintainability etc.

[RussKie]

Equally it will also make it difficult to understand and appreciate Windows Forms related concerns if they are split across multiple repositories. It will also sound very odd "If you need to see changes for Windows Forms in Windows Desktop go and make a change in WPF".

That's fair. On balance I'm more interested in ensuring a high-quality product and SDK's like this are used commonly and inadvertent mistakes in these can cause regressions easily.

If we have a well defined integration point (e.g. System.Windows.Forms.props/targets) then any regressions would be isolated to Windows Forms. We do routinely test builds (when they flow through, but that's a separate issue), so regression will be caught (sufficiently early). IMO dealing with such regressions would be easier if they are in a single repo.
We need to increase our test coverage in Windows Desktop, where we test both Windows Forms and WPF scenarios (e.g. "can create a blank app with top level statements", "can create a blank app with top level statements idsabled", etc.) similar to what the SDK has. This is probably our biggest gap to date.

In the medium term, the appropriate solution would be to refactor the SDK into a separate repo (along with System.Xaml, PresentationBuildTasks) that builds prior to either of dotnet/winforms, dotnet/wpf, and then take a flow from it.

That'd be a great initiative. Though with the current load on both teams I can't see this work happening in the near-med term.
@merriemcgaw / @Pilchie to prioritise.

[merriemcgaw]

That'd be a great initiative. Though with the current load on both teams I can't see this work happening in the near-med term.
@merriemcgaw / @Pilchie to prioritise.

Unfortunately at this stage I'd have to agree.

[vatsan-madhavan]

If we have a well defined integration point (e.g. System.Windows.Forms.props/targets) then any regressions would be isolated to Windows Forms. We do routinely test builds (when they flow through, but that's a separate issue), so regression will be caught (sufficiently early). IMO dealing with such regressions would be easier if they are in a single repo.

These sorts of changes are direct additions into the SDK - they're not additions to the winforms portion of the runtime. What matters here is the maintainability of the WindowsDesktop SDK. I'm interested in preventing WindowsDesktop SDK from generally breaking users during builds of either WinForms or WPF builds, or other generalized scenarios like multitargeting builds, .NET Core builds etc. - and to that end maintainability and review-ability are important.

Take for instance the fact that I tried to review your change and failed to coherently build a mental picture to my satisfaction because this PR doesn't have that overall picture in it. And in order to gain that overall view, I'd have to build your topic branch, get a NuGet package out of it, and then inspect it. If I wanted to look at a diff, then I'd have to diff that locally built NuGet package against a previous version (for e.g, one built out of main) and only then I could review this PR with any level of confidence. I'm simply saying that's not the sort of of reviewability/maintainability loss that seems reasonable.

[Nirmal4G]

Yeah, this is baaaaaad!!! not adding the props/targets/tasks into the Windows Desktop SDK directly will definitely cause lot more pain in the future.

But as Vatsan Madhavan suggested, we can move the Desktop SDK and the PresentationBuildTasks into .NET SDK repo. I see that PBT currently depends on sources of PresentationCore, PresentationFramework, WindowsBase and a whole lot of shared sources! So, to move the Windows Desktop SDK into .NET SDK repo, we could reference them as assemblies and either link and trim them similar to what NuGet.Build.Tasks.Pack does or require Windows Desktop runtime to be installed to use PBT.

IMHO, I prefer link and trim solution than having to have Desktop runtime requirement!

[RussKie]

@Nirmal4G are you volunteering to do the work and see it through?

[Nirmal4G]

are you volunteering to do the work and see it through?

I could try but I don't know how the SDK, WPF and WinForms repo's build system work. Also, from what I understand, this needs changes across all three repos. Even if it's okay with the teams (all three of them), I might need a lot of help to understand all the three repos and its build mechanism to make these changes. Is that right?

So, is it okay for me to take this?

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[RussKie]

I could try but I don't know how the SDK, WPF and WinForms repo's build system work. Also, from what I understand, this needs changes across all three repos. Even if it's okay with the teams (all three of them), I might need a lot of help to understand all the three repos and its build mechanism to make these changes. Is that right?

So, is it okay for me to take this?

The Windows Desktop SDK is jointly owned by Windows Forms team (I'm from it) and the WPF team. The teams have different priorities, engagement models, workloads, etc. As noted above, at this stage the Windows Forms team doesn't any capacity to participate in the restructure of the Windows Desktop SDK; and I suspect the WPF is in the same boat.

Whilst we all appreciate help with a restructure I don't think I'd be able to provide necessary amount of attention or guidance at least until after we'll have shipped .NET 6.0.

[Nirmal4G]

I thought so too. That's why, I've been looking at how the ASP.NET team moved their SDKs into the .NET SDK repo. I think I can do this without any help, since, the template on how to do it, is already there.

If it's okay with the teams, I can open a Draft PR soon.

Do we want to preserve history or just moving the latest HEAD is enough?

[RussKie]

Do we want to preserve history or just moving the latest HEAD is enough?

I don't have a strong preference either way.

I thought so too. That's why, I've been looking at how the ASP.NET team moved their SDKs into the .NET SDK repo. I think I can do this without any help, since, the template on how to do it, is already there.

If it's okay with the teams, I can open a Draft PR soon.

Sure thing. IIRC there was a push to break the SDK into separate optional workloads, e.g. Windows Desktop, ASP.NET Core, etc., so that developer would download and install only relevant workloads.
We can use your draft as a way to start a discussion.

[Nirmal4G]

For the Draft PR, I'll start with the latest HEAD. Then, if we need history preserved, I'll update it with the filtered commits.

there was a push to break the SDK into separate optional workloads…

This is great. But the SDKs all have to be there inorder for this to come to fruition. So, moving the Windows Desktop SDK would definitely push that.

We can use your draft as a way to start a discussion.

Sure.

[Pilchie]

Note that before doing the work to move everything to the SDK repo, it would be good to open an issue there and discuss the proposal with that team.

[ryalanms]

IIUIC, WinFx.targets is shipped with VS, which ties us to VS release cadence, and makes our functionality VS version specific. So I don't believe this is a good way forward either.

There are two WinFX.targets, one that is shipped with VS for .NET Framework and one that is part of .NET Core. WPF will import one or the other depending on the target framework and UseWPF value.

If targeting .NET Framework, the .NET Framework WinFX.targets is imported unless UseWPF is set. If targeting .NET Core, the new WinFX.targets from the WindowsDesktop SDK is always imported.

[RussKie]

@ryalanms I have made changes as per our discussion. The PR description is updated to reflect that.

[Nirmal4G]

Please add the WinFX targets at last.

[RussKie]

Please add the WinFX targets at last.

Why? It is WPF specific, and I'm adding Windows Forms specific targets. They have no relation, and I can't see a compelling reason to inject the new functionality in the middle.

[Nirmal4G]

As I said before, it's just a matter of convention.

Also, we ensure that we don't accidently override something from the WinFX targets, if we import it before. This isn't 100% foolproof but not having the WindowsForms targets here helps a bit.

If I were to do this, I would take the opportunity to refactor this into WPF-specific logic into its own file.

May be Microsoft.NET.Sdk.WindowsDesktop.WPF.<props/targets>!?

[RussKie]

If I were to do this, I would take the opportunity to refactor this into WPF-specific logic into its own file.

May be Microsoft.NET.Sdk.WindowsDesktop.WPF.<props/targets>!?

It is something we discussed offline with @ryalanms. It is his call though.

[Nirmal4G]

We don't need to do it now but planning ahead is not bad though! 🤓🧐