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

[Feature] Make Amber work in Git Bash on Windows #490

Closed
hdwalters opened this issue Sep 22, 2024 · 14 comments · Fixed by #499 or #501
Closed

[Feature] Make Amber work in Git Bash on Windows #490

hdwalters opened this issue Sep 22, 2024 · 14 comments · Fixed by #499 or #501
Labels
enhancement New feature or request

Comments

@hdwalters
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Amber is officially supported on Linux and Mac systems only. I use Windows in my day job; the Amber website advises me to install WSL, but I use Git Bash as my primary shell.

Describe the solution you'd like
I would like to be able to use Amber in Git Bash on Windows.

Describe alternatives you've considered
The alternative is to install WSL, but that is not an option for me.

Additional context
The Amber source code has only one Windows conditional compilation directive. Nothing I've seen looks like it would break on Windows; and I already fixed the place I found where a file system path was being constructed by hand, rather than using PathBuf::join(). I do not see any major reasons why it should not work on Windows.

@hdwalters hdwalters added the enhancement New feature or request label Sep 22, 2024
@b1ek
Copy link
Member

b1ek commented Sep 23, 2024

what if you compile it on your windows machine, and then run an amber program on that machine with git bash? if its good, then its just a matter of building the releases for windows

@hdwalters
Copy link
Contributor Author

Yep, I intend to try that when I get a chance to set up a GitHub SSH key on that PC. This issue is a placeholder for discussion of any further work that may be necessary.

@Mte90
Copy link
Member

Mte90 commented Sep 23, 2024

Maybe also setup something on the CI it will be useful

@Ph0enixKM
Copy link
Member

Yeah... I just wanted to avoid all the "the amber compiler compiles to .bat files that don't work" kind of drama. The compiler should work perfectly fine on Windows. We can add windows in CI pretty easily

@hdwalters
Copy link
Contributor Author

hdwalters commented Sep 23, 2024

Compile to ".bat" files? Shudder. Neither do we want PowerShell.

@hdwalters
Copy link
Contributor Author

hdwalters commented Sep 25, 2024

I tried compiling and running Amber on Git Bash:

$ cat hello.ab
#!/usr/bin/env amber
echo "Hello world"
$ amber.exe hello.ab -
#!/usr/bin/env bash
# Written in [Amber](https://amber-lang.com/)
# version: 0.3.5-alpha
# date: 2024-09-25 18:13:00
# #!/usr/bin/env amber
echo "Hello world"
$ ./hello.ab
Error: Os { code: 3, kind: NotFound, message: "The system cannot find the path specified." }

So it looks like the compiler works fine (as expected) but it's unable to find some resource in the file system when it tries to execute the transpiled code. I'll have a go at fixing this; shouldn't be too hard.

@hdwalters
Copy link
Contributor Author

hdwalters commented Sep 27, 2024

That NotFound error was because the Amber compiler tries to run /usr/bin/env bash -c "[code]", and that doesn't work in Git Bash. I wrote some alternate code for Windows builds, which searches the directories in the PATH enviroment variable for bash.exe, and calls that. Typically it will find C:\Program Files\Git\usr\bin\bash.exe.

$ cat hello.ab
#!/usr/bin/env amber
echo "Hello world"
$ amber.exe hello.ab -
#!/usr/bin/env bash
# Written in [Amber](https://amber-lang.com/)
# version: 0.3.5-alpha
# date: 2024-09-26 22:38:51
# #!/usr/bin/env amber
echo "Hello world"
$ ./hello.ab
Hello world

PR forthcoming, once I've done a final test on my Windows machine.

@hdwalters
Copy link
Contributor Author

hdwalters commented Sep 27, 2024

However, note that the unit tests do not run, at least in part because bc is not installed by default on Git Bash. There are instructions on how to install it, and I can try to work through them... but honestly, how bothered do we need to be about this?

@Mte90
Copy link
Member

Mte90 commented Sep 30, 2024

#366

The plan is to remove bc and sed requirements, I have a PR pending for Sed and should remove the bc requirement.
The PR is blocked by an escaping issue in the code generated #381

@hdwalters
Copy link
Contributor Author

Ok. I can either hold off for now, or submit a PR on the understanding that unit tests will not work without further work. Preferences anyone?

@hdwalters
Copy link
Contributor Author

I'm inclined to submit the PR anyway. It's not like it will hurt anything.

@Mte90
Copy link
Member

Mte90 commented Sep 30, 2024

Yes proceed as anyway we are working to remove it.

@b1ek
Copy link
Member

b1ek commented Sep 30, 2024

"the amber compiler compiles to .bat files that don't work"

we could display a warning message for windows users when they install it

@hdwalters
Copy link
Contributor Author

"the amber compiler compiles to .bat files that don't work"

we could display a warning message for windows users when they install it

I'm not even sure if that's necessary. Amber creates Bash output, and advertises itself as a system which does that; if anyone is daft enough to confuse Bash with Batch, that's on them.

Though when we write the documentation (after removing bc and sed dependencies) we can emphasise that it runs on Git Bash, not that it runs on WIndows.

@Ph0enixKM Ph0enixKM linked a pull request Oct 1, 2024 that will close this issue
@Ph0enixKM Ph0enixKM linked a pull request Oct 3, 2024 that will close this issue
@Ph0enixKM Ph0enixKM reopened this Oct 3, 2024
@Mte90 Mte90 closed this as completed in #501 Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants