We should generate an error or warning if Restore and Build targets are run in the same evaluation

[nguerrera]

See dotnet/cli#9117 and #2010

It has always been incorrect in general to to msbuild /t:restore;build or <MSBuild Targets="Restore;Build" because evaluation is shared and so build doesn't see the targets pulled down from nuget. #2010 adds a hard dependency on the ProjectAssetsFile being set by the generated props file in restore, which means that the first run of these incorrect patterns will always fail now instead of sometimes getting lucky. Note that lucky could be silent bad behavior.

Options I see:

1. Improve the error message to hint that the cause of the error may be that restore and build were attempted in the same evaluation.
2. Improve the message and also downgrade it to a warning.
3. Revert the change.

I would like to do (1) because we've lost a lot of time debugging subtly broken builds with this incorrect pattern, but we should think carefully about it.

cc @livarcocc @dsplaisted @wli3 @rrelyea @rohit21agrawal @nkolev92

[rainersigwald]

I also like option 1.

[nguerrera]

Another thing worth noting here is that this will only fail on the first attempt. Rerunning the same incorrect pattern will succeed when the props file is already on disk. This means that the error message has to be crystal clear or users will just chalk it up to perceived flakiness.

[NickCraver]

For what it's worth, this would break many of our builds running fine today. It is incorrect to say it'll only fail on the first attempt. A good build on a build agent should be doing a clean build on every pass, and therefore will fail on every attempt.

I definitely disagree with breaking this behavior that works for many people today. They have no idea they shouldn't do it because it works. I get that it may break some situations, but I think you'll quickly find this is used more than is probably currently thought. I've seen it a lot of places not related to us while figuring out how to do various things with builds along the way with SDK projects.

If it helps, the general procedure that I see lead to this is "what does Visual Studio do?", "Oh, it just restores first...I'll do that, Restore;Build".

[nguerrera]

To be clear, I wasn't pointing to the fact that it works on retry as a reason to leave this. Quite the opposite, I'm saying it will be hard to understand when you can't repro it for more than one attempt locally.

[nguerrera]

I think it's worth being pretty clear about what scenarios are broken if you use the pattern in question.

If any package in your graph, adds targets you'll never use those targets (except on local incremental builds). This could be silent bad behavior: build succeeds but a vital step is skipped. And you might not notice it locally because the bits you're debugging are often incremental. I think that we should at least warn.

There's also the fact that keeping this default of ProjectAssetsFile in our code blocks nuget from changing the default location. And the current location is busted for projects in the same directory with default intermediate path.

[nguerrera]

I've personally lost a lot of time chasing down subtle behavior from this, which is why I hope we can at least warn. But even warning will break builds :(

[bording]

Reading about this, I now realize that I've definitely been hit by it, but was never able to figure what the cause was. I'd much rather see some notification that calling restore and build together is wrong than continue to let it silently be a problem.

I suppose it's a long shot, but would there be a way to just do to the right thing on behalf of the user when one of bad invocation patterns was detected?

[daveaglick]

I suppose it's a long shot, but would there be a way to just do to the right thing on behalf of the user when one of bad invocation patterns was detected?

Could something like what happens with multi-targeting work here? I.e., if this pattern is detected could an “outer build” call the Restore target and then let an “inner build” handle all the subsequent targets?

[rainersigwald]

@daveaglick Sadly no. Currently, there's no way to detect inside a build that you were called in such a way, so there's nothing to condition off of.

But more importantly, there is no way to get correct behavior inside of a single build. Running Restore with a global property set to force reevaluation works only if no preexisting files get modified during restore. That means it works for the restore-from-clean scenario, because the .nuget.g.props files didn't exist before and forcing reevaluation will bring them in. But it uses stale package build logic if you

1. Restore
2. Change the version of a PackageReference (say from 1.0.0 to 1.0.1)
3. Restore;Build

Confusingly, that produces different results than you'd see if you cleaned between steps 2 and 3. The correct result is to use the .props and .targets from Package.1.0.1, but the previously-exisiting .nuget.g.props would point to Package.1.0.0, and be used the second time.

This is a result of MSBuild's caching of project XML, which was intended as a performance and correctness feature--even if you do something evil like modify Microsoft.Common.targets mid-build, you're presented with a consistent view of it through the entire build invocation--but is now quite confusing in the face of the pointers-to-NuGet-package-MSBuild-files world.

[NickCraver]

@nguerrera I thought about this over the weekend and while breaking people sucks, and warnings break people, I still think number 2 is the best available option here.

People will get the debug information and they can even <NoWarn> it if they so choose, or some other property the warning tells them to set.

[dsplaisted]

We decided not to take the breaking change for 2.1.3xx (ie option 3), but intend to bring it back in a subsequent release (option 1 or 2), ensuring we have the right error / warning message / experience. I've filed #2173 for fixing the breaking change, and I've updated the title of this issue to track implementing option 1 or 2.