Do not import project extensions during restore

[jeffkl]

Fixes #9512

Context

During restore, MSBuild needs to evaluate the entry project and during this evaluation if any of the project extension files that NuGet generates (project.nuget.g.props and project.nuget.g.targets) they are used during the next restore. Another side effect is the files that exist on disk at the beginning of restore are embedded in the binary log and not the ones that are generated by NuGet.

Changes Made

Introduced a new property MSBuildIsRestoring which is set during an implicit restore (/restore) or an explicit restore (/Target:restore) so that we don't need to take a dependency on MSBuildRestoreSessionId. This is now a property users can key off of to detect if a command-line based restore is running.

Default ImportProjectExtensionProps and ImportProjectExtensionTargets to false if MSBuildIsRestoring is true unless a users has explicitly set ImportProjectExtensionProps or ImportProjectExtensionTargets to true. This allows users to bring back the old behavior if desired.

I also added MSBuildRestoreSessionId and MSBuildIsRestoring to MSBuildConstants so their names can be defined in a single place.

Testing

Added to existing unit tests to verify

1. ImportProjectExtensionProps and ImportProjectExtensionTargets default to false if MSBuildIsRestoring is true
2. When ImportProjectExtensionProps and ImportProjectExtensionTargets are set to true, their values are not set even if MSBuildIsRestoring is true

Notes

[jeffkl]

FYI @KirillOsenkov

[ladipro]

Looks great. I guess adding more special-case handling of the restore target should boost the priority of #9553.

[JanKrivanek]

Looks great. I guess adding more special-case handling of the restore target should boost the priority of #9553.

100% agree on we should add mechanism flagging that issue.
Though it seems it won't be able to benefit from this change - as the Restore in this case is 'marked' only if it's the only executed target of the build and explicitly specified. If multiple targets are explicitly given (e.g. /t:'Build;Execute'), or if the Restore is added in the script (e.g. InitialTragets="Restore" or <BuildDependsOn>$(BuildDependsOn);Restore</BuildDependsOn>) then it's not marked.
So we'll probably need to infer the situation from the build execution (e.g. TargetStartedEvent args with the Restore or Pack target and as well as with the Build target are encountered with the same ProjectContextId)

[JanKrivanek]

Thank you!

[ladipro]

I wonder if in this case we could get away with adding a high-importance message right to XMake.cs (as opposed to building an analyzer).

[JanKrivanek]

I wonder if in this case we could get away with adding a high-importance message right to XMake.cs (as opposed to building an analyzer).

I support that!

It wouldn't catch the case of restore run requested via dependencies definition - but I'd hope it's rather esoteric. And even if it wouldn't be esoteric - check in XMake is cheap and we can have it quickly... - do you want to shoot a PR? :-)

[rainersigwald]

Wait, I don't think I understand what the message is for? Is it "you're running Restore and something else in the same invocation"?

[ladipro]

Yes, it would basically warn against /t:Restore;SomethingElse. Because it looks like it's broken enough to maybe warrant this.

[rainersigwald]

I fear that's the same kind of "breaking change" we've had to back out before. I think the analyzer-related "have a way to opt into new warnings" would have to be in place first.

[jeffkl]

Question for everyone:

Should this pull request also set EmbedProjectAssetsFile to false or should I update the .NET SDK to default EmbedProjectAssetsFile to false when MSBuildIsRestoring is true?

https://github.com/dotnet/sdk/blob/7b872bf735f7d86b6c20281ab9f7a55d9c712a91/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets#L69

[rainersigwald]

I'd leave that definition in the SDK I think.

[YuliiaKovalova]

Hi @jeffkl,

These changes backfired in an unusual way for the tests in sdk (e.g. dotnet/sdk#39093)
You can see the attached binlogs here
sdk_test_binlogs.zip

In the test, publish and restore are run on the same project separately, and on restore execution PublishTrimmed switch is passed in a special way
https://github.com/dotnet/sdk/blob/0a2a432e980a143c6f7a9827d3ce015b39564427/test/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs#L2252 (hopefully @MiYanni has some story behind this, please let us know!)
In fact it writes a new property to *.g.targets and expects from msbuild to react to it!

Since restore happens after publish, project is considered as up to date and this stage is skipped:

As a possible strategies we can suggest:

1. Make changes on MSBuild side and check *.g.targets for up to date state;
2. Revert the changes if it can be considered as a real world scenario;
3. Probably something can be adapted on the Nuget's side (?).

Please share you opinion on that!
Thank you.

[KirillOsenkov]

also adding @Forgind for his insights

[KirillOsenkov]

If my understanding is correct, we should update the tests to account for the new behavior.

I've filed an issue here and added some details:
dotnet/sdk#39266

[Forgind]

jeffkl and I investigated this yesterday in the SDK insertion PR and found the same root cause as YuliiaKovalova. We also decided it's probably best to just update the tests. We're still talking a bit (with dsplaisted) about how big of a breaking change it likely is and what we should do about it. Thanks for the parallel investigation!

[YuliiaKovalova]

If my understanding is correct, we should update the tests to account for the new behavior.

I've filed an issue here and added some details: dotnet/sdk#39266

Even switching to inline argument will help!

But I though it was intentionally passed this way for checking some specific scenario which I am not aware of.

[MiYanni]

@YuliiaKovalova

I was trying to understand how I'm related to this since I don't even know what this is. Took me a second as I looked at the blame on the file, and I'm not in the blame. However, I'm in the commit history for the file because I moved every test in the repo in this PR. So, I'd just recommend looking at the blame history over the commit history since it usually gives you a more accurate gauge of the people involved.