Check TargetFramework Using Intrinsic Function #5799

benvillalobos · 2020-10-13T23:25:01Z

We had two areas where MSBuild checked TargetFramework based on whether it started with netcore and netstandard.

Ex:

msbuild/src/Directory.BeforeCommon.targets

Line 114 in a710679

    
           <PropertyGroup Condition="$(TargetFramework.StartsWith('netstandard')) or $(TargetFramework.StartsWith('netcore'))">

With net5.0 resolving to net5.0 instead of netcoreapp5.0, this logic would no longer succeed, so here's a quick fix via intrinsic function GetTargetFrameworkIdentifier introduced in #5429.

Some other notes:

There are a few other areas that look like we could use these intrinsic functions, but it would overcomplicate the logic. Here's what I found with rg -iF "StartsWith('net"

src\MSBuild.Bootstrap\MSBuild.Bootstrap.csproj
55:  <Import Project="..\Package\GetBinPaths.targets" Condition="$(TargetFramework.StartsWith('net4'))"/>

src\Directory.Build.props
71:  <PropertyGroup Condition="$(TargetFramework.StartsWith('net4'))">
77:  <PropertyGroup Condition="!$(TargetFramework.StartsWith('net4'))">

src\Directory.Build.targets
141:  <Import Project="$(BUILD_STAGINGDIRECTORY)\MicroBuild\Plugins\MicroBuild.Plugins.IBCMerge.*\**\build\MicroBuild.Plugins.*.targets" Condition="'$(BUILD_STAGINGDIRECTORY)' != '' and $(TargetFramework.StartsWith('net4')) and '$(MicroBuild_EnablePGO)' != 'false'" />

src\Directory.BeforeCommon.targets
19:  <PropertyGroup Condition="$(TargetFramework.StartsWith('net4')) Or $(TargetFramework.StartsWith('net3'))">

eng\BootStrapMSBuild.targets
15:    <BootstrapDependsOn Condition="$(TargetFramework.StartsWith('net4'))">BootstrapFull</BootstrapDependsOn>
16:    <BootstrapDependsOn Condition="!$(TargetFramework.StartsWith('net4'))">BootstrapNetCore</BootstrapDependsOn>

These checks are likely better than

[MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework' and [MSBuild]::GetTargetPlatformVersion('$(TargetFramework)', 1)) == '4'

benvillalobos · 2020-10-14T15:49:45Z

Team Triage: We can't do this until both the hosted azure devops build machines and the internal build pool that does our official builds is updated to 16.8.

Nirmal4G · 2020-10-16T06:54:09Z

PR Title should have TargetFramework instead of TargetPlatform.

benvillalobos · 2020-10-16T15:48:33Z

@Nirmal4G thanks! updated

rainersigwald · 2020-12-22T16:47:19Z

Looking at PRs this morning--this should work now.

rainersigwald · 2020-12-22T16:47:24Z

/azp run

azure-pipelines · 2020-12-22T16:47:33Z

Azure Pipelines successfully started running 1 pipeline(s).

rainersigwald · 2020-12-22T17:50:49Z

Oh, of course: all the Core build job flavors won't work because they build with an old .NET Core SDK. So this is blocked on #5515.

Forgind · 2021-03-08T15:39:00Z

Is this unblocked now that we're on .NET 5?

benvillalobos · 2021-03-08T22:52:56Z

@Forgind Good question. I'll rebase and run the tests again

benvillalobos · 2021-03-10T23:35:56Z

This is ready for review.

src/Directory.BeforeCommon.targets

Forgind · 2021-03-15T23:39:54Z

src/Directory.Build.targets

@@ -69,6 +69,10 @@
    <PackageReference Update="Microsoft.NETCore.App" PrivateAssets="All" />
  </ItemGroup>

+  <PropertyGroup Condition="'$(MonoBuild)' != 'true' and '$(TargetFrameworkIdentifier)' == ''">
+    <TargetFrameworkIdentifier>$([MSBuild]::GetTargetFrameworkIdentifier($(TargetFramework)))</TargetFrameworkIdentifier>


Did you verify this comes before the one from D.BC.t? Also, is $(TargetFrameworkIdentifier) really not used?

Forgind · 2021-06-01T21:06:32Z

Any recent progress on this? Would be nice to finish it or close it until you're ready to come back to it.

src/Tasks/Microsoft.Common.CurrentVersion.targets

rainersigwald · 2021-06-07T19:09:48Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    <TargetFrameworkIdentifier Condition="'$(TargetFrameworkIdentifier)' == ''">$([MSBuild]::GetTargetFrameworkIdentifier($(TargetFramework)))</TargetFrameworkIdentifier>
+    <TargetFrameworkVersion Condition="'$(TargetFrameworkVersion)' == ''">$([MSBuild]::GetTargetFrameworkVersion($(TargetFramework)))</TargetFrameworkIdentifier>


This worries me. Does it really not conflict with something NuGet does?

I had a general concern overriding these, so I added the check to see if it didn't already have a value. Do you think this could mess with something nuget does where they do a similar "set it if it isn't already set" check?

When setting these values here, I considered prefixing this with an underscore for safety. Maybe we have a different naming convention for "Values that likely won't change during a build that we store for msbuild specific purposes"?

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

Forgind · 2021-06-14T15:52:54Z

Team triage: This can break projects that don't have TFI defined and don't expect to define it. We fixed this in other ways in other parts of our repo.

benvillalobos · 2021-06-14T17:22:35Z

At minimum shouldn't this be

<GenAPIShortFrameworkIdentifier Condition="'$([MSBuild]::GetTargetFrameworkIdentifier($(TargetFramework)))' == '.NETFramework'">net</GenAPIShortFrameworkIdentifier>
<GenAPIShortFrameworkIdentifier Condition="'$([MSBuild]::GetTargetFrameworkIdentifier($(TargetFramework)))' == '.NETStandard'">netstandard</GenAPIShortFrameworkIdentifier>
<GenAPIShortFrameworkIdentifier Condition="'$([MSBuild]::GetTargetFrameworkIdentifier($(TargetFramework)))' == '.NETCoreApp'">netstandard</GenAPIShortFrameworkIdentifier>

Or save $([MSBuild]::GetTargetFrameworkIdentifier($(TargetFramework))) into some unique property just before this and check that?

Or should #5792 be closed?

rainersigwald · 2021-06-14T17:23:55Z

Yes, changing the checks to use the property function for our own repo is appropriate. Adding it to common.targets is riskier and I don't think we should do it now.

benvillalobos · 2021-06-14T17:31:57Z

@rainersigwald ah too broadly scoped, got it. Will revert it to mostly the first version of this PR

src/Directory.BeforeCommon.targets

src/Tasks/Microsoft.Common.CurrentVersion.targets

rainersigwald · 2021-06-18T15:38:26Z

src/Directory.BeforeCommon.targets

@@ -110,7 +110,7 @@
    <DefineConstants Condition="$([MSBuild]::IsOSPlatform('windows'))">$(DefineConstants);TEST_ISWINDOWS</DefineConstants>
  </PropertyGroup>

-  <PropertyGroup Condition="'$(MonoBuild)' != 'true' and ($([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'netcoreapp1.0')) or $(TargetFramework.StartsWith('netstandard')))">
+  <PropertyGroup Condition="'$(MonoBuild)' != 'true' and $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp' or $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard'">


src/Directory.BeforeCommon.targets(113,18): error MSB4130: The condition "'$(MonoBuild)' != 'true' and $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp' or $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard'" may have been evaluated incorrectly in an earlier version of MSBuild. Please verify that the order of the AND and OR clauses is written as intended. To avoid this warning, add parentheses to make the evaluation order explicit.

The bug to remove this old warning (#1698) is 4 years old. The warning itself was commited on 2006-06-13 16:04:38.

😔

However, let's go ahead and overparenthesize.

Suggested change

<PropertyGroup Condition="'$(MonoBuild)' != 'true' and $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp' or $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard'">

<PropertyGroup Condition="'$(MonoBuild)' != 'true' and ($([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp' or $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard' )">

benvillalobos · 2021-06-21T18:41:13Z

/azp run

azure-pipelines · 2021-06-21T18:41:23Z

Azure Pipelines successfully started running 1 pipeline(s).

benvillalobos added this to the MSBuild 16.9 milestone Oct 14, 2020

benvillalobos changed the title ~~Check TargetPlatform Using Intrinsic Function~~ Check TargetFramework Using Intrinsic Function Oct 16, 2020

benvillalobos force-pushed the tfm-conditions branch from 4001e78 to 929e40e Compare March 8, 2021 23:05

Forgind reviewed Mar 10, 2021

View reviewed changes

src/Directory.BeforeCommon.targets Show resolved Hide resolved

Base automatically changed from master to main March 15, 2021 20:09

Forgind reviewed Mar 15, 2021

View reviewed changes

Create new private properties for TFI and TFV

76fd297

benvillalobos force-pushed the tfm-conditions branch 2 times, most recently from 15d3ba6 to a6534d0 Compare June 3, 2021 23:48

Condition GenAPI on new property. Change where its defined

e0fd7c1

benvillalobos force-pushed the tfm-conditions branch from a6534d0 to e0fd7c1 Compare June 4, 2021 19:21

Forgind reviewed Jun 7, 2021

View reviewed changes

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

rainersigwald reviewed Jun 7, 2021

View reviewed changes

Update src/Tasks/Microsoft.Common.CurrentVersion.targets

579949a

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

benvillalobos force-pushed the tfm-conditions branch from 88233b3 to 579949a Compare June 7, 2021 23:11

Forgind closed this Jun 14, 2021

benvillalobos reopened this Jun 14, 2021

benvillalobos added 2 commits June 14, 2021 10:45

Check TargetFramework with GetTargetFrameworkIdentifier

aa159e3

Don't modify TFI in currentversion.targets

712912d

rainersigwald requested changes Jun 14, 2021

View reviewed changes

src/Directory.BeforeCommon.targets Outdated Show resolved Hide resolved

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

rainersigwald approved these changes Jun 14, 2021

View reviewed changes

rainersigwald modified the milestones: MSBuild 16.9, 17.0 Jun 15, 2021

rainersigwald approved these changes Jun 18, 2021

View reviewed changes

Forgind approved these changes Jun 18, 2021

View reviewed changes

Add netstandard check for NetCoreBuild properties

cd4390d

benvillalobos force-pushed the tfm-conditions branch from 2c6e350 to cd4390d Compare June 21, 2021 17:43

Forgind 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 26, 2021

AR-May merged commit 4f7de9a into dotnet:main Jun 30, 2021

GangWang01 mentioned this pull request Sep 25, 2023

Update GenApi to use nuget compat functions #5947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check TargetFramework Using Intrinsic Function #5799

Check TargetFramework Using Intrinsic Function #5799

benvillalobos commented Oct 13, 2020

benvillalobos commented Oct 14, 2020 •

edited

Loading

Nirmal4G commented Oct 16, 2020

benvillalobos commented Oct 16, 2020

rainersigwald commented Dec 22, 2020

rainersigwald commented Dec 22, 2020

azure-pipelines bot commented Dec 22, 2020

rainersigwald commented Dec 22, 2020

Forgind commented Mar 8, 2021

benvillalobos commented Mar 8, 2021

benvillalobos commented Mar 10, 2021

Forgind Mar 15, 2021

Forgind commented Jun 1, 2021

rainersigwald Jun 7, 2021

benvillalobos Jun 7, 2021

Forgind commented Jun 14, 2021

benvillalobos commented Jun 14, 2021

rainersigwald commented Jun 14, 2021

benvillalobos commented Jun 14, 2021

rainersigwald Jun 18, 2021

benvillalobos commented Jun 21, 2021

azure-pipelines bot commented Jun 21, 2021

		<TargetFrameworkIdentifier Condition="'$(TargetFrameworkIdentifier)' == ''">$([MSBuild]::GetTargetFrameworkIdentifier($(TargetFramework)))</TargetFrameworkIdentifier>
		<TargetFrameworkVersion Condition="'$(TargetFrameworkVersion)' == ''">$([MSBuild]::GetTargetFrameworkVersion($(TargetFramework)))</TargetFrameworkIdentifier>

	<PropertyGroup Condition="'$(MonoBuild)' != 'true' and $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp' or $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard'">
	<PropertyGroup Condition="'$(MonoBuild)' != 'true' and ($([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp' or $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard' )">

Check TargetFramework Using Intrinsic Function #5799

Check TargetFramework Using Intrinsic Function #5799

Conversation

benvillalobos commented Oct 13, 2020

benvillalobos commented Oct 14, 2020 • edited Loading

Nirmal4G commented Oct 16, 2020

benvillalobos commented Oct 16, 2020

rainersigwald commented Dec 22, 2020

rainersigwald commented Dec 22, 2020

azure-pipelines bot commented Dec 22, 2020

rainersigwald commented Dec 22, 2020

Forgind commented Mar 8, 2021

benvillalobos commented Mar 8, 2021

benvillalobos commented Mar 10, 2021

Forgind Mar 15, 2021

Choose a reason for hiding this comment

Forgind commented Jun 1, 2021

rainersigwald Jun 7, 2021

Choose a reason for hiding this comment

benvillalobos Jun 7, 2021

Choose a reason for hiding this comment

Forgind commented Jun 14, 2021

benvillalobos commented Jun 14, 2021

rainersigwald commented Jun 14, 2021

benvillalobos commented Jun 14, 2021

rainersigwald Jun 18, 2021

Choose a reason for hiding this comment

benvillalobos commented Jun 21, 2021

azure-pipelines bot commented Jun 21, 2021

benvillalobos commented Oct 14, 2020 •

edited

Loading