-
Notifications
You must be signed in to change notification settings - Fork 454
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
Update project outline on configure #3270
Conversation
@microsoft-github-policy-service agree |
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.
Overall it looks pretty good, I just have a couple of questions and changes to be made.
Thanks for review, I hope it will be useful. |
@vlavati Thanks for the PR! I plan to test on Monday to confirm no breakages and then approve. Thanks! |
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.
Tested changes and things seemed to work as expected.
@benmcmorran Do you think the Windows test failure is due to these changes? It seems to be failing reliably but the failure itself is a "Resource is busy" error, which doesn't immediately strike me as something wrong with the code. Thanks! |
@gcampbell-msft I just queued a test run for #3336. If that one passes then I suspect changes in this PR are causing the failure. |
@gcampbell-msft Saw the same failure in the other PR, so I don't think we should consider it blocking here (but we should independently investigate why it started failing). |
@vlavati Hi! I've tested and all of the changes look good! One final thing before merging, could you please add a Once that is done we can merge! |
@gcampbell-msft , @benmcmorran Regarding the failed tests on Windows, I suppose that it is because of the Terminal which blocks a current working directory. Some tests run "await cmakeProject.launchTarget();", which starts a terminal and a compiled target. But I did not find a way to check the exit status or wait until the Terminal is closed. |
Merging. I'm bypassing the failing Windows tests because we're seeing the same failure on other PRs and it's not related to this change. |
This change addresses item #2823
This changes visible behavior of project outline
The following changes are proposed:
Other Notes/Information
On the image below there is a project outline for the "multi-root-UI". It contains projects for project-folder-2/subroot1, project-folder-2/subroot2, and project-folder-2.
If commands (Build/Launch) are called from context menu for the targets in multiroot workspace, it is needed to provide source directory of the project to choose correct project to execute cmake commands. (By default there is used active project)