Allow projects with no restore target to build under dotnet build
#6430
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeffkl
approved these changes
May 7, 2021
src/MSBuild.UnitTests/XMake_Tests.cs
Outdated
| /// Verifies a non-existent target doesn't fail restore as long as its not considered an entry target, in this case Restore. | ||
| /// </summary> | ||
| [Fact] | ||
| public void RestoreSkipsNonExistentNonEntryTargets() |
There was a problem hiding this comment.
This unit test is no longer applicable
src/MSBuild.UnitTests/XMake_Tests.cs
Outdated
| /// Verifies restore will run InitialTargets. | ||
| /// </summary> | ||
| [Fact] | ||
| public void RestoreRunsInitialTargets() |
There was a problem hiding this comment.
Leave this one here just for good measure
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
force-pushed
the
fail-on-missing-sdk-only
branch
from
f2d3736
to
24ae032
Compare
src/Build/Evaluation/Evaluator.cs
Outdated
| @@ -1785,8 +1785,7 @@ private List<ProjectRootElement> ExpandAndLoadImports(string directoryOfImportin | |||
|
|
|||
| if (!sdkResult.Success) | |||
| { | |||
| // Ignore the missing import if IgnoreMissingImports is set unless FailOnUnresolvedSdk is also set | |||
| if (_loadSettings.HasFlag(ProjectLoadSettings.IgnoreMissingImports) && !_loadSettings.HasFlag(ProjectLoadSettings.FailOnUnresolvedSdk)) | |||
| if (_loadSettings.HasFlag(ProjectLoadSettings.IgnoreMissingImports) && (!ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_0) || !_loadSettings.HasFlag(ProjectLoadSettings.FailOnUnresolvedSdk))) | |||
There was a problem hiding this comment.
I don't think I understand this. Why is it not wave 16.10, the one for the release this is targeting? What is this expression representing? Why not do this the way it was done in the prior commit and condition adding the new flag on the changewave?
There was a problem hiding this comment.
Will change to 16.10. I can also retarget this to vs16.10, since it's currently at master.
I think making the change in either place should be equivalent. I thought this would be cleaner. If you don't like it, I can change to the other way, though.
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
force-pushed
the
fail-on-missing-sdk-only
branch
from
24ae032
to
835ad3a
Compare
Reverts dotnet#6312 and dotnet#6372. SHAs reverted: 29dc5e1 and da900e2 respectively.
Maximal subset of dotnet#6312 and dotnet#6372. Also removes an unnecessary test per dotnet#6430 (comment)
Forgind
force-pushed
the
fail-on-missing-sdk-only
branch
from
835ad3a
to
389daf5
Compare
rainersigwald
changed the title
dotnet build
rainersigwald
approved these changes
May 11, 2021
JaynieBai
pushed a commit
that referenced
this pull request
Feb 8, 2023
) Bumps Microsoft.CodeAnalysis.BannedApiAnalyzers from 3.3.3 to 3.3.4. Release notes Sourced from Microsoft.CodeAnalysis.BannedApiAnalyzers's releases. v3.3.4 Release build of Roslyn-analyzers based on Microsoft.CodeAnalysis 3.3.1 NuGet packages. Works with VS 2019 16.9 or later. Contains important functionality and performance bug fixes on top of v3.3.3 release Commits 22ea642 Merge pull request #6436 from ViktorHofer/patch-1 9018071 Fix buildtransitive vs buildTransitive difference afa5665 [main] Update dependencies from dotnet/arcade (#6423) afee469 Merge pull request #6427 from mavasani/CodeAnalysisTreatWarningsAsErrors_Glob... b858999 Merge pull request #6430 from dotnet/locfiles/be3abf9f-1f22-469b-b26d-7648587... 0cbc3c5 Localized file check-in by OneLocBuild Task: Build definition ID 830: Build I... 113dadc Merge pull request #6429 from dotnet/revert-6364-net7-tests 356147f Revert "Move tests to target .NET 7" 1d7244a Update documentation for CodeAnalysisTreatWarningsAsErrors implementation 6c028d3 Re-implement CodeAnalysisTreatWarningsAsErrors with globalconfig files Additional commits viewable in compare view
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Work item (Internal use): AB#1326205
Summary
We introduced a new error with #6312 when there is no restore target. Builds had previously failed mysteriously after Restore had failed (because it didn't exist) but not failed the whole build. We put it under a change wave in #6372, but we still decided to revisit it. Specifically, if customers don't intentionally request a restore, dotnet build will still implicitly add one. With that change, customers without a restore target will then fail. They may not have a valid restore target because they failed to resolve an SDK—a problem we should alert them to—but they also might not have needed it.
This change maintains the SDK resolution failure case but dispenses with the missing target failure case.
Customer Impact
Customers calling restore (including indirectly through
dotnet build) see failures if there is no restore target, including when no sdk with a restore target is in use.Regression?
Yes. Worked in 16.9, regressed in 16.10.0 because of #6312
Testing
Unit tests and tried a local repro that didn't work with the bootstrap version.
Risk
Low. Just removes a new error.