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 spectre.console lib to manage ANSI escape code #497

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

AnimalXing
Copy link
Contributor

Why

Avoid using hardcoded ANSI escape codes to make it more readable.

test

Ticket

VUL-424

@AnimalXing AnimalXing marked this pull request as ready for review December 2, 2024 18:22
@AnimalXing AnimalXing requested a review from a team as a code owner December 2, 2024 18:22
@boarnoah
Copy link
Member

boarnoah commented Dec 2, 2024

Looks nice, can you verify things still function with a native AOT build

build it similar to how we do it in CI (AFAIK PRs don't generate artifacts for u test with)

dotnet publish `
/p:Version="$versionWithoutv" `
/p:InformationalVersion="$version" `
/p:IncludeSourceRevisionInInformationalVersion=false `
-r $env:PLATFORM-x64 `
-o build/x64

something like

dotnet publish -r win-x64 -o build/

Verify that things still work with that build, not expecting trouble here I think, since there is no trimming warnings / huge increase in binary size.

@AnimalXing
Copy link
Contributor Author

@boarnoah

dotnet publish -r win-x64 -o build/

Ran this command and tested with its build. It looked fine to me. However, it had some trimming warnings just like from CI:

Warning: /home/runner/work/puppeteer-sharp/puppeteer-sharp/lib/PuppeteerSharp/BrowserFetcher.cs(177): warning IL3000: PuppeteerSharp.BrowserFetcher.GetBrowsersLocation(): 'System.Reflection.Assembly.Location.get' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'. [D:\a\bmx\bmx\src\D2L.Bmx\D2L.Bmx.csproj] etc.

Is this something to worry about?

@boarnoah
Copy link
Member

boarnoah commented Dec 2, 2024

That is a known warning with our choice of puppeter sharp (and present prior to this PR), doesn't look relevant to the new lib you are introducing.

What we do need to verify is that the coloured text still prints as expected when invoking the native AoT build of BMX which you just built.

===

It isn't likely to be an issue in this particular case, but in the past for example we've had issues where the application works as expected when doing dotnet run.

However later when we try the native build (which has had code trimming done as part of building for Native AoT) the application no longer functions etc..

Native AoT stuff has difficulty understanding code is actually used in some cases and isn't dead code that can be trimmed, so best to always check things are functional after introducing a new library.

@AnimalXing
Copy link
Contributor Author

It isn't likely to be an issue in this particular case, but in the past for example we've had issues where the application works as expected when doing dotnet run.

However later when we try the native build (which has had code trimming done as part of building for Native AoT) the application no longer functions etc..

Thank you for the info! Just verified that the coloured text worked as expected with the Native AoT build, both when the NO_COLOR flag was on and off.

@AnimalXing AnimalXing merged commit 2d7f2fb into main Dec 2, 2024
9 checks passed
@AnimalXing AnimalXing deleted the add-spectre.console branch December 2, 2024 21:50
cfbao added a commit that referenced this pull request Jan 7, 2025
2d7f2fb
/ #497 broke `bmx print` by
writing all these message to stdout.

Only the environment variable setting commands outputted by `bmx print`
can go to stdout. All other messages should to stderr.

https://desire2learn.atlassian.net/browse/VUL-628
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants