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

Ammonite not working under fish shell #813

Closed
mdedetrich opened this issue May 18, 2018 · 28 comments · Fixed by com-lihaoyi/mill#338
Closed

Ammonite not working under fish shell #813

mdedetrich opened this issue May 18, 2018 · 28 comments · Fixed by com-lihaoyi/mill#338

Comments

@mdedetrich
Copy link

mdedetrich commented May 18, 2018

When running Ammonite under fish shell, you get the following

Failed to execute process '/usr/local/bin/amm'. Reason:
exec: Exec format error
The file '/usr/local/bin/amm' is marked as an executable but could not be run by the operating system.

The issue is that you don't specify under what shell to run Ammonite under, i.e. if I edit the shell script to start ammonite, I see

@ 2>/dev/null # 2>nul & echo off & goto BOF^M
:
exec java -Xmx500m -XX:+UseG1GC $JAVA_OPTS -cp "$0" ammonite.Main "$@"
....

It instead should have the shebang, i.e. something like

#!/bin/bash
@ 2>/dev/null # 2>nul & echo off & goto BOF^M
:
exec java -Xmx500m -XX:+UseG1GC $JAVA_OPTS -cp "$0" ammonite.Main "$@"
....
@mdedetrich
Copy link
Author

mdedetrich commented May 18, 2018

Found the issue here, https://github.com/lihaoyi/mill/blob/master/main/src/mill/modules/Jvm.scala Going to submit a PR on mill.

Once the PR is merged and mill is released I can submit a fix here on ammonite

@lihaoyi
Copy link
Member

lihaoyi commented May 18, 2018

@mdedetrich did you install Ammonite using the command below? It's meant to prepend the shebang for you:

sudo sh -c '(echo "#!/usr/bin/env sh" && curl -L https://github.com/lihaoyi/Ammonite/releases/download/1.1.2/2.12-1.1.2) > /usr/local/bin/amm && chmod +x /usr/local/bin/amm' && amm

Note that the command for the stable-version installation was incorrect until recently (yesterday?) and I just corrected it

@mdedetrich
Copy link
Author

mdedetrich commented May 19, 2018

@lihaoyi I installed ammonite via homebrew, here is the formula

https://github.com/Homebrew/homebrew-core/blob/master/Formula/ammonite-repl.rb

The formula literally just downloads the zip and extracts it, is it possible to update it?

@lihaoyi
Copy link
Member

lihaoyi commented May 19, 2018

@mdedetrich sure send a PR to homebrew

@ilovezfs
Copy link

@lihaoyi Can you please provide a release download that includes the shebang line? That's not the kind of logic that belongs in the Homebrew formula. Thanks!

@mdedetrich
Copy link
Author

mdedetrich commented May 20, 2018

@lihaoyi I agree with @ilovezfs , can you re-open the ticket and let me know when mill is released with the fix that was merged yesterday (i.e. com-lihaoyi/mill#338)? I can then send a PR that properly adds the shebang as well as updating the documentation.

@lihaoyi lihaoyi reopened this May 20, 2018
@lihaoyi
Copy link
Member

lihaoyi commented May 20, 2018

@mdedetrich done; you'll probably need to make it fall back to uploading separate windows/bash artifacts in that case, since the goal of removing the shebang line was to try and unify the two scripts (#789) (CC @robby-phd since IIRC you wrote the batch file prefix)

@lihaoyi
Copy link
Member

lihaoyi commented May 20, 2018

FWIW whatever solution we come up for this will also probably need to be ported to the Mill launcher script, which shares the same problem

@lhns
Copy link

lhns commented May 23, 2018

What if we always appended the shebang line? This would cause an error message on windows.
I experimented a bit and came up with this to remove the error message after it is printed:
https://gist.github.com/LolHens/00ccf1d14363411e0917bc1a8fad67e6
This has to be configurable of course.
Windows will still print 'C:\Users\xxxxxxx\Documents>#!/bin/sh"' but at least there would be no error message that would not confuse people and it will make it much easier to install for linux people.
I admit this is a somewhat hacky solution but it should work.

@mdedetrich
Copy link
Author

The thing is, ideally you should be making different targets for different platforms, this is what sbt-native-packager does (it creates a launcher for windows and one for *nix)

@lihaoyi What I have in mind is to create launchers in a seperate folders. So there will be one folder which will be called nix (for MacOSX/linux/unix/solaris etc etc) and windows for Windows

What do you think?

@robby-phd
Copy link
Collaborator

While I think it's ideal to have separate packages for different platforms, in this case, the only difference is a single shebang line for specific shells such as fish, ksh, etc.; so it is a matter of uploading tens of MBs for every commit of amm (and mill) with just a single line that differs. Granted, GitHub does not put a limit on release sizes now, but it seems like a waste to me.

I don't use homebrew so I'm not familiar with it, but perhaps a compromise would be to change the installation setup for homebrew+fish/ksh/etc. users to something like the following (someone can figure the a single line command for this, I'm sure):

  1. install amm/mill from homebrew as usual
  2. prepend shebang to amm/mill installed by homebrew

For example, the setup instruction for Cygwin provides a sed command to modify the path to use cygdrive, and it's used for testing mill under Cygwin in AppVeyor.

(This compromise works if homebrew does not detect integrity of installed files via hashing when running certain tasks and decides it should reinstall stuff.)

@mdedetrich
Copy link
Author

@robby-phd In general, the homebrew guys aren't really happy with modifying files directly unless strictly necessary, their argument is that this should be fixed upstream (see Homebrew/homebrew-core#28053 (comment) for more info). I would also comment in that thread if you want to convince the homebrew guys that you really don't want to make a separate distribution just for nix.

From what I know, if you wan't to be really strict about not making separate distributions, technically I would have to use homebrews patch command so it actually verifies the checksum of the resulting modified file

@ilovezfs
Copy link

My suggestion is to have an installation script in the zip file that takes a prefix parameter.

./install.sh --prefix=/somewhere

or

./install.sh /somewhere

and then it can do whatever magic is necessary, and we'll happily run

def install
  system "./install.sh", prefix
end

in the formula.

@ilovezfs
Copy link

@robby-phd
Copy link
Collaborator

@mdedetrich I'm well aware of the discussion in Homebrew/homebrew-core#28053 (comment) before responding, hence my suggestion to take the unsanctioned logic out of homebrew hoping that it might work.

It's not that I want to be strict about having only one distribution, IMO, I just don't think a single line diff as a strong justification to complicate things for everyone, especially if there is a simple workaround on the specifically affected user side.

Anyway, if either the patch or the install script works, then that sounds like a better option.

@lihaoyi
Copy link
Member

lihaoyi commented May 23, 2018

Using patch or having an install script both sound good to me

@AesaKamar
Copy link

AesaKamar commented Aug 14, 2018

Also encountered this when I installed ammonite via nix-env

Using this configuration

@what-the-functor
Copy link

Why not have just two separate launchers? One for unix/linux, written in sh or bash, with the appropriate shebang line included. One for windows, written in ?

@sake92
Copy link
Collaborator

sake92 commented Oct 20, 2018

@tonylotts I agree with you. That's what all other tools/apps like maven, gradle, tomcat do.
Another issue with current solution is that shebang has to be the first line. So there's no way to do a check for OS or a jump..
Also, feels so unnatural to change the whole JAR file just to tweak a bit of launcher script..
What about using ammonite in different environments? I have to rename it always, once it's amm, on win it's amm.bat etc.

@PatrickF1
Copy link

Sorry I don't follow the thread--should people who use fish just reinstall ammonite without brew?

@mdedetrich
Copy link
Author

@patrick-premont If you do this then you would have to manually edit the launcher in ammonite to add the #!/bin/bash

Personally I just run sh before running ammonite, then you can install it as you wish

@ghost
Copy link

ghost commented Jan 16, 2019

I use a custom amm function in my Fish configuration to work around this issue:

function amm --description 'Scala REPL'
    sh -c 'amm "$@"' amm $argv
end

@PatrickF1
Copy link

Thanks @swiesner-dlr . Works for me!

@PatrickF1
Copy link

Looks like this issue is fixed now, as your PR has been merged and released, right @mdedetrich ?

@jsatk
Copy link

jsatk commented Jun 16, 2021

I use a custom amm function in my Fish configuration to work around this issue:

function amm --description 'Scala REPL'
    sh -c 'amm "$@"' amm $argv
end

Is this still needed? Does ammonite now work w/o this script in fish shell?

@laughedelic
Copy link

laughedelic commented Jun 16, 2021

It seems to work for me well without any workarounds on Fish 3.2.2 and Ammonite 2.4.0.

(I did experience this issue before, but at some point it disappeared)

@jsatk
Copy link

jsatk commented Jun 16, 2021

Excellent. Will delete my wrapper script. Thanks @laughedelic.

@jsatk
Copy link

jsatk commented Jun 16, 2021

@lihaoyi it appears this can be closed.

$ echo $SHELL
/usr/local/bin/fish
$ amm
Loading...
Welcome to the Ammonite Repl 2.3.8 (Scala 2.13.3 Java 1.8.0_275)
@ 1 + 2
res0: Int = 3

@

@lihaoyi lihaoyi closed this as completed Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet