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

Run amm as shell and cmd script #781

Merged
merged 2 commits into from
Mar 28, 2018
Merged

Run amm as shell and cmd script #781

merged 2 commits into from
Mar 28, 2018

Conversation

lhns
Copy link

@lhns lhns commented Mar 27, 2018

As of v1.1.0 ammonite can run on windows.
This pr adds the ability to run the ammonite shell directly as a cmd script while it remains being a proper shell script.
With the .cmd or .bat extension it can be run by double clicking making it very easy for newcomers to launch the ammonite shell.
Feedback is very welcome :)

@lihaoyi
Copy link
Member

lihaoyi commented Mar 27, 2018

@robby-phd as the author of com-lihaoyi/mill#243, could you help review this? =)

).flatten
}

def defaultUniversalScript(javaOpts: Seq[String] = Seq.empty, shebang: Boolean = true): Seq[String] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shebang defaults to true gives me:

'#!' is not recognized as an internal or external command,
operable program or batch file.

Defaulting to false works for both sh and batch modes (Windows 10).
Any reason why it defaults to true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is an unfortunate tradeoff. If the shebang line is disabled you cannot simply run "./amm" on linux.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, perhaps a different shell might not like it; I tried without it on macOS's terminal (GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)) and KDE Neon's Konsole (GNU bash, version 4.3.48(1)-release (x86_64-pc-linux-gnu)).
At any rate, I think it's a better trade-off to enhance the curl+chmod command in Ammonite installation guide to prepend the shebang line (rather than all batch users getting the above error message).

val javaOptsString = javaOpts.map(_ + " ").mkString
universalScript(
shellCommands = Seq(s"exec java -jar $javaOptsString" + """$JAVA_OPTS "$0" "$@""""),
cmdCommands = Seq(s"java -jar $javaOptsString" + """%JAVA_OPTS% "%~dpnx0" %*"""),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though it's not necessary at this point as the commands are single-line now, it would be nice to use multiline Strings here instead of Seq[String] and have shellCommands and cmdCommands split by universalScript just in case the scripts need to be adjusted in the future to include more lines.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure if I understand this correctly but multiple lines would be possible like this:
Seq("line1", "line2")
or like that:
Seq("line1\nline2")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but one would rather write:

"""line1
  |line2""".stripMargin

Though, I think using what you have and passing:

"""line1
  |line2""".stripMargin.split('\n')

would work too. So, nevermind.

@lihaoyi
Copy link
Member

lihaoyi commented Mar 27, 2018

@robby-phd I also gave you commit access to the Ammonite repo, feel free to merge when you're happy!

@robby-phd
Copy link
Collaborator

This is an interesting coincidence, I was looking at https://stackoverflow.com/questions/17510688/single-script-to-run-in-both-windows-batch-and-linux-bash to see if mill's sh and batch versions can be combined after I did com-lihaoyi/mill#243 (but I had to fix other things ;p).

@lihaoyi Thanks; that would make it easier to do round trip Ammonite<->mill (trivial) changes. :)

@LolHens Would you be willing to bring this feature to mill as well (adapting com-lihaoyi/mill#243)? =)

@lhns
Copy link
Author

lhns commented Mar 27, 2018

I'll have to look into it but it should be possible

@robby-phd
Copy link
Collaborator

That would be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants