Adding condition for default TargetPlatform properties by sfoslund · Pull Request #5391 · dotnet/msbuild

sfoslund · 2020-06-02T17:23:59Z

Adding a condition for setting default TargetPlatform properties. _EnableDefaultWindowsPlatform will be set from the SDK in a separate PR

dsplaisted · 2020-06-02T20:22:19Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    <TargetPlatformVersion Condition="'$(TargetPlatformVersion)' == '' and '$(_EnableDefaultWindowsPlatform)' != 'false'">7.0</TargetPlatformVersion>
    <TargetPlatformSdkPath Condition="'$(TargetPlatformSdkPath)' == '' and '$(TargetPlatformSdkRootOverride)' != ''">$(TargetPlatformSdkRootOverride)\</TargetPlatformSdkPath>
    <TargetPlatformSdkPath Condition="'$(TargetPlatformSdkPath)' == '' and '$(TargetPlatformIdentifier)' == 'Windows' and '$(OS)' == 'Windows_NT' and '$(MSBuildRuntimeType)' != 'Core'">$([MSBuild]::GetRegistryValueFromView('HKEY_LOCAL_MACHINE\Software\Microsoft\Microsoft SDKs\Windows\v$(TargetPlatformVersion)', InstallationFolder, null, RegistryView.Registry32, RegistryView.Default))</TargetPlatformSdkPath>
    <TargetPlatformSdkPath Condition="'$(TargetPlatformSdkPath)' == ''">$([Microsoft.Build.Utilities.ToolLocationHelper]::GetPlatformSDKLocation($(TargetPlatformIdentifier), $(TargetPlatformVersion)))</TargetPlatformSdkPath>


Will GetPlatformSDKLocation and GetPlatformSDKDisplayName (on line 100) handle empty parameters correctly? Did you do a smoke test that you can still build .NET core projects with these changes and _EnableDefaultWindowsPlatform set to false?

We may want/need to disable setting UseOSWinMdReferences on line 97. We won't be using WinMDs at all on .NET Core.

Yes, TargetPlatformDisplayName just defaults to an empty string and I'm able to build a .NET core console app.

Would you suggest using the same property (_EnableDefaultWindowsPlatform) to disable UseOSWinMdReferences?

Actually, I think we should probably just set UseOSWinMdReferences to false in the .NET SDK (possibly conditioned on .NET 5 or higher).

dsplaisted · 2020-06-02T20:26:14Z

I'd also be interested in hearing if @rainersigwald has a better suggestion for the name than _EnableDefaultWindowsPlatform.

dsplaisted · 2020-06-02T22:10:32Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    <TargetPlatformVersion Condition="'$(TargetPlatformVersion)' == '' and '$(_EnableDefaultWindowsPlatform)' != 'false'">7.0</TargetPlatformVersion>
    <TargetPlatformSdkPath Condition="'$(TargetPlatformSdkPath)' == '' and '$(TargetPlatformSdkRootOverride)' != ''">$(TargetPlatformSdkRootOverride)\</TargetPlatformSdkPath>
    <TargetPlatformSdkPath Condition="'$(TargetPlatformSdkPath)' == '' and '$(TargetPlatformIdentifier)' == 'Windows' and '$(OS)' == 'Windows_NT' and '$(MSBuildRuntimeType)' != 'Core'">$([MSBuild]::GetRegistryValueFromView('HKEY_LOCAL_MACHINE\Software\Microsoft\Microsoft SDKs\Windows\v$(TargetPlatformVersion)', InstallationFolder, null, RegistryView.Registry32, RegistryView.Default))</TargetPlatformSdkPath>
    <TargetPlatformSdkPath Condition="'$(TargetPlatformSdkPath)' == ''">$([Microsoft.Build.Utilities.ToolLocationHelper]::GetPlatformSDKLocation($(TargetPlatformIdentifier), $(TargetPlatformVersion)))</TargetPlatformSdkPath>


Actually, I think we should probably just set UseOSWinMdReferences to false in the .NET SDK (possibly conditioned on .NET 5 or higher).

rainersigwald · 2020-06-03T20:50:30Z

I think you could drop the underscore and make this "public", but I don't feel super strongly about it. My biggest concern is unintended consequences. Can you diff the evaluated properties with and without this set (and no other changes) and see if there are any interesting knock-on effects?

sfoslund · 2020-06-03T22:44:57Z

@rainersigwald sure, here's the diff when building a console app:

With defaults:

TargetPlatformDisplayName = Windows 7.0
TargetPlatformIdentifier = Windows 
TargetPlatformMoniker = Windows,Version=7.0 
TargetPlatformRegistryBase = Software\Microsoft\Microsoft SDKs\Windows
TargetPlatformVersion = 7.0

Without defaults (_EnableDefaultWindowsPlatform set to false):

TargetPlatformDisplayName =
TargetPlatformMoniker = ,Version=
TargetPlatformRegistryBase = Software\Microsoft\Microsoft SDKs\

ProjectCapability item group does not contain BuildWindowsDesktopTarget

rainersigwald

That diff sounds reasonable. I'll leave the underscore to your discretion.

sfoslund · 2020-06-04T16:25:35Z

@dsplaisted do you have strong feelings on dropping or keeping the underscore?

dsplaisted · 2020-06-04T17:14:44Z

@dsplaisted do you have strong feelings on dropping or keeping the underscore?

No, I don't feel strongly. Personally, I would keep it.

Nirmal4G · 2021-08-05T13:19:10Z

@sfoslund @rainersigwald @zivkan

The TargetPlatformMoniker should not be set when Id or version is empty. The empty ,Version= causes failure during build and/or restore in a heavily customised build.

I have recently encountered both NuGet/Home#10423 and invalid platform version...

D:\Projects\Work\MSBuild\BuildTasks
> msbuild -v:m BuildTasks.sln
  Determining projects to restore...
C:\Program Files\Microsoft Visual Studio\2022\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(131,5): error : Invalid platform version '.0'. [D:\Projects\Work\MSBuild\BuildTasks\BuildTasks.sln]

The code paths that are affected by this empty moniker value are in NuGet's parsing logic...

https://github.com/NuGet/NuGet.Client/blob/8b3be1734d595aa591557ab710cad5c4ce661810/src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs#L225-L231

https://github.com/NuGet/NuGet.Client/blob/8b3be1734d595aa591557ab710cad5c4ce661810/src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs#L266-L272

Either we should fix the parsing logic to account for empty versions or we should make sure TPV and TFV shouldn't be empty.

I can do a PR but I need consensus from the MSBuild/NuGet/SDK teams.

ViktorHofer · 2021-08-24T09:36:49Z

Sorry for replying to a one year old PR but what was the reasoning for adding a default TargetPlatformIdentifier i.e. to netstandard2.0?

Adding condition for default TargetPlatform properties

244a220

sfoslund requested review from dsplaisted and rainersigwald June 2, 2020 17:52

dsplaisted reviewed Jun 2, 2020

View reviewed changes

dsplaisted approved these changes Jun 2, 2020

View reviewed changes

Forgind approved these changes Jun 4, 2020

View reviewed changes

rainersigwald approved these changes Jun 4, 2020

View reviewed changes

rainersigwald added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jun 5, 2020

rainersigwald merged commit 3c69de3 into dotnet:master Jun 10, 2020

ghost added this to the current-release milestone Jun 10, 2020

sfoslund deleted the DefaultTargetPlatform branch June 15, 2020 14:51

sfoslund mentioned this pull request Jun 16, 2020

Disable Default Target Platform Properties for .NET 5.0+ dotnet/sdk#12084

Merged

dsplaisted mentioned this pull request Jun 25, 2020

Set default TargetPlatformVersion for Windows dotnet/sdk#11233

Closed

rainersigwald modified the milestones: current-release, MSBuild 16.7 Preview 4 Jul 2, 2020

dsplaisted mentioned this pull request Aug 7, 2020

TargetPlatformMoniker should be blank if TargetPlatformIdentifier is not set #5622

Closed

Conversation

sfoslund commented Jun 2, 2020

Uh oh!

dsplaisted Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

dsplaisted Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

sfoslund Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

dsplaisted Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

dsplaisted commented Jun 2, 2020

Uh oh!

dsplaisted Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

rainersigwald commented Jun 3, 2020

Uh oh!

sfoslund commented Jun 3, 2020

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

sfoslund commented Jun 4, 2020

Uh oh!

dsplaisted commented Jun 4, 2020

Uh oh!

Nirmal4G commented Aug 5, 2021

Uh oh!

ViktorHofer commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants