Skip to content
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

use ANSI escape codes to control colours of parameters we print #466

Merged
merged 1 commit into from
Jul 24, 2024
Merged

Conversation

cfbao
Copy link
Member

@cfbao cfbao commented Jul 23, 2024

Why

.NET's Console.ForegroundColor only targets stdout.
If we rely on Console.ForegroundColor and stdout is redirected (e.g. bmx print | iex), we won't get any colour on stderr either.
Related .NET issue: dotnet/runtime#83146

Changing to use ANSI escape codes to control text colours to work around this issue.

Ticket

https://desire2learn.atlassian.net/browse/VUL-423


depends on #464

@github-actions github-actions bot added language/csharp size/M A medium-sized PR. labels Jul 23, 2024
@cfbao cfbao force-pushed the colour branch 2 times, most recently from 6d5b9a5 to 2f7848c Compare July 23, 2024 20:36
@cfbao cfbao marked this pull request as ready for review July 23, 2024 20:41
@cfbao cfbao requested a review from a team as a code owner July 23, 2024 20:41
@cfbao cfbao requested review from boarnoah, scowing, jharoutuniand2l, jkomonen and gord5500 and removed request for a team July 23, 2024 20:41
scowing
scowing previously approved these changes Jul 23, 2024
gord5500
gord5500 previously approved these changes Jul 24, 2024
Comment on lines +33 to +36
if( ( mode & ENABLE_VIRTUAL_TERMINAL_PROCESSING ) == ENABLE_VIRTUAL_TERMINAL_PROCESSING ) {
_enabled = true;
return true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be for "its already enabled"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Comment on lines +42 to +43
[LibraryImport( Kernel32 )]
private static partial IntPtr GetStdHandle( int nStdHandle );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You sure we don't need to do SetLastError = true here?

If we are calling into un-managed code & checking if that was a success (I might be mis-understanding the purpose of that flag).

On .NET, the error information is cleared (set to 0) before invoking the callee when this field is set to true
So maybe that only applies for repeated calls with subsequent successes?

Copy link
Member Author

@cfbao cfbao Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's only needed if you want detailed error info.
See https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.marshal.getlastpinvokeerror?view=net-8.0.

It doesn't affect a function's return value.
It's sorta like $LASTEXITCODE vs stdout in PowerShell, where even if it's not set, it doesn't affect stdout of a process.

boarnoah
boarnoah previously approved these changes Jul 24, 2024
Base automatically changed from always-print-params to main July 24, 2024 13:28
@cfbao cfbao dismissed stale reviews from boarnoah, gord5500, and scowing July 24, 2024 13:28

The base branch was changed.

@cfbao cfbao merged commit 2dd49e6 into main Jul 24, 2024
9 checks passed
@cfbao cfbao deleted the colour branch July 24, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language/csharp size/M A medium-sized PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants