New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot multi-target to netcoreapp < 3.0 and use WindowsDesktop SDK uncoditionally #327

Open
nguerrera opened this Issue Feb 5, 2019 · 12 comments

Comments

Projects
None yet
4 participants
@nguerrera
Copy link
Member

nguerrera commented Feb 5, 2019

  • .NET Core Version: 3.0 Preview 2
  • Windows version: (winver): N/A, build time error
  • Does the bug reproduce also in WPF for .NET Framework 4.8?: N/A

Problem description:

If a project targets netcoreapp1.x or netcoreapp2.x in addition to netcoreapp3.0, and uses WindowsDesktop SDK in order to use WPF conditionally on 3.0, it fails to build. (Importing SDKs conditionally is possible, but difficult to get right and hard to maintain.)

The implicit framework reference is conditioned only on TargetFrameworkIdentifier being .NETCoreApp. It ignores TargetFrameworkVersion.

Minimal repro:
dotnet build this:

<Project Sdk="Microsoft.NET.Sdk.WindowsDesktop">
  <PropertyGroup>
   <TargetFrameworks>netcoreapp1.0;netcoreapp3.0</TargetFrameworks>
   <UseWPF Condition="'$(TargetFramework)' == 'netcoreapp3.0'">true</UseWPF>
  </PropertyGroup>
</Project>

Actual behavior:

error NETSDK1073: The FrameworkReference 'Microsoft.WindowsDesktop.App' was not recognized

Expected behavior:

Successful build

cc @AArnott @dsplaisted @vatsan-madhavan

@vatsan-madhavan

This comment has been minimized.

Copy link
Member

vatsan-madhavan commented Feb 5, 2019

Looks to me like the project is using WindowsDesktop Sdk unconditionally.

This seems to work for me though.

<Project Sdk="Microsoft.NET.Sdk">
  <Import Sdk="Microsoft.NET.Sdk.WindowsDesktop" Project="Sdk.props" Condition="'$(TargetFramework)'=='netcoreapp3.0'"/>
  <PropertyGroup>
    <TargetFrameworks>netcoreapp1.0;netcoreapp3.0</TargetFrameworks>
    <UseWpf>true</UseWpf>
  </PropertyGroup>
  <Import Sdk="Microsoft.NET.Sdk.WindowsDesktop" Project="Sdk.targets" Condition="'$(TargetFramework)'=='netcoreapp3.0'"/>
</Project>
@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented Feb 5, 2019

This also relates to the composability of the WindowsDesktop SDK so things like my Extras can use them....

@nguerrera

This comment has been minimized.

Copy link
Member Author

nguerrera commented Feb 5, 2019

@vatsan-madhavan That is true. Consider that the project in the repro steps is easier to read and maintain, and this tracks a feature request to allow it. I think this is a natural thing that users would expect to work. Even I had to look at the code to figure it out, and the error today does not provide a lot of insight into a solution. Also, having a good error will be complicated to implement. I think it would be simpler and better to allow this.

@nguerrera

This comment has been minimized.

Copy link
Member Author

nguerrera commented Feb 5, 2019

Also, you will get duplicate import warnings with that:

warning MSB4011: "C:\Program Files\dotnet\sdk\3.0.100-preview-010149\Sdks\Microsoft.NET.Sdk\Sdk\Sdk.props" cannot be imported again. It was already imported at "D:\Temp\wpfmt\wpfmt.csproj". This is most likely a build authoring error. This subsequent import will be ignored.
@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented Feb 5, 2019

Related: dotnet/core-sdk#181

@nguerrera

This comment has been minimized.

Copy link
Member Author

nguerrera commented Feb 5, 2019

To avoid the duplicate import warnings, you need:

<Project>
  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" Condition="'$(TargetFramework)' != 'netcoreapp3.0'" />
  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk.WindowsDesktop" Condition="'$(TargetFramework)' == 'netcoreapp3.0'" />
  
  <PropertyGroup>
   <TargetFrameworks>netcoreapp1.0;netcoreapp3.0</TargetFrameworks>
   <UseWPF>true</UseWPF>
  </PropertyGroup>

 <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" Condition="'$(TargetFramework)' != 'netcoreapp3.0'" />
 <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk.WindowsDesktop" Condition="'$(TargetFramework)' == 'netcoreapp3.0'" />
</Project>

I tried the less verbose <Sdk Name="..."> form which can inject top and bottom imports, but it didn't seem to evaluate the conditions correctly.

@nguerrera nguerrera changed the title Cannot multi-target to netcoreapp < 3.0 and use WindowsDesktop SDK Cannot multi-target to netcoreapp < 3.0 and use WindowsDesktop SDK uncoditionally Feb 5, 2019

@nguerrera

This comment has been minimized.

Copy link
Member Author

nguerrera commented Feb 5, 2019

Updated title and description accordingly.

@vatsan-madhavan

This comment has been minimized.

Copy link
Member

vatsan-madhavan commented Feb 5, 2019

I agree with your point about maintainability @nguerrera, and I hadn't originally considered the problem with double-import of Sdk.props. It makes my workaround more complex and even harder to maintain, I think.

I feel like it would be odd to be able to write <Project Sdk=”Microsoft.NET.Sdk.WindowsDesktop”> + <TargetFramwork>netcoreapp2.0<TargetFramework> and have that project function as a regular .NET Core project (and not as a WPF/WinForms project).

We have seen real-world examples of developers inadvertently trying to build WPF applications but getting the Sdk wrong (using Microsoft.NET.Sdk instead of Microsoft.NET.Sdk.WindowsDesktop). If we don’t have a clear error/warning as we do now, I worry that we will start seeing complaints like “My netcoreapp2.0 WindowsDesktop Sdk WPF/WinFroms project is not building correctly - help!”. I’d like to avoid generating this support/bug tail if at all possible.

Would we be ok supporting this with a suppressible warning (assuming it can be implemented easily enough) – something like “Warning XXX – you are no longer building a WindowsDesktop Sdk/WPF/WinForms application because the TargetFramework is <netcoreapp2.0>. You can suppress this warning by setting UseWindowsDesktopSdkAsNetSdkOnUnsupportedFrameworks property to true” ?

@vatsan-madhavan

This comment has been minimized.

Copy link
Member

vatsan-madhavan commented Feb 5, 2019

@AArnott

This comment has been minimized.

Copy link

AArnott commented Feb 5, 2019

I'd rather not have to set some new property to suppress a warning. Why not just use NoWarn?

But ya, the error that is printed by the build gives no indication regarding the target framework being built at the time. This is in fact a general problem with multi-targeting today: when a warning/error is printed, MSBuild only tells us the project that produced it--not it's target framework, which is all-too-relevant for many issues, especially this one. I honestly thought it was the netcoreapp3.0 target that was complaining since that was the one I was working on upgrading when I hit this.
Until such time as MSBuild fixes this so that $(TargetFramework) is included in all errors/warnings, whatever comes up when the user tries what is in the original repro steps should be very clear about what target framework is failing and what the user should do to correct the problem (assuming at least they are multitargeting).

Also: is it really necessary to even have a special SDK just for WindowsDesktop? Can't WPF/WinForms support be built into the regular SDK? I'm not sure which way is the right way here, but it seems that a single SDK would make the resolution to this much easier to identify.

@vatsan-madhavan

This comment has been minimized.

Copy link
Member

vatsan-madhavan commented Feb 5, 2019

NoWarn over a new property seems reasonable.

/cc @DustinCampbell

@nguerrera

This comment has been minimized.

Copy link
Member Author

nguerrera commented Feb 5, 2019

NoWarn today doesn't work for arbitrary MSBuild warnings, each tool has to consume it manually. There is a MSBuildWarningsAsMessages that does almost the same thing. I have argued NoWarn should have the same impact in informal conversation, but I don't think there's a tracking issue. cc @rainersigwald

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment