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

Open
mdedetrich opened this Issue May 18, 2018 · 23 comments

Comments

Projects
None yet
10 participants
@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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Owner

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Owner

lihaoyi commented May 19, 2018

@mdedetrich sure send a PR to homebrew

@ilovezfs

This comment has been minimized.

Copy link

ilovezfs commented May 20, 2018

@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

This comment has been minimized.

Copy link

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. 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

This comment has been minimized.

Copy link
Owner

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

This comment has been minimized.

Copy link
Owner

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

@LolHens

This comment has been minimized.

Copy link
Contributor

LolHens 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

This comment has been minimized.

Copy link

mdedetrich commented May 23, 2018

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

This comment has been minimized.

Copy link
Collaborator

robby-phd commented May 23, 2018

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

This comment has been minimized.

Copy link

mdedetrich commented May 23, 2018

@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

This comment has been minimized.

Copy link

ilovezfs commented May 23, 2018

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

This comment has been minimized.

@robby-phd

This comment has been minimized.

Copy link
Collaborator

robby-phd commented May 23, 2018

@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

This comment has been minimized.

Copy link
Owner

lihaoyi commented May 23, 2018

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

@AesaKamar

This comment has been minimized.

Copy link

AesaKamar commented Aug 14, 2018

Also encountered this when I installed ammonite via nix-env

Using this configuration

@tonylotts

This comment has been minimized.

Copy link

tonylotts commented Oct 20, 2018

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

This comment has been minimized.

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.

@patrickf3139

This comment has been minimized.

Copy link

patrickf3139 commented Nov 2, 2018

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

@mdedetrich

This comment has been minimized.

Copy link

mdedetrich commented Nov 5, 2018

@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

@swiesner-dlr

This comment has been minimized.

Copy link

swiesner-dlr 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
@patrickf3139

This comment has been minimized.

Copy link

patrickf3139 commented Jan 16, 2019

Thanks @swiesner-dlr . Works for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment