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

Adding condition for default TargetPlatform properties #5391

Merged
merged 1 commit into from
Jun 10, 2020
Merged

Adding condition for default TargetPlatform properties #5391

merged 1 commit into from
Jun 10, 2020

Conversation

sfoslund
Copy link
Member

@sfoslund sfoslund commented Jun 2, 2020

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

dsplaisted
dsplaisted reviewed Jun 2, 2020
View reviewed changes
src/Tasks/Microsoft.Common.CurrentVersion.targets
<TargetPlatformIdentifier Condition="'$(TargetPlatformIdentifier)' == ''">Windows</TargetPlatformIdentifier>
<TargetPlatformVersion Condition="'$(TargetPlatformVersion)' == ''">7.0</TargetPlatformVersion>
<TargetPlatformIdentifier Condition="'$(TargetPlatformIdentifier)' == '' and '$(_EnableDefaultWindowsPlatform)' != 'false'">Windows</TargetPlatformIdentifier>
<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>
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

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, 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?

Copy link
Member

Choose a reason for hiding this comment

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

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

@dsplaisted
Copy link
Member

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

dsplaisted
dsplaisted approved these changes Jun 2, 2020
View reviewed changes
src/Tasks/Microsoft.Common.CurrentVersion.targets
<TargetPlatformIdentifier Condition="'$(TargetPlatformIdentifier)' == ''">Windows</TargetPlatformIdentifier>
<TargetPlatformVersion Condition="'$(TargetPlatformVersion)' == ''">7.0</TargetPlatformVersion>
<TargetPlatformIdentifier Condition="'$(TargetPlatformIdentifier)' == '' and '$(_EnableDefaultWindowsPlatform)' != 'false'">Windows</TargetPlatformIdentifier>
<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>
Copy link
Member

Choose a reason for hiding this comment

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

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

@rainersigwald
Copy link
Member

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
Copy link
Member Author

sfoslund commented Jun 3, 2020

@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
rainersigwald approved these changes Jun 4, 2020
View reviewed changes
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

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

@sfoslund
Copy link
Member Author

sfoslund commented Jun 4, 2020

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

@dsplaisted
Copy link
Member

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

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

@rainersigwald 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 rainersigwald merged commit 3c69de3 into dotnet:master Jun 10, 2020
@ghost ghost added this to the current-release milestone Jun 10, 2020
@sfoslund sfoslund deleted the DefaultTargetPlatform branch June 15, 2020 14:51
@sfoslund sfoslund mentioned this pull request Jun 16, 2020
Merged
@dsplaisted dsplaisted mentioned this pull request Jun 25, 2020
Closed
@dsplaisted dsplaisted mentioned this pull request Aug 7, 2020
Closed
@Nirmal4G
Copy link
Contributor

Nirmal4G commented Aug 5, 2021

@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
Copy link
Member

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

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

@Forgind Forgind Forgind approved these changes

@rainersigwald rainersigwald rainersigwald approved these changes

@dsplaisted dsplaisted dsplaisted approved these changes

Assignees
No one assigned
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Milestone
MSBuild 16.7 Preview 4
Development

Successfully merging this pull request may close these issues.
6 participants
@sfoslund @dsplaisted @rainersigwald @Nirmal4G @ViktorHofer @Forgind