[Bug]: MSBuild should not embed project extensions during restore

[jeffkl]

Issue Description

During restore, any generated file in $(MSBuildProjectExtensionsPath) is imported here and here. Imports are automatically embedded in the binary log but in the case of these files, they are generated by the restore gesture.

When a user specifies /Target:Restore or /Restore, MSBuild launches a build invocation of the Restore target with a special global property MSBuildRestoreSessionId. It should also set ImportProjectExtensionProps and ImportProjectExtensionTargets to false to skip the import of any project extension during restore. This would only break users who are doing the following:

1. Invoke some custom build target that generates files in $(MSBuildProjectExtensionsPath)
2. Invoke restore and expect their generated props and targets to affect Restore

I am not aware of anyone doing this so I don't think it would be a breaking change. At this time, since the files are embedded during evaluation, the ones that get embedded are not guaranteed to be correct if restore writes new versions. The functionality that executes restore (like NuGet) should embed these files instead.

I'd be more than happy to send the pull request.

Steps to Reproduce

1. Restore a project with the binary logger (/t:restore /bl)
2. Modify a package reference
3. Restore again

Expected Behavior

The binary log should have a the updated .nuget.g.props

Actual Behavior

The embedded .nuget.g.props in the binary log is the one that existed on disk when the project was evaluated not the one that was generated during restore.

Analysis

No response

Versions & Configurations

No response

[JanKrivanek]

Team triage: versioning would be better solution - though more complicated. Restore filesmight possbily be of interest in some scenarios - so preventing them might be problematic as well.

[KirillOsenkov]

Note that the current behavior is plain wrong and incorrect. While versioning might be helpful in other scenarios, here it will only harm, by embedding irrelevant versions of these files, causing confusion, increasing binlog size and slowing the build down. What Jeff proposes is the best solution here.

[KirillOsenkov]

also by not importing these files during restore we make the build slightly faster and avoid non-determinism, where old content might feed into the evaluation in an unpredictable way.

[JanKrivanek]

@KirillOsenkov - Agreed. I as well missed the info about https://github.com/NuGet/NuGet.Client/pull/5494 that addresses concern of someone possibly wanting to see the files from restore time.

@jeffkl - you expressed the willingness to contribute a fixing PR - this would be very appreciated.

[jeffkl]

Yes absolutely, one last question:

The easiest implementation is to set some global properties here:

https://github.com/dotnet/msbuild/blob/45f3aed1944548f123ce928d1253e38b5008dccf/src/MSBuild/XMake.cs#L1775

However, it seems a little bad to have MSBuild setting global properties that are specific to any particular build. Instead, we could start setting a new property like IsMSBuildRestoring and then we could update the common props/targets to make decisions based on whether or not a restore is running. We'd just have to make sure we come up with a property name that no one is using. Alternatively, we could just update the common props/targets to read the MSBuildRestoreSessionId property to not import these generated files. So to summarize, the options are:

1. Have MSBuild set global properties ImportProjectExtensionProps and ImportProjectExtensionTargets to false during restore
2. Have MSBuild set a new global property like IsMSBuildRestoring and update the here to not import if that property is set
3. Update here to not import if MSBuildRestoreSessionId is set.

In my opinion, option 2 is the cleanest and I wish I had added such a property when I added MSBuildRestoreSessionId but any of the options make sense. Please weigh in.

/cc @rainersigwald

[rainersigwald]

Yeah, I also like 2; I would name it MSBuildIsRestoring. Though since we already have MSBuildRestoreSessionId, comparing it to nonempty seems pretty reasonable too.

How confident are we about other users of restoration not expecting the ProjectExtension stuff to be present at MSBuild-integrated restore time? I can imagine a system that would break under such a circumstance but I don't know if any exists. A changewave would probably be enough.

[jeffkl]

How confident are we about other users of restoration not expecting the ProjectExtension stuff to be present at MSBuild-integrated restore time? I can imagine a system that would break under such a circumstance but I don't know if any exists. A changewave would probably be enough.

I think its extremely unlikely that anyone is doing this. You would have to run some external process which generates the files and then run restore, and finally build, while expecting the restore to take into account what you generated up front. I highly doubt anyone has done this.