Adjust properties to allow muti-target #2869

wli3 · 2020-04-09T23:54:42Z

without special Sdk="Microsoft.NET.Sdk.WindowsDesktop"

The project could look like

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>WinExe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
    <UseWPF>true</UseWPF>
  </PropertyGroup>

</Project>

wli3 · 2020-04-09T23:55:20Z

packaging/Microsoft.NET.Sdk.WindowsDesktop/Sdk/Sdk.props

@@ -13,13 +13,11 @@ Copyright (c) .NET Foundation. All rights reserved.

  <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.props" />



Most of the sdk.props and sdk.targets are empty now since props and targets are directly imported by SDK

wli3 · 2020-04-09T23:56:26Z

packaging/Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.props

@@ -1,40 +1,6 @@
 <Project>
-  <PropertyGroup>


Moved to target

Instead of moving the code, just add import into WinFX targets, if they are not already imported.

OR Is that pattern not preferred?

wli3 · 2020-04-09T23:57:29Z

packaging/Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.props

-  <ItemGroup Condition=" ('$(EnableDefaultItems)' == 'true') And ('$(UseWPF)' == 'true') And 
-                         ('$(_TargetFrameworkVersionValue)' != '$(_UndefinedTargetFrameworkVersion)') And 
-                         ('$(_TargetFrameworkVersionValue)' >= '$(_WindowsDesktopSdkTargetFrameworkVersionFloor)')">
+  <ItemGroup Condition=" '$(_EnableWindowsDesktopGlobbing)' == 'true' ">


Extract a new property in target to reduce the surface area, since props will always be imported

wli3 · 2020-04-09T23:58:43Z

src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft.WinFx.props

@@ -1,16 +0,0 @@
-<Project>
-  <PropertyGroup>


Moved to WinFx.target

Instead of moving the code, just add import into WinFX targets, if they are not already imported.

OR Is that pattern not preferred?

In this case, props will be imported regardless if the workload is installed or not. So we need to move everything that could be moved.

For more see the description, linked issue. As well as the doc related to the net5.0 design.

You could keep the logic in the props file and import it in the Sdk targets. No need to move the logic and risk losing history. -- Just a suggestion

wli3 · 2020-04-09T23:59:10Z

packaging/Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.targets

+
+    <!-- PropertyGroup flags control if Itemgroups in Microsoft.NET.Sdk.WindowsDesktop.props file is being imported -->
+  <PropertyGroup>
+      <_EnableWindowsDesktopGlobbing Condition=" ('$(EnableDefaultItems)' == 'true') And ('$(UseWPF)' == 'true') And


Please make sure the name looks good @dsplaisted

wli3 · 2020-04-10T00:00:15Z

@dsplaisted also I realized later the first insertion to SDK will fail without SDK side of change. However, I think that is fine, we can merge the SDK side of change with the insertion

vatsan-madhavan

Architecturally you have to understand Pbt.props/Pbt.targets before attempting this surgery. Microsoft.DotNet.Arcade.Wpf.Sdk and dotnet-wpf-test repo has dependencies in this architecture. Any change must be compatible with the test repo, or corresponding changes must be propagated there.

I highly recommend putting up PR’s involving deep refactoring that build cleanly on “build -pack”, or start with a WIP/draft PR for now until things firm-up.

A few hints:

In this repo, never build locally without “-pack”.
Release and Debug build a bit differently - exercise them both
Platform x86 is the default (unlike other repos where x64 is the default).
- x86 is where the bait-and-switch packages are built.
- AnyCpu is the same as x86 - there is no AnyCpu really, because C++/CLI is integral to the build.
Do not build from VS Developer command prompt - use vanilla powershell or cmd
Documentation/wpf.vsconfig + >= Dev16.5
If you set up a branch under “experimental/, you can get CI and PR builds for free. Combine it with “General Testing” channel for experiments that need those.

wli3 · 2020-04-10T04:20:27Z

@vatsan-madhavan let's setup a time to discuss that. I am surprised that .NET Core SDK is not the only public facing dependent.

vatsan-madhavan · 2020-04-10T05:22:44Z

Microsoft.DotNet.Arcade.Wpf.Sdk is internal to WPF repos and it’s used in managing our dual-repo (or 3 repo, if you include test) build orchestration. Think of it as an augmentation to Arcade that includes all of WPF’s build intelligence and overrides, including the ability to do markup compilation from sources before WindowsDesktop SDK is built. That last part is what’s coming from Pbt.*.

This project is going to require some experimenting and getting things right. I’d recommend spending some time building the 3 repos and getting to know Microsoft.DotNet.Arcade.Wpf.Sdk, how local markup compilation works and how we build other wpf repos etc. MSBuild Binlogs are your friend.

Also spend time understanding the packaging architecture - it’s pretty powerful and unique to our repo. There is nothing magic about it, but there is a lot of convention-based stuff going on.

vatsan-madhavan · 2020-04-10T16:53:12Z

Could you add properties to detect multiple inclusion, and simply include WindowsDesktop SDK from with the base Microsoft.NET.Sdk ?

Then if Net.Sdk.WindowsDesktop includes Microsoft.NET.Sdk, then Microsoft.NET.Sdk has to detect this condition (Net.Sdk.WindowsDesktop would have already set a property) and avoid re-inclusion.

And if Microsoft.NET.Sdk is above, and includes Net.Sdk.WindowsDesktop (owning to UseWpf or UseWindowsForms), then Net.Sdk.WindowsDesktop has to detect this condition (probably by leveraging a property set by Microsoft.NET.Sdk) and avoid re-inclusion of Microsoft.NET.Sdk

wli3 · 2020-04-16T01:45:27Z

@dsplaisted @vatsan-madhavan

Could you add properties to detect multiple inclusion, and simply include WindowsDesktop SDK from with the base Microsoft.NET.Sdk ?

Actually I don't think there is a case multiple inclusion will occur. There is graph of import

wli3 · 2020-04-16T05:55:29Z

I updated my PR. This should be a "refactoring". So there should be no visible change from outside (no SDK import method change)

rladuca · 2020-04-16T14:34:19Z

Actually I don't think there is a case multiple inclusion will occur. There is graph of import

Can someone still FrameworkReference? People have been using that idiom to get WPF/WinForms binaries in console apps. This change shouldn't break those projects.

dsplaisted · 2020-04-16T15:14:00Z

Can someone still FrameworkReference? People have been using that idiom to get WPF/WinForms binaries in console apps. This change shouldn't break those projects.

Yes, adding an explicit FrameworkReference should not be impacted by these changes.

wli3 · 2020-05-19T23:07:00Z

Any update?

wli3 · 2020-05-30T18:24:19Z

Ping. It is been more than a month

ryalanms · 2020-06-09T00:26:42Z

I'll be testing this today and tomorrow. Thanks.

ryalanms · 2020-06-09T01:02:36Z

This will require an update to the SDK in all projects in https://github.com/microsoft/WPF-Samples.

dsplaisted · 2020-06-09T01:05:50Z

This will require an update to the SDK in all projects in https://github.com/microsoft/WPF-Samples.

How will they need to update?

The intent is that existing projects should continue to work. It would be good to update the samples to the new project format once we ship an SDK that supports it. However I wouldn't expect the samples to break with this change. Was there a break you ran into?

ryalanms · 2020-06-09T01:09:50Z

Please disregard. I haven't seen anything break, and it looks like the samples are still building against 3.1.

ryalanms · 2020-06-09T05:06:29Z

All samples from the wpf-samples repo I've tested are building successfully against these changes. The updates to the props/targets files did not cause any regressions with building existing .NET 5 applications.

To clarify the goal and staging of this feature: @wli3 is enabling WPF applications to build with Project SDK set to Microsoft.NET.Sdk, but this will come in two separate parts. This is part 1, which refactors Microsoft.NET.Sdk.WindowsDesktop\targets files (Microsoft.WinFX.props/targets, Microsoft.NET.Sdk.WindowsDesktop.props/targets). WPF applications will require <Project = Sdk="Microsoft.NET.Sdk.WindowsDesktop"> until part 2 of @wli3's changes come in.

wli3 · 2020-06-09T05:08:13Z

@vatsan-madhavan could you dismiss your "change required"? So I can merge

wli3 · 2020-06-09T05:10:13Z

also looks like WIP bot get stuck (it was removed across the org). Maybe I need to create a new PR just to workaround that

wli3 · 2020-06-09T17:49:34Z

Continue here #3111

ghost requested review from vatsan-madhavan and rladuca April 9, 2020 23:54

ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Apr 9, 2020

ghost requested a review from SamBent April 9, 2020 23:54

wli3 commented Apr 9, 2020

View reviewed changes

wli3 force-pushed the muti-target-50 branch from e594ab5 to a2e4c1e Compare April 9, 2020 23:58

wli3 commented Apr 9, 2020

View reviewed changes

vatsan-madhavan requested changes Apr 10, 2020

View reviewed changes

vatsan-madhavan assigned fabiant3 Apr 10, 2020

wli3 changed the title ~~Adjust properties to allow muti-target~~ WIP Adjust properties to allow muti-target Apr 10, 2020

dsplaisted mentioned this pull request Apr 10, 2020

Epic: Support targeting iOS and Android with the .NET SDK dotnet/sdk#11165

Closed

25 tasks

Shuffle property location

907077e

wli3 force-pushed the muti-target-50 branch from a2e4c1e to 907077e Compare April 16, 2020 05:54

wli3 requested a review from a team as a code owner April 16, 2020 05:54

ryalanms approved these changes Jun 9, 2020

View reviewed changes

wli3 changed the title ~~WIP Adjust properties to allow muti-target~~ Adjust properties to allow muti-target Jun 9, 2020

wli3 mentioned this pull request Jun 9, 2020

Shuffle property location #3111

Merged

wli3 closed this Jun 9, 2020

wli3 deleted the muti-target-50 branch June 10, 2020 05:56

This was referenced Jun 18, 2020

Support targeting WPF and WindowsForms using Microsoft.NET.Sdk (instead of Microsoft.NET.Sdk.WindowsDesktop) dotnet/sdk#12108

Closed

Muti target 5.0 second part #3155

Merged

ghost locked as resolved and limited conversation to collaborators Apr 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust properties to allow muti-target #2869

Adjust properties to allow muti-target #2869

wli3 commented Apr 9, 2020

wli3 Apr 9, 2020 •

edited

Loading

wli3 Apr 9, 2020

Nirmal4G May 10, 2020

wli3 Apr 9, 2020

wli3 Apr 9, 2020

Nirmal4G May 10, 2020

wli3 May 11, 2020

Nirmal4G May 11, 2020

wli3 Apr 9, 2020

wli3 commented Apr 10, 2020

vatsan-madhavan left a comment

wli3 commented Apr 10, 2020

vatsan-madhavan commented Apr 10, 2020

vatsan-madhavan commented Apr 10, 2020

wli3 commented Apr 16, 2020

wli3 commented Apr 16, 2020

rladuca commented Apr 16, 2020 •

edited

Loading

dsplaisted commented Apr 16, 2020

wli3 commented May 19, 2020

wli3 commented May 30, 2020

ryalanms commented Jun 9, 2020

ryalanms commented Jun 9, 2020

dsplaisted commented Jun 9, 2020

ryalanms commented Jun 9, 2020

ryalanms commented Jun 9, 2020

wli3 commented Jun 9, 2020

wli3 commented Jun 9, 2020

wli3 commented Jun 9, 2020

		@@ -13,13 +13,11 @@ Copyright (c) .NET Foundation. All rights reserved.

		<Import Sdk="Microsoft.NET.Sdk" Project="Sdk.props" />

Adjust properties to allow muti-target #2869

Adjust properties to allow muti-target #2869

Conversation

wli3 commented Apr 9, 2020

wli3 Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented Apr 10, 2020

vatsan-madhavan left a comment

Choose a reason for hiding this comment

wli3 commented Apr 10, 2020

vatsan-madhavan commented Apr 10, 2020

vatsan-madhavan commented Apr 10, 2020

wli3 commented Apr 16, 2020

wli3 commented Apr 16, 2020

rladuca commented Apr 16, 2020 • edited Loading

dsplaisted commented Apr 16, 2020

wli3 commented May 19, 2020

wli3 commented May 30, 2020

ryalanms commented Jun 9, 2020

ryalanms commented Jun 9, 2020

dsplaisted commented Jun 9, 2020

ryalanms commented Jun 9, 2020

ryalanms commented Jun 9, 2020

wli3 commented Jun 9, 2020

wli3 commented Jun 9, 2020

wli3 commented Jun 9, 2020

wli3 Apr 9, 2020 •

edited

Loading

rladuca commented Apr 16, 2020 •

edited

Loading