Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add WindowsDesktop warnings and errors #12386

Merged
merged 10 commits into from
Jul 17, 2020
Merged

Add WindowsDesktop warnings and errors #12386

merged 10 commits into from
Jul 17, 2020

Conversation

sfoslund
Copy link
Member

@sfoslund sfoslund commented Jul 8, 2020 •
edited
Loading

Fixes #11241

Depends on #12108 to complete testing (cc @wli3)

@sfoslund sfoslund marked this pull request as ready for review July 10, 2020 18:34
@sfoslund sfoslund requested review from wli3 and dsplaisted July 10, 2020 18:34
dsplaisted
dsplaisted reviewed Jul 10, 2020
View reviewed changes
@wli3
Copy link

wli3 commented Jul 12, 2020

agree with Daniel. The rest is good

@sfoslund
Copy link
Member Author

sfoslund commented Jul 14, 2020 •
edited
Loading

@dsplaisted @wli3 @rainersigwald do you have any more feedback here?

dsplaisted
dsplaisted reviewed Jul 14, 2020
View reviewed changes
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets Outdated
@@ -157,7 +157,7 @@ Copyright (c) .NET Foundation. All rights reserved.
</Target>

<PropertyGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0))">
<_EnableDefaultWindowsPlatform Condition="'$(_EnableDefaultWindowsPlatform)' == ''" >false</_EnableDefaultWindowsPlatform>
<_EnableDefaultWindowsPlatform Condition="'$(_EnableDefaultWindowsPlatform)' == '' and '$(UseWPF)' != 'true' and '$(UseWindowsForms)' != 'true'" >false</_EnableDefaultWindowsPlatform>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still set _EnableDefaultWindowsPlatform to false in all cases. We don't want the legacy MSBuild logic which sets it to Windows 7.0 to be active.

Then Windows Forms or WPF projects will get the WindowsDesktopTargetPlatformMustBeWindows error you're adding if they don't set the target platform to Windows.

We can temporarily set the TargetPlatformIdentifier to Windows here if UseWPF or UseWindowsForms is true, until we have the TargetFramework support for net5.0-windows and the Windows Forms / WPF templates are updated.

Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense. I've made #12492 to track removing this change once the templates are updated.

@sfoslund sfoslund mentioned this pull request Jul 15, 2020
Closed
@sfoslund sfoslund force-pushed the WindowsDesktopWarn branch 2 times, most recently from e47009d to 8b7fe09 Compare July 15, 2020 17:17
@sfoslund sfoslund merged commit e053f40 into dotnet:master Jul 17, 2020
@sfoslund sfoslund deleted the WindowsDesktopWarn branch July 17, 2020 18:05
@ericstj ericstj mentioned this pull request Sep 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rainersigwald rainersigwald rainersigwald left review comments

@dsplaisted dsplaisted dsplaisted approved these changes

@wli3 wli3 Awaiting requested review from wli3

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add warnings for WindowsDesktop targeting
4 participants
@sfoslund @wli3 @dsplaisted @rainersigwald