alda-lang / alda

A music programming language for musicians. :notes:
https://alda.io
Eclipse Public License 2.0
5.6k stars 288 forks source link

PS color output fix #501

Closed Vanello1908 closed 1 month ago

Vanello1908 commented 1 month ago

Fixed one of bugs mentioned in this issue

daveyarwood commented 1 month ago

Hi, thanks for the PR! I tried this branch locally and unfortunately, it doesn't compile on Linux. I get this error:

$ go build .
package alda.io/client
        imports alda.io/client/cmd
        imports alda.io/client/color
        imports golang.org/x/sys/windows/registry: build constraints exclude all Go files in /home/dave/.go/pkg/mod/golang.org/x/sys@v0.22.0/windows/registry

I did some googling and found this StackOverflow question which looks like the same issue.

If we wanted to get this approach to work, I think we would need to use build constraints to exclude the Windows registry code when the code is being compiled and run in a non-Windows environment.

I did a little test of this by copy-pasting the code you added into a new file called color_windows.go, and it did build successfully after that. However, it won't exactly work that way, because there's a potential race condition where we could end up running Aurora = auroraLib.NewAurora(EnableColor) in color.go before the code in color_windows.go sets EnableColor appropriately.

If you'd like to keep working on this approach and adjust it along those lines so that it will compile on Linux, you're welcome to do so. You can check the CircleCI logs to see if the build succeeded, i.e. click the :x: or :heavy_check_mark: next to the commit message:

2024-07-27-173632_1156x324_scrot

And then click the "Details" link to go to the build logs in CircleCI:

2024-07-27-173704_1454x852_scrot

daveyarwood commented 1 month ago

My other thought on this is that it would be better if we didn't have to include a bunch of ad hoc code in Alda to work around OS-specific issues. We should really let the color library handle this for us.

The version of the color library we're using (Aurora) is from 2020, which is several years old at this point. I think it's worth trying to update to the latest version and see if it works better out of the box in Powershell.

If that doesn't work, switching to another color library is also an option. @KenP97 took a stab at that in #420, using the Gookit library.

Vanello1908 commented 1 month ago

Ok, this is done with build constraint. Next step - black symbols, it is a problem with readline lib. There are 3 options:

daveyarwood commented 1 month ago

Looking good so far! If you don't mind implementing my suggestions above, I think this will be ready to merge.

I am curious if you've tried updating the Aurora dependency, to see if maybe the library has improved in the last 4 years to the point that none of this OS-specific code is necessary. But I'm also fine with the approach in this PR, at least as an interim improvement.

Vanello1908 commented 1 month ago

Aurora v2.0.3+incompatible also has problems with PowerShell, so we remain this code