Pass SolutionFilterName as a property#6234
Conversation
jeffkl
left a comment
jeffkl
left a comment
There was a problem hiding this comment.
The change looks good. But as a best practice, any time there's a regression, we should add a unit test that definitively fails without the change. This just helps ensure it doesn't regress again. Obviously we have no unit test that covered this scenario, so I would add one now.
There was a problem hiding this comment.
super-nit: Can the test solution file be simplified by removing x64 and/or Debug configurations, for example?
Forgind
force-pushed
the
solutionfiltername-fix
branch
from
45b3d0c to
faf1b13
Compare
| EndProject | ||
| Project(`{"{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}"}`) = `B`, `{proj2.Path}`, `{"{881C1674-4ECA-451D-85B6-D7C59B7F16FA}"}` | ||
| ProjectSection(ProjectDependencies) = postProject | ||
| {"{4A727FF8-65F2-401E-95AD-7C8BBFBE3167}"} = {"{4A727FF8-65F2-401E-95AD-7C8BBFBE3167}"} |
There was a problem hiding this comment.
The email thread that started this had two project dependencies, does testing one project dependency cover the N project dependencies scenario?
There was a problem hiding this comment.
Yes. I ran this test before and after the change, and it failed before and passed after.
What's important is that it has to build a metaproj.
Forgind
added
the
merge-when-branch-open
5 participants
Fixes internally reported bug
Context
I had previously added SolutionFilterName as a property. I missed one place it needed to be added for it to be passed properly to BuildRequests. This manifested as build failures in unusual cases when the engine generated a metaproj, didn't save it to disk—only into a cache, then queried the cache to see whether a project had already been built with the same name/guid/global properties. Because the third changed, it failed to recognize it and failed the build.
Changes Made
Added the property in the one missing spot.
Testing
@jeffkl showed me it working on one repro on his computer. I intend to try it on another repro shortly.
Notes