sdk: We should generate an error or warning if Restore and Build targets are run in the same evaluation
See https://github.com/dotnet/cli/pull/9117 and https://github.com/dotnet/sdk/pull/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:
- Improve the error message to hint that the cause of the error may be that restore and build were attempted in the same evaluation.
- Improve the message and also downgrade it to a warning.
- 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
About this issue
- Original URL
- State: open
- Created 6 years ago
- Comments: 15 (12 by maintainers)
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”.
This should be a hard error in msbuild.
@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.