Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Apr 8, 2021

Conversation

@jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Mar 31, 2021

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
@jeffkl jeffkl force-pushed the jeffkl:fix-restore-success branch from ba5e258 to dd7831c Mar 31, 2021
@jeffkl jeffkl changed the title WIP Fail Restore when an SDK is unresolved or top level target does not exist Mar 31, 2021
@jeffkl jeffkl marked this pull request as ready for review Mar 31, 2021
Copy link
Member

@Forgind Forgind left a comment

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

@jeffkl
Copy link
Contributor Author

@jeffkl jeffkl commented Apr 1, 2021

@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:

|| 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
Copy link
Member

@Forgind Forgind commented Apr 1, 2021

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 jeffkl changed the title Fail Restore when an SDK is unresolved or top level target does not exist Fail Restore when an SDK is unresolved or entry target does not exist Apr 1, 2021
@jeffkl jeffkl force-pushed the jeffkl:fix-restore-success branch from 1936bff to 9a23e41 Apr 1, 2021
@jeffkl
Copy link
Contributor Author

@jeffkl jeffkl commented Apr 1, 2021

@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
Forgind approved these changes Apr 1, 2021
Copy link
Member

@Forgind Forgind left a comment

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
Copy link
Member

@Forgind Forgind commented Apr 5, 2021

/azp run

@azure-pipelines
Copy link

@azure-pipelines azure-pipelines bot commented Apr 5, 2021

Azure Pipelines successfully started running 1 pipeline(s).
@jeffkl jeffkl closed this Apr 6, 2021
@jeffkl jeffkl reopened this Apr 6, 2021
@Forgind Forgind merged commit 29dc5e1 into dotnet:main Apr 8, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210406.1 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
@jeffkl jeffkl deleted the jeffkl:fix-restore-success branch Apr 15, 2021
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Apr 26, 2021
Add the behavior improvements from dotnet#6312 to the 16.10 changewave since
they cause failures in cases that didn't fail before.
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Apr 26, 2021
Add the behavior improvements from dotnet#6312 to the 16.10 changewave since
they cause failures in cases that didn't fail before.
ladipro added a commit that referenced this pull request Apr 27, 2021
#6312 made (good, IMO) changes to the behavior of -restore and -t:restore invocations of MSBuild in malformed projects. Since these are new errors, though, the new behavior might fail passing builds. So put it behind a change wave Just In Case.
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Apr 27, 2021
Add the behavior improvements from dotnet#6312 to the 16.10 changewave since
they cause failures in cases that didn't fail before.
Forgind added a commit to Forgind/msbuild that referenced this pull request May 7, 2021
Reverts dotnet#6312 and dotnet#6372.
SHAs reverted: 29dc5e1 and da900e2 respectively.
Forgind added a commit to Forgind/msbuild that referenced this pull request May 7, 2021
Maximal subset of dotnet#6312 and dotnet#6372.

Also removes an unnecessary test per dotnet#6430 (comment)
Forgind added a commit to Forgind/msbuild that referenced this pull request May 7, 2021
Maximal subset of dotnet#6312 and dotnet#6372.

Also removes an unnecessary test per dotnet#6430 (comment)
Forgind added a commit to Forgind/msbuild that referenced this pull request May 10, 2021
Reverts dotnet#6312 and dotnet#6372.
SHAs reverted: 29dc5e1 and da900e2 respectively.
Forgind added a commit to Forgind/msbuild that referenced this pull request May 10, 2021
Maximal subset of dotnet#6312 and dotnet#6372.

Also removes an unnecessary test per dotnet#6430 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants