Fail Restore when an SDK is unresolved or entry target does not exist

[jeffkl]

Fixes #6281

Context

BuildRequestDataFlags and ProjectLoadSettings are set during /t:restore in a best effort to run the Restore target in hopes that it will correct the potentially bad state that a project is in. Visual Studio also sets ProjectLoadSettings.IgnoreMissingImports so that an unresolved MSBuild project SDK doesn't prevent loading of a bad project so it can give the user an error and let them edit the file.

However, this means that from the command-line an unresolved SDK doesn't fail /t:restore. This is because the missing "import" is ignored and non-existent targets are ignored so the build succeeds.

Changes Made

Introduced two new BuildRequestDataFlags:

• SkipNonexistentNonTopLevelTargets - A new flag to be used in this context to tell the build to ignore non-existent targets but not top level ones. In this case we're specifying /t:restore so if the Restore target doesn't exist, that should be an error. Only other targets that are trying to run are ignored (like InitialTargets, Before/AfterTargets, etc).
• FailOnUnresolvedSdk - We still need to ignore missing imports and I can't introduce a new flag to split the implementation now since Visual Studio sets ProjectLoadSettings.IgnoreMissingImports as a way to ignore unresolved SDKs. So this flag tells the evaluator to fail on an unresolved SDK but to continue ignoring other missing imports.

Testing

Added three new unit tests:

• Restore fails when an SDK can't be resolved
• Restore fails if a top-level target like Restore doesn't exist
• Restore succeeds if a non-top-level target doesn't exist

[Forgind]

Looks good! Seems like there was no easy way to pass the entrypoint target(s) through to here?

[jeffkl]

@Forgind

Seems like there was no easy way to pass the entrypoint target(s) through to here?

Sort of, in this context entry.Request.Targets represents the top level target(s) to be executed based on the command-line based build:

msbuild/src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs

Line 152 in dd7831c

|| entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets) && !entry.Request.Targets.Contains(targetName))

Which is why I'm checking if the target that doesn't exist is in that list, this tells us if a top level target isn't there which should fail the build in this context. Other targets like InitialTargets and Before/AfterTargets to be built are passed in as targetNames by the RequestBuilder which handles figuring that out.

So the original entry.Request.Targets represents the "entry" targets or "top level" targets.

[Forgind]

Oh, I think I misunderstood that part, then. So did you add the new BuildRequestDataFlag in case that part fails? Looking more closely at it, it seems to log a message if (a particular target isn't in the list and the SkipNonexistentTopLevelTargets flag is set) or (the target is missing, and another flag is set). Otherwise, it adds it as if it's present. How is the error fitting in? Also, is that right? I would imagine something more like:
!targetExists && ((SkipNonexistentTargets && !entry.Request.Targets.Contains(targetName)) || (SkipNonexistentTopLevelTargets && entry.Request.Targets.Contains(targetName)))
Otherwise, it's skipping when you have a nonentry target but set SkipNonexistentTopLevelTargets.

Also, as a nit, HasFlag is slow. Not that big a deal, but I thought I should mention it.

[jeffkl]

@Forgind I've updated the PR to address the issue you pointed out and added a unit test for it as well. I'm going to leave HasFlag in for now since its adding to an existing one, but I do think we should ban it in the repo.

[Forgind]

Looks good! I tried to think of cases in which this would fail but didn't come up with any, and your explanation helped me properly understand that if clause. Thanks!

[Forgind]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).