-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix control sequence emission #7630
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
Fix control sequence emission #7630
Conversation
AR-May
left a comment
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.
LGTM
|
Does this work on Windows 7? |
I don't know nearly enough about the differences between windows 7 and windows 11 to have a clue. Is there some reason we should expect it to fail? I can spin up a VM to test if you'd like. |
simply that there was considerable investment since Windows 7 in better supporting standard terminal output. I have zero context on this particular API but it struck me that it could have been affected. |
rainersigwald
left a comment
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 think the win7 question is a good one that needs to be thought through. It may introduce a limitation on the MSBuild Server stuff (but I don't think it's worth a huge amount of effort: I wouldn't be too sad if the answer was "disable MSBuild server on Win7").
|
I spun up a windows 7 VM and overwrote the MSBuild assemblies in the 6.0.300 SDK with assemblies from this build. When I tried building in cmd.exe, output with MSBUILDUSESERVER=1 or undefined looked the same. However, it took several seconds to start up when MSBUILDUSESERVER was enabled—not entirely sure why. In any case, I think this works properly on windows 7, but it seems like maybe people using windows 7 will want to opt out anyway. |
Context
When building with the MSBuild server, you could sometimes see VT100 control character sequences rather than interpreting them as desired. This fixes that.
Changes Made
Tell the console to expect VT100.
Testing
Built before and after; it displayed properly with this change.