Build should block on restore completion

[drewnoakes]

From internal email:

We’re having a problem with MAUI projects when changing between debug targets that have different RuntimeIdentifiers. After updating the RID value the project is targeting, we need to run a NuGet Restore so the project.assets.json file is updated.

For context, when the debug target is changed, we update the RuntimeIdentifier value in the csproj.user file. This change starts a NuGet restore, but there’s a lapse of time in between we make the change and the NuGet Restore process starts and completes in which the project is in an invalid state (the RID in the project is different than in the project.assets.json file), and the user can try to run/build the project.

If the user builds the project in that state they’ll get the NETSDK1047 build error, stating the selected RID is not present in the project.assets.json file.

Analysis from @davkean:

We have the same problem if you update the TFM and immediately build.

I think something on the .NET Project side or CPS side should block build (via RegisterAsyncTask) on restore data being up-to-date with the latest version of the project file.

Analysis from @lifengl:

I guess net core project side can handle this, because it knows when NuGet restore ends for the project. To ensure there is no time gap, the critical task might need be registered during ProjectChangedSynchronous event.

And, we need ensure fast build up to date check won’t skip the build completely in this case too.

[tmeschter]

@drewnoakes @davkean @lifengl Doesn't NuGet restore happen as part of the build itself when run from within VS? Or have I misunderstood what this setting does:

[davkean]

This provides no guarantees that the restore is up-to-date with the current version of the project.

[lifengl]

@davkean is right. Blocking restoring to finish before building sounds a right thing to do, however it does not ensure the project system to pick up nuget changes. Solution build is done out of proc, so once the files are on the disk, it will pick up them. During the project deployment step, if it requires the project state after picking up nuget restored files on the disk, they will have to use WhenProcessCompletedAsync to wait on that. There is no reason to block on that for build.

[lifengl]

Now, for the work to block build for pending NuGet restore:

1, once the nuget restore starts, NET Core has already registered it in the critical task list, which will block build to start until it is done.

2, however, this would not happen immediately after the project is changed, but only happens after NuGet restore starts. Because it requires a design time build to gather the data, this time gap can be quite big, and build is not blocked during that time frame.

3, NET core has already implemented a logic to check whether the project is in that state, and a way to wait it to finish. It is done in PackageRestoreDataSource for NuGet originally. So during the time a project is changed to the time it starts NuGet restore, HasPendingNomination will return true, and calling WhenNominated will create a task, which will finish either the package restore is started, or it is aborted (for example, if the project is being closed.) This implementation is being used and tested for NuGet scenario, so I think we should not repeat another implementation.

The problem of using that, is this logic is passive. It will not do anything if a project is changed, but only to check it/create a task when it is necessary (which means a nuget restore is needed, or in this case, a build is requested.) It was designed in this way to reduce cost/overhead, because we have far more time to modify a project than doing those operations. However, this would mean it would not be able to start a logic to RegisterAsyncTask.

So, here we are, we need a point before building to check this NuGet restore state, and either RegisterAsyncTask, or just wait this to happen. This is a problem, because CPS StartBuild will immediately wait on critical tasks, instead of having an extension point for other code to do further injections. Also using RegisterAsyncTask to inject that WhenNominated task is very problematic, because we didn't do any JTF for WhenNorminated, because this function was purely designed to know when this condition is satisfied, instead of waiting on that. NuGet will not block on UI thread to wait on that. We know StartBuid won't, but this is not generally expectation of CPS critical tasks.

For that, i think we should discuss what should happen. I lean towards adding another build extension points to allow CPS to wait when build is valid to start. But I want to hear your feedback @davkean , @drewnoakes ?

[lifengl]

I think we want something similar to IConfiguredProjectReadyToBuild, although that contract was for design time build, but we want one for real configured project build. The contract is quite confusing to me because it didn't even mention DT build in the type. Maybe we can reuse the contract type, but just come with a different MEF export name?

[drewnoakes]

@lifengl thanks for the analysis here. Let me paraphrase to ensure my understanding.

For this issue, we have the following steps:

1. User changes debug target so that RID changes, updating RuntimeIdentifier in the .csproj.user file
2. This project change triggers evaluation/DTB
3. NuGet restore is nominated
4. Restore completes
5. If restore changed the project, evaluation/DTB
6. Builds can now start

Currently, we block build for steps 3 and 4 via IProjectAsynchronousTasksService.RegisterAsyncTask in PackageRestoreDataSource.RestoreCoreAsync.

Builds requested during step 5 wait for the latest fast up-to-date check project data (after the design-time build in step 5 completes).

The problem today is that a build requested between steps 1 and 3 is allowed to start. It sees a mismatch between the new project and the old project.assets.json file, leading to build errors.

To fix this, we will extend the period during which a build may not be scheduled to cover the time between steps 1 and 3.

Something I am not clear about is how we would know that a given project change will require builds to be blocked. Are we talking about blocking all builds during evaluation/design-time builds?

I suppose if we have two different techniques to block build (one for steps 1-3, another for steps 3-4), then we need to ensure they overlap sufficiently that builds don't happen during the handover. It might be simpler to have a new component that blocks from steps 1-4 entirely.

Regarding reuse of IConfiguredProjectReadyToBuild, do we want to gate this at the configuration level? The RegisterAsyncTask approach is unconfigured.

[lifengl]

oh, so we need block it before finishing up to date checks? hmm... that sounds right, and maybe a little bit more complex than my previous thought. (I thought up to date check would return false, if the project is changed, but i guess we also talked about ignoring changes in the .user file.

We don't know what change would impact NuGet restore, what would not. As you said, it means to block all builds until DT builds is done. It would impact build speed right after project changes. (I wonder up to date has already waited on that today? or it just waits the first DT builds?)

Note: we only block build to start today for steps 3-4, not up to date check, so if it is needed before up to date check, it would be somewhat a different design.

And for overlay, that has been covered by the earlier design of HasPendingNomination and WhenNominated. Those two will only flip when NuGet nomination is started (but not wait on that). They were carefully designed to prevent deadlocks for NuGet scenarios, so I think it is far easier to use that, instead of creating a new one. It is far more likely to get it wrong.

[lifengl]

For IConfiguredProjectReadyToBuild: because build always happens in the configured project level, so i think it makes sense to keep it that way. It doesn't mean an implementation must depend on configured project level state. If it just tracks the project level state, it is perfect as well. RegisterAsyncTask was provided on both configured and unconfigured project level, although most usages might happen in the unconfigured project level. Note, the reason why we add the new contract is that we should not register WhenNominated as a critical task. We don't want to block solution closing on that, for example. If you want to register it, it might need some new wires for JTF, and handle cancellations. I guess it is doable -- I will not suggest to change the current implementation to wire JTF, it would be a wrap, i guess...

[drewnoakes]

I thought up to date check would return false, if the project is changed, but i guess we also talked about ignoring changes in the .user file

The FUTDC does return false if a project file changes.

I wonder up to date has already waited on that today? or it just waits the first DT builds?

The FUTDC waits on builds in order to get the latest data. This only impacts incremental builds, not rebuilds or cleans.

Data within the FUTDC is derived from project state and exposed as a data source. The actual check uses GetLatestVersionAsync on that data source to get a snapshot to use for the tests. My understanding of GetLatestVersionAsync is that it only returns a published version when it matches the current project version at that point in time.

we only block build to start today for steps 3-4, not up to date check, so if it is needed before up to date check, it would be somewhat a different design.

I don't think we need a change here. Any time an incremental build occurs, the FUTDC will block that build by itself.

I'll aim to reuse the NuGet implementation and wrap it to provide a configured-scope version of IConfiguredProjectReadyToBuild with a different contract name. I assume we'll need a change on the CPS side to pick up this new export too.

[lifengl]

Thanks a lot, that is my understanding as well. I worried about that, because we happened to have a discussion that FUTDC to skip changes in .user file. It turns out that optimization is not valid for this scenario. If we haven't done any optimization for the .user file, we don't have a problem then.

To get this to work, CPS has to import the new component with a different MEF contract name, even if we reuse the same interface. Don't expose a normal IConfiguredProjectReadToBuild component to wait NuGet restore to start, it will block DT build to start, and effectively introduces a deadlock.

The drawback of this contract is that it doesn't provide any message why the build is being held. Certainly, we don't need a message for DT build, because it is really internal background work, but if the real build waits for a while, maybe we want to log a message string to the output window. If that is needed, maybe we should define a new contract to return an optional message string along with a task?

[drewnoakes]

we happened to have a discussion that FUTDC to skip changes in .user file

I should have mentioned above that I don't believe we can skip the .user file without making changes to MSBuild. This is because changes to the .user file currently appear to the FUTDC via the MSBuildAllProjects property, where MSBuild automatically sets the most recently modified file path at the start of that property's value. If we ignore a .user file, there may also have been other project files that were changed, which we then would not see. To achieve this properly, MSBuild would have to ignore the .user file when setting MSBuildAllProjects, which would probably be behind some kind of setting. All that to say, it'd have to be solving a common enough problem to justify those changes across layers.

Having a user-visible message seems helpful. We could subclass IConfiguredProjectReadToBuild or come up with a new type. Let's discuss today in our sync meeting.

[drewnoakes]

CPS PR that adds support to postpone solution builds: https://devdiv.visualstudio.com/DevDiv/_git/CPS/pullrequest/397664

[adamint]

@drewnoakes will you be tackling this or is it me? I see we're both assigned, I can unassign myself

[adamint]

@drewnoakes I'm unable to actually repro this. Can you provide guidance?

[drewnoakes]

An approach suggested by @davkean above is to change the TargetFramework then build.

1. Create a Console App
2. Build it
3. Open the .csproj file and change the TargetFramework, eg from net6.0 to net5.0 and DO NOT SAVE
4. Build the solution (which will save)

On my machine those steps produce:

1>FastUpToDate: Input 'C:\Users\drnoakes\source\repos\ConsoleApp436\ConsoleApp436\ConsoleApp436.csproj' is newer (2022-08-02 11:49:45.485) than earliest output 'C:\Users\drnoakes\source\repos\ConsoleApp436\ConsoleApp436\bin\Debug\net6.0\ConsoleApp436.pdb' (2022-08-02 11:49:31.534), not up-to-date. (ConsoleApp436)
1>------ Build started: Project: ConsoleApp436, Configuration: Debug Any CPU ------
1>C:\Program Files\dotnet\sdk\7.0.100-preview.4.22252.9\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(216,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
1>C:\Program Files\dotnet\sdk\7.0.100-preview.4.22252.9\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(267,5): error NETSDK1005: Assets file 'C:\Users\drnoakes\source\repos\ConsoleApp436\ConsoleApp436\obj\project.assets.json' doesn't have a target for 'net5.0'. Ensure that restore has run and that you have included 'net5.0' in the TargetFrameworks for your project.
1>Done building project "ConsoleApp436.csproj" -- FAILED.

The key bit there is:

Assets file project.assets.json doesn't have a target for 'net5.0'. Ensure that restore has run and that you have included 'net5.0' in the TargetFrameworks for your project.

In other words, build did not wait for the restore to complete. The expected behaviour is that the restore completes, which updates project.assets.json, then the build runs. We are running the build before the assets file has been updated, leading to this error.

[drewnoakes]

Another way to reproduce this (from AB#1889176):

1. Create a WPF .NET app, select .NET 8.0 preview
2. Edit CSPROJ file and add property (don't save!):

   <SelfContained>true</SelfContained>
   

3. Build

If this happens fast enough, you'll see something like:

NETSDK1047: Assets file 'C:\Path\obj\project.assets.json' doesn't have a target for 'net8.0-windows/win-x64'. Ensure that restore has run and that you have included 'net8.0-windows' in the TargetFrameworks for your project. You may also need to include 'win-x64' in your project's RuntimeIdentifiers.; in file "Microsoft.PackageDependencyResolution.targets"