-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Building a subset of a VS solution at the command line causes MSB4057 #6465
Building a subset of a VS solution at the command line causes MSB4057 #6465
Conversation
@@ -782,7 +782,7 @@ private void EvaluateAndAddProjects(List<ProjectInSolution> projectsInOrder, Lis | |||
AddTraversalTargetForProject(traversalInstance, project, projectConfiguration, "Publish", null, canBuildDirectly); | |||
|
|||
// Add any other targets specified by the user that were not already added | |||
foreach (string targetName in _targetNames.Except(traversalInstance.Targets.Keys, StringComparer.OrdinalIgnoreCase)) | |||
foreach (string targetName in _targetNames.Where(i => !traversalInstance.Targets.ContainsKey(i))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we extract method that returns the enumerable? And use it in four places? And add a detailed comment explaining that it has to be lazy because evidently the traversalInstance.Targets contents changes during enumeration? It's a tricky spot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need to pay attention to how Keys behaves - is it returning the live collection or a snapshot of the keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract method might be dicey - maybe better keep as is for simplicity... just add comments? You decide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want this to be as small of a change and as safe of a change as possible, since this is going into 16.10 rather than 17.0. This is a straight revert of the relevant part of the commit that caused the issue, which is safest in my opinion. Adding a comment is a good plan, though—I'll just add it for the first case, since that's the only one that I know should be just in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on keeping this to a revert. Can extract as follow up in a future milestone if that's better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let’s not extract, it was just a knee jerk reaction of seeing the changes in four places. All good!
Exclude takes a snapshot of what we're excluding. We change the Enumerable mid-enumeration, which makes that invalid.
2491470
to
94ce625
Compare
e32bd4c
to
d6a27dd
Compare
This allows to build Node.js at the (temporary) cost of longer build times. Refs: nodejs#38872 Refs: https://github.com/dotnet/msbuild/releases/tag/v16.10.0 Refs: dotnet/msbuild#6465 Refs: dotnet/msbuild#6373
it seems that using the "rebuild" switch is working as a workaround msbuild solution.sln -t project:rebuild |
This allows to build Node.js at the (temporary) cost of longer build times. Refs: nodejs#38872 Refs: https://github.com/dotnet/msbuild/releases/tag/v16.10.0 Refs: dotnet/msbuild#6465 Refs: dotnet/msbuild#6373 PR-URL: nodejs#38873 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
This allows to build Node.js at the (temporary) cost of longer build times. Refs: #38872 Refs: https://github.com/dotnet/msbuild/releases/tag/v16.10.0 Refs: dotnet/msbuild#6465 Refs: dotnet/msbuild#6373 PR-URL: #38873 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Will this be released in 16.10.1? We have tests and things that are hitting this bug. |
@ben-may Yes, it's in the internal builds and will be in the public 16.10.1. |
Thanks! |
This allows to build Node.js at the (temporary) cost of longer build times. Refs: #38872 Refs: https://github.com/dotnet/msbuild/releases/tag/v16.10.0 Refs: dotnet/msbuild#6465 Refs: dotnet/msbuild#6373 PR-URL: #38873 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
This allows to build Node.js at the (temporary) cost of longer build times. Refs: #38872 Refs: https://github.com/dotnet/msbuild/releases/tag/v16.10.0 Refs: dotnet/msbuild#6465 Refs: dotnet/msbuild#6373 PR-URL: #38873 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
This allows to build Node.js at the (temporary) cost of longer build times. Refs: #38872 Refs: https://github.com/dotnet/msbuild/releases/tag/v16.10.0 Refs: dotnet/msbuild#6465 Refs: dotnet/msbuild#6373 PR-URL: #38873 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
We've hit this issue with Pipeline builds in Azure DevOps as you've updated your build agents. When will the fix be deployed to Azure? |
This allows to build Node.js at the (temporary) cost of longer build times. Refs: #38872 Refs: https://github.com/dotnet/msbuild/releases/tag/v16.10.0 Refs: dotnet/msbuild#6465 Refs: dotnet/msbuild#6373 PR-URL: #38873 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Fixes #6373
Summary
Solution metaprojects should have a target for each project they contain, so that you can build a single project or set of projects in the context of the solution. A regression in 16.10 means that instead, specifying a project name as a target when building a solution attempts to build a target with that name for every project in the solution.
Customer impact
Customers cannot build a subset of a solution by specifying a project between #6282 and this PR.
Regression?
Yes, from 16.9. This bug was introduced in #6282. The critical line is line 786 of SolutionProjectGenerator, though all similar instances were reverted in this case. Note that as #6282 is an agglomeration of functionally unrelated changes, no other files in #6282 relied on the changes in SolutionProjectGenerator.
Changes Made
Reverted the part of #6282 using Except incorrectly, i.e., the part in SolutionProjectGenerator, and added a test.
Testing
Created a unit test that fails before the other changes in this PR and succeeds afterwards. The unit test mimics but simplifies the customer's repro.
Risk
Low. Revert to prior implementation.