Changing Windows_NT -> Windows for enabling TargetPlatformMoniker feature of the sdk for platform specific tfms. #43651

Anipik · 2020-10-20T19:09:57Z

Related to #32451 & #43646
Sdk introduced the TargetPlatformMoniker feature to support the platforms in the tfms string. But the tfms cannot have underscore in their name.

Anipik · 2020-10-21T17:35:18Z

We are changing Windows_NT -> win to use the sdks TargetPlatformMoniker feature. TargetPlatformMoniker does not allow underscore in the tfm string,
I chose win to match with rid used.

The other option could be Windows if we want to avoid abbreviations. @jkotas @stephentoub @terrajobst do you have any preferences here ?

jkotas · 2020-10-21T17:51:22Z

The regular user-facing TargetPlatformMonikers look like Windows,Version=7.0, etc. Is that correct?

I think we should match the public SDK style. The namespace for these is different from the RID namespace and that's by design.

Anipik · 2020-10-21T17:57:22Z

The regular user-facing TargetPlatformMonikers look like Windows,Version=7.0, etc. Is that correct?

Thats correct.

.../Microsoft.Extensions.Hosting/tests/UnitTests/Microsoft.Extensions.Hosting.Unit.Tests.csproj

huoyaoyuan · 2020-10-21T21:09:25Z

Don't forget newly updated doc using this term in #43557

docs/pr-guide.md

safern

Will this cause the SDK to add a dependency to WPF/Winforms for tmfs >= net5.0 where tfm-windows is a valid target framework?

docs/project/garbage-collector-guidelines.md

Anipik · 2020-10-22T17:02:48Z

Will this cause the SDK to add a dependency to WPF/Winforms for tmfs >= net5.0 where tfm-windows is a valid target framework?

Currently the sdk does not know about the -windows platform is there way to verify if sdk will add a dependency ?

ViktorHofer · 2020-10-22T17:20:12Z

Are you sure about that? net5.0-windows should already be something that the SDK parses into the windows TargetPlatformMoniker.

net5.0-windows will be used to expose Windows-specific functionality, like Windows Forms and WPF.

from https://devblogs.microsoft.com/dotnet/announcing-net-5-0-preview-8/

Anipik · 2020-10-22T17:52:00Z

Are you sure about that? net5.0-windows should already be something that the SDK parses into the windows TargetPlatformMoniker.

i was talking about our current way of parsing. After we enabled the new feature it will use the sdk parsing. Currently we parse it our selves.

Anipik · 2020-10-22T18:55:03Z

@ViktorHofer @safern @ericstj can you please review this one ?

ViktorHofer · 2020-10-22T18:56:56Z

i was talking about our current way of parsing. After we enabled the new feature it will use the sdk parsing. Currently we parse it our selves.

Can you please test that scenario before we review/merge this, given that we are planning to depend on that feature (TargetPlatformMoniker) soon?

Anipik · 2020-10-22T19:45:40Z

Can you please test that scenario before we review/merge this, given that we are planning to depend on that feature (TargetPlatformMoniker) soon?

I verified there are no additional references or changes due to the platform.

ViktorHofer · 2020-10-22T19:47:32Z

I verified there are no additional references or changes due to the platform.

@dsplaisted what are the observable changes when the TargetPlatformMoniker is set to windows?

jkotas · 2020-10-22T19:58:47Z

This is not just about -windows. There are other platform monikers used by dotnet/runtime libraries projects that overlap with the official platform monikers in .NET SDK, for example -Android.

ViktorHofer · 2020-10-22T19:59:50Z

Right but AFAIK those aren't yet implemented and observable?

dsplaisted · 2020-10-22T20:09:49Z

Right now using -windows in the TargetFramework doesn't add any references. It will default to Windows 7.0. If you specify UseWindowsFormsorUseWPF, you will get a reference to the Microsoft.WindowsDesktop.Appshared framework. And if you specify a 10.0 version of Windows, you will get aMicrosoft.Windows.SDK.NET.Ref` targeting pack reference which will provide the CSWinRT projections of the Windows WinRT APIs.

Android and iOS aren't supported currently by the SDK, and you'll get errors if you try to target them unless you set some properties (TargetPlatformSupported and TargetPlatformVersionSupported).

ViktorHofer · 2020-10-22T20:16:30Z

Thanks Daniel.

@ericstj what are your thoughts on this? Sounds like we would need to remove additional references if they would come via TargetPlatformMoniker.

ericstj · 2020-10-22T21:25:53Z

@ericstj what are your thoughts on this? Sounds like we would need to remove additional references if they would come via TargetPlatformMoniker.

It doesn't seem all that different to me than the fact that we need to remove all references that come from the SDK since we build those things. I would hope that anything that might be implicitly referenced in is also disable-able.

ViktorHofer · 2020-10-22T21:30:40Z

@dsplaisted if I understood correctly, the Microsoft.WindowsDesktop.App shared framework won't come into play as long as we don't set UseWindowsForms or UseWPF. Is it possible to disable the implicit Microsoft.Windows.SDK.NET.Ref targeting pack?

I would hope that anything that might be implicitly referenced in is also disable-able.

@dsplaisted can we make sure that anything that is referenced based on the TargetPlatformMoniker can be disabled?

dsplaisted · 2020-10-22T21:35:10Z

Generally you can disable these by setting DisableImplicitFrameworkReferences to true. If there's one that's not covered with a flag, we can probably always add a flag if you need to, or it's MSBuild so you can generally work around it anyway.

ViktorHofer · 2020-10-22T22:19:42Z

Thanks for the input Daniel and Eric.

src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj

ViktorHofer

LGTM in general but I didn't review fully. We might want to wait until 11/2 (November's infra rollout) before merging.

ViktorHofer · 2020-10-22T22:28:53Z

I found one usage of TargetOS Windows_NT in Arcade that needs to be updated: https://github.com/dotnet/arcade/blob/cc7b7d1ee1d070cb3ccc2ffd6f4061850ddd232a/src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets#L43.

ViktorHofer · 2020-10-28T14:32:57Z

@Anipik what's the status of this PR? Asking as you didn't add it to the November rollout issue.

Anipik · 2020-10-28T16:38:34Z

@Anipik what's the status of this PR? Asking as you didn't add it to the November rollout issue.

its ready for merging. i wasnt aware of how we do the rollouts. i added the pr to the issue

ViktorHofer · 2020-10-28T16:40:04Z

i added the pr to the issue

Thanks, I will send out the rollout notice to the team today.

aaronfranke · 2020-11-01T05:23:39Z

docs/coding-guidelines/project-guidelines.md

@@ -16,7 +16,7 @@ Below is a list of all the various options we pivot the project builds on:

 - **Target Frameworks:** .NETFramework, .NETStandard, .NETCoreApp
 - **Platform Runtimes:** .NETFramework (aka CLR/Desktop), CoreCLR, Mono
- **OS:** Windows_NT, Linux, OSX, FreeBSD, AnyOS
+- **OS:** windows, Linux, OSX, FreeBSD, AnyOS


Why make Windows be lowercase but the rest capitalized? Is the plan to make the rest lowercase too?

yes the plan is to make them similar to sdk targetPlatform strings

ViktorHofer · 2020-11-03T12:10:13Z

@Anipik as discussed last week, can you please make sure that issues are filed/refcounted for the CI failures?

// EDIT: Ah I see you did but didn't link them here. If you don't mind please link them back in failing PRs so that we know if folks followed up on issues. Thanks

Dotnet-GitSync-Bot added the area-Meta label Oct 20, 2020

Anipik requested a review from marek-safar as a code owner October 21, 2020 19:20

Anipik changed the title ~~[Donot Review][Draft] Changing Windows_NT -> win for enabling TargetPlatformMoniker feature of the sdk for platform specific tfms.~~ Changing Windows_NT -> Windows for enabling TargetPlatformMoniker feature of the sdk for platform specific tfms. Oct 21, 2020

jkotas reviewed Oct 21, 2020

View reviewed changes

.../Microsoft.Extensions.Hosting/tests/UnitTests/Microsoft.Extensions.Hosting.Unit.Tests.csproj Outdated Show resolved Hide resolved

Anipik requested review from ericstj, ViktorHofer and safern October 21, 2020 23:07

ViktorHofer reviewed Oct 21, 2020

View reviewed changes

docs/pr-guide.md Outdated Show resolved Hide resolved

safern reviewed Oct 21, 2020

View reviewed changes

ViktorHofer reviewed Oct 21, 2020

View reviewed changes

docs/project/garbage-collector-guidelines.md Show resolved Hide resolved

Anipik added 2 commits October 22, 2020 10:10

eng change

7cf962d

coreclr change

5274531

Anipik added 6 commits October 22, 2020 10:20

libraries change

40f59ca

tests change

217b860

installer & mono change

94669ab

py files changed

e6b41f4

.md change

f13e42f

.yml change

65847b1

ViktorHofer reviewed Oct 22, 2020

View reviewed changes

src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj Outdated Show resolved Hide resolved

ViktorHofer approved these changes Oct 22, 2020

View reviewed changes

Anipik mentioned this pull request Oct 28, 2020

Infrastructure - Rollout (November 2020) #43351

Closed

Merge branch 'master' into win

f314792

remove redundant targetos property

1f82bbf

Anipik mentioned this pull request Oct 28, 2020

Use targetPlatformMoniker for net5.0 and newer tfms #43965

Merged

aaronfranke reviewed Nov 1, 2020

View reviewed changes

Anipik added 3 commits November 2, 2020 09:57

Merge branch 'master' into win

d251d3a

Windows_NT -> windows after ninja change

9fe0ffc

fixing runtime build failure

5f2bfb6

This was referenced Nov 2, 2020

Test failure: profiler/elt/slowpatheltenter/slowpatheltenter.sh #44055

Closed

Exception in test infrastructure - System.InvalidOperationException: Collection was modified after the enumerator was instantiated #11063

Closed

Anipik merged commit b37f10a into dotnet:master Nov 2, 2020

Anipik deleted the win branch November 2, 2020 22:41

kunalspathak mentioned this pull request Nov 3, 2020

Add threshold on number of files / partition in SPMI collection #44180

Merged

kunalspathak mentioned this pull request Nov 20, 2020

Fix jit-format for Windows dotnet/jitutils#303

Merged

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing Windows_NT -> Windows for enabling TargetPlatformMoniker feature of the sdk for platform specific tfms. #43651

Changing Windows_NT -> Windows for enabling TargetPlatformMoniker feature of the sdk for platform specific tfms. #43651

Anipik commented Oct 20, 2020

Anipik commented Oct 21, 2020 •

edited

Loading

jkotas commented Oct 21, 2020 •

edited

Loading

Anipik commented Oct 21, 2020

huoyaoyuan commented Oct 21, 2020

safern left a comment

Anipik commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020 •

edited

Loading

Anipik commented Oct 22, 2020

Anipik commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020

Anipik commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020 •

edited

Loading

jkotas commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020

dsplaisted commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020 •

edited

Loading

ericstj commented Oct 22, 2020 •

edited

Loading

ViktorHofer commented Oct 22, 2020

dsplaisted commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020

ViktorHofer left a comment •

edited

Loading

ViktorHofer commented Oct 22, 2020

ViktorHofer commented Oct 28, 2020 •

edited

Loading

Anipik commented Oct 28, 2020

ViktorHofer commented Oct 28, 2020

aaronfranke Nov 1, 2020

Anipik Nov 2, 2020

ViktorHofer commented Nov 3, 2020 •

edited

Loading

Changing Windows_NT -> Windows for enabling TargetPlatformMoniker feature of the sdk for platform specific tfms. #43651

Changing Windows_NT -> Windows for enabling TargetPlatformMoniker feature of the sdk for platform specific tfms. #43651

Conversation

Anipik commented Oct 20, 2020

Anipik commented Oct 21, 2020 • edited Loading

jkotas commented Oct 21, 2020 • edited Loading

Anipik commented Oct 21, 2020

huoyaoyuan commented Oct 21, 2020

safern left a comment

Choose a reason for hiding this comment

Anipik commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020 • edited Loading

Anipik commented Oct 22, 2020

Anipik commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020

Anipik commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020 • edited Loading

jkotas commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020

dsplaisted commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020 • edited Loading

ericstj commented Oct 22, 2020 • edited Loading

ViktorHofer commented Oct 22, 2020

dsplaisted commented Oct 22, 2020

ViktorHofer commented Oct 22, 2020

ViktorHofer left a comment • edited Loading

Choose a reason for hiding this comment

ViktorHofer commented Oct 22, 2020

ViktorHofer commented Oct 28, 2020 • edited Loading

Anipik commented Oct 28, 2020

ViktorHofer commented Oct 28, 2020

aaronfranke Nov 1, 2020

Choose a reason for hiding this comment

Anipik Nov 2, 2020

Choose a reason for hiding this comment

ViktorHofer commented Nov 3, 2020 • edited Loading

Anipik commented Oct 21, 2020 •

edited

Loading

jkotas commented Oct 21, 2020 •

edited

Loading

ViktorHofer commented Oct 22, 2020 •

edited

Loading

ViktorHofer commented Oct 22, 2020 •

edited

Loading

ViktorHofer commented Oct 22, 2020 •

edited

Loading

ericstj commented Oct 22, 2020 •

edited

Loading

ViktorHofer left a comment •

edited

Loading

ViktorHofer commented Oct 28, 2020 •

edited

Loading

ViktorHofer commented Nov 3, 2020 •

edited

Loading