Restore: Prefer `TargetFramework` over `TargetFrameworks` when it is specified as a global property. #4587

nkolev92 · 2022-04-21T00:35:54Z

Bug

Regression? Last working version:

Description

NuGet/Home#11653 contains details about the scenario at hand, but the gist is that:

$ dotnet new maui
$ dotnet build -f:net6.0-ios -r ios-arm64

does not work because nuget prefers TargetFrameworks over TargetFramework, so it essentially "tries" to do a restore for net6.0-android + ios-arm64 which obviously isn't something that works.

This change allows NuGet to detect the user intent in the scenarios like the above, and only restore for the framework passed on the commandline.

The change boils down to NuGet preferring the TargetFramework when it is specified on the commandline, as a global property.

While working on this I discovered NuGet/Home#11761, which basically means static graph can't support the equivalent at this point.

Given that this change requires multiple teams to validate it's good enough, I've added a means to disable it in case something does go really wrong while we're attempting that validation. _DisableNuGetRestoreTargetFrameworksOverride.

PR Checklist

nkolev92 · 2022-04-21T01:02:08Z

@rolfbjarne and @dsplaisted - please take a look.

I've made NuGet prefer the global TargetFramework property when set.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetRestoreTests.cs

rolfbjarne · 2022-04-21T05:57:23Z

CC @jonathanpeppers

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

nkolev92 · 2022-04-21T16:36:17Z

Thanks for the review @jonathanpeppers and @rolfbjarne

Addressed your feedback.

dsplaisted · 2022-04-26T14:45:04Z

@rainersigwald Is there a better way to check whether a property is a global property than to do a separate MSBuild task invocation? Would something like this work to do it in simple evaluation?

<PropertyGroup>
  <_TargetFrameworkBackup>($TargetFramework)</TargetFrameworkBackup>
  <TargetFramework>FakeOverriddenValue</TargetFramework>
  <TargetFrameworkIsGlobal Condition="'$(TargetFramework)' == '$(TargetFrameworkBackup)'">true</TargetFrameworkIsGlobal>
  <TargetFramework>$(_TargetFrameworkBackup)</TargetFramework>
</PropertyGroup>

rainersigwald · 2022-04-26T14:55:12Z

@rainersigwald Is there a better way to check whether a property is a global property than to do a separate MSBuild task invocation?

There's now (since 16.5) an API that will allow doing it inside a task: dotnet/msbuild#4939. @nkolev92 would that let you do this inside the NuGet tasks themselves?

Would something like this work to do it in simple evaluation?

<PropertyGroup>
  <_TargetFrameworkBackup>($TargetFramework)</TargetFrameworkBackup>
  <TargetFramework>FakeOverriddenValue</TargetFramework>
  <TargetFrameworkIsGlobal Condition="'$(TargetFramework)' == '$(TargetFrameworkBackup)'">true</TargetFrameworkIsGlobal>
  <TargetFramework>$(_TargetFrameworkBackup)</TargetFramework>
</PropertyGroup>

This does work in simple evaluation, but beware: if you put the same property settings inside a target, it does not work.

nkolev92 · 2022-04-26T21:50:21Z

That API could work. I think we use it in static graph scenarios already.

Right now we only use that to decide which frameworks to run inner builds for. I'd probably have to add an extra task to just get the effective TargetFramework(s).

jeffkl · 2022-05-02T18:00:03Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

+    <MSBuild
+        BuildInParallel="$(RestoreBuildInParallel)"
+        Condition=" '$(TargetFrameworks)' != '' "
+        Projects="$(MSBuildThisFileFullPath)"


I would put some comments in here that you're "building" NuGet.targets just to detect if a global property is set. More explanation of how this target works would be very helpful.

jeffkl · 2022-05-02T18:04:10Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetRestoreTests.cs

+
+            // Act
+            var additionalArgs = useStaticGraphEvaluation ? "/p:RestoreUseStaticGraphEvaluation=true" : string.Empty;
+            var result = _msbuildFixture.RunDotnet(pathContext.SolutionRoot, args: $"restore a.csproj {additionalArgs} /p:TargetFramework=\"net5.0\"", ignoreExitCode: true);


The result of this restore will only work for one framework and subsequent builds without a restore would fail right? Is it not possible to just restore a whole project regardless of target framework(s) and just let the build be scoped? We can remove global properties when generating the ProjectSpec items

https://docs.microsoft.com/en-us/dotnet/api/microsoft.build.tasks.msbuild.removeproperties?view=msbuild-17-netcore

Yeah, it'd only restore/build for that specific framework.

When a RID is passed, we simply can't know whether we'd need to include for all or only a specific framework, so we only restore for that framework.

nkolev92 · 2022-05-10T21:02:20Z

Ready for another look @rolfbjarne @jonathanpeppers @dsplaisted @NuGet/nuget-client

src/NuGet.Core/NuGet.Build.Tasks/GetGlobalPropertyValueTask.cs

zivkan · 2022-05-13T13:37:30Z

src/NuGet.Core/NuGet.Build.Tasks/GetGlobalPropertyValueTask.cs

+            IReadOnlyDictionary<string, string> msBuildGlobalProperties = null;
+
+            // MSBuild 16.5 added a new interface, IBuildEngine6, which has a GetGlobalProperties() method.  However, we compile against
+            // Microsoft.Build.Framework version 4.0 when targeting .NET Framework, so reflection is required since type checking


Is there a reason we're building with such an old version of Microsoft.Build.Framework? Does nuget.exe even work without a recent-ish msbuild.exe? Or is it to avoid ilmerging Microsoft.Build.Framework.dll into nuget.exe, since I guess version 4.0 is in the GAC?

zivkan · 2022-05-13T13:37:30Z

src/NuGet.Core/NuGet.Build.Tasks/GetGlobalPropertyValueTask.cs

+            IReadOnlyDictionary<string, string> msBuildGlobalProperties = null;
+
+            // MSBuild 16.5 added a new interface, IBuildEngine6, which has a GetGlobalProperties() method.  However, we compile against
+            // Microsoft.Build.Framework version 4.0 when targeting .NET Framework, so reflection is required since type checking


Is there a reason we're building with such an old version of Microsoft.Build.Framework? Does nuget.exe even work without a recent-ish msbuild.exe? Or is it to avoid ilmerging Microsoft.Build.Framework.dll into nuget.exe, since I guess version 4.0 is in the GAC?

NuGet.exe uses whichever version of msbuild is available on their machine.
We don't/won't merge msbuild because it is used to discover imports and all that.

actually, I misunderstood your question.

Not 100% sure if we can update. I can look into it, but we're doing this in a bunch of different places so I just largely followed that logic.

Yeah, it might be reasonable to rebaseline, since I suspect you don't support running modern nuget.exe against dev11. But I don't think it's urgent.

you don't support running modern nuget.exe against dev11

NuGet.exe restore help unfortunately lists a lot of msbuild versions.

We don't really explicitly talk about what's supported vs what's not.

It's a ymmv type of thing.

…orks

nkolev92 · 2022-05-20T19:24:55Z

@NuGet/nuget-client

Hoping to get some reviews here so I can merge? :)

zivkan

I'm curious if there's a measurable performance degradation with this change? The PR adds a new target, which calls a C# task. But MSBuild is an interpreted scripting language, so calling targets is a lot more expensive than calling methods in a JITed language, and the task uses reflection.

I hope it's too small to be measurable, but we seem to be concerned about the pdb PR's impact.

nkolev92 · 2022-05-27T16:30:37Z

I'm curious if there's a measurable performance degradation with this change? The PR adds a new target, which calls a C# task. But MSBuild is an interpreted scripting language, so calling targets is a lot more expensive than calling methods in a JITed language, and the task uses reflection.

It registers as 1ms only. #4587 (comment)

Note that this is only called on the commandline and not in VS, so the concerns are lesser.

jeffkl

There was no CI build for this change so I kicked one off

nkolev92 marked this pull request as ready for review April 21, 2022 01:00

nkolev92 requested a review from a team as a code owner April 21, 2022 01:00

rolfbjarne previously approved these changes Apr 21, 2022

View reviewed changes

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetRestoreTests.cs Outdated Show resolved Hide resolved

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetRestoreTests.cs Outdated Show resolved Hide resolved

jonathanpeppers reviewed Apr 21, 2022

View reviewed changes

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets Outdated Show resolved Hide resolved

nkolev92 dismissed rolfbjarne’s stale review via 2b04ad9 April 21, 2022 16:34

nkolev92 mentioned this pull request Apr 21, 2022

Restore with static graph evaluation fails with an NRE when a TargetFramework is specified NuGet/Home#11761

Open

jeffkl reviewed May 2, 2022

View reviewed changes

nkolev92 force-pushed the dev-nkolev92-respectTargetFrameworkOverrides branch from 2b04ad9 to 6bf125a Compare May 3, 2022 00:35

nkolev92 requested a review from jeffkl May 6, 2022 00:22

jonathanpeppers reviewed May 10, 2022

View reviewed changes

src/NuGet.Core/NuGet.Build.Tasks/GetGlobalPropertyValueTask.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Build.Tasks/GetGlobalPropertyValueTask.cs Outdated Show resolved Hide resolved

rolfbjarne previously approved these changes May 11, 2022

View reviewed changes

nkolev92 dismissed rolfbjarne’s stale review via 8593454 May 13, 2022 00:38

nkolev92 force-pushed the dev-nkolev92-respectTargetFrameworkOverrides branch from 93ae383 to 8593454 Compare May 13, 2022 00:38

nkolev92 requested a review from a team May 13, 2022 00:39

zivkan reviewed May 13, 2022

View reviewed changes

nkolev92 requested a review from zivkan May 16, 2022 21:20

nkolev92 added 6 commits May 20, 2022 12:24

Respect target framework overrides

559edfc

cleanup

cb2e946

Try again. Are we better?

519ccee

[skip ci] Cleanup

6bcac16

[skip ci] Cleanup

92dc4ff

add more comments

7dc6ae9

nkolev92 added 14 commits May 20, 2022 12:24

cleanup

5e5fd92

add a few tests

ae2964a

cleanup..maybe

be36303

Cleanup the tests, ensure tests are run for static graph too.

5d7ce54

cleanup the targets...all tests are passing

f9d89f2

cleanup

7dcb47a

add a means to disable it

8df4a46

Cleanup

3cc9a85

no need to call separate targets for targetframework and targetframew…

af8541f

…orks

cleanup

1979cda

cleanup

ca9f0f6

cleanup

f2e0e57

Add logging, add tests

20f6f7d

use more efficient reflection

587dd8b

nkolev92 force-pushed the dev-nkolev92-respectTargetFrameworkOverrides branch from 8593454 to 587dd8b Compare May 20, 2022 19:24

zivkan approved these changes May 27, 2022

View reviewed changes

jeffkl approved these changes May 27, 2022

View reviewed changes

nkolev92 merged commit 0da9fda into dev May 27, 2022

nkolev92 deleted the dev-nkolev92-respectTargetFrameworkOverrides branch May 27, 2022 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore: Prefer `TargetFramework` over `TargetFrameworks` when it is specified as a global property. #4587

Restore: Prefer `TargetFramework` over `TargetFrameworks` when it is specified as a global property. #4587

nkolev92 commented Apr 21, 2022 •

edited

Loading

nkolev92 commented Apr 21, 2022

rolfbjarne commented Apr 21, 2022

nkolev92 commented Apr 21, 2022

dsplaisted commented Apr 26, 2022

rainersigwald commented Apr 26, 2022

nkolev92 commented Apr 26, 2022

jeffkl May 2, 2022

jeffkl May 2, 2022

nkolev92 May 2, 2022

nkolev92 commented May 10, 2022

zivkan May 13, 2022

zivkan May 13, 2022

nkolev92 May 13, 2022

nkolev92 May 13, 2022

rainersigwald May 13, 2022

nkolev92 May 16, 2022

nkolev92 commented May 20, 2022

zivkan left a comment

nkolev92 commented May 27, 2022 •

edited

Loading

jeffkl left a comment

Restore: Prefer TargetFramework over TargetFrameworks when it is specified as a global property. #4587

Restore: Prefer TargetFramework over TargetFrameworks when it is specified as a global property. #4587

Conversation

nkolev92 commented Apr 21, 2022 • edited Loading

Bug

Description

PR Checklist

nkolev92 commented Apr 21, 2022

rolfbjarne commented Apr 21, 2022

nkolev92 commented Apr 21, 2022

dsplaisted commented Apr 26, 2022

rainersigwald commented Apr 26, 2022

nkolev92 commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 commented May 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 commented May 20, 2022

zivkan left a comment

Choose a reason for hiding this comment

nkolev92 commented May 27, 2022 • edited Loading

jeffkl left a comment

Choose a reason for hiding this comment

Restore: Prefer `TargetFramework` over `TargetFrameworks` when it is specified as a global property. #4587

Restore: Prefer `TargetFramework` over `TargetFrameworks` when it is specified as a global property. #4587

nkolev92 commented Apr 21, 2022 •

edited

Loading

nkolev92 commented May 27, 2022 •

edited

Loading