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

Partition system and app args to bootstrapped executable #852

Merged
merged 8 commits into from May 21, 2018

Conversation

Projects
None yet
4 participants
@2m
Contributor

2m commented May 3, 2018

Fixes #848

2m added some commits May 3, 2018

@2m

This comment has been minimized.

Contributor

2m commented May 6, 2018

I finally got all of the bashisms out which fixed all of the previous errors from the bash script when running on Travis. However now CI failed with mismatched output from the checks in travis.sh. It looks as if the cli is being used not from this PR. And I am not able to reproduce this on my machine:

➜  ./cs-echo-launcher-args -Dother=thing foo -Dfoo=baz
foo
@alexarchambault

This comment has been minimized.

Member

alexarchambault commented May 8, 2018

Thanks @2m! I'm trying that locally…

Maybe this could be put behind an option. And if it's enabled, bash can be used as a shell I guess… Reason I'd like to make it optional is sbt-pack recently started to do the same, and I've been bitten by it a few times already (it passed some -D options to java rather than my app…).

@alexarchambault

This comment has been minimized.

Member

alexarchambault commented May 8, 2018

It seemed to work locally for me too…

@2m

This comment has been minimized.

Contributor

2m commented May 8, 2018

I am not too happy with the solution in my PR as well. It is a bit too magical.

An alternativce approach would be a -J flag which, for example, sbt launcher uses:

-J-X               pass option -X directly to the java runtime

So then the bootstrap generated script could partition the arguments if they start with -J and pass in the ones starting with -J before -jar with -J removed.

@2m

This comment has been minimized.

Contributor

2m commented May 9, 2018

I gave this another try and this time implemented the -J support like sbt does.

@2m

This comment has been minimized.

Contributor

2m commented May 14, 2018

Thanks for the d4c57d5

It looks like the last implementation of -J works even for the older bash and sh that are in Travis CI environment. So this looks ready to be merged in.

@alexarchambault

This comment has been minimized.

Member

alexarchambault commented May 21, 2018

Appveyor job seemed quite unstable recently, but it's green now… Merging, thanks!

@alexarchambault alexarchambault merged commit e4f40b6 into coursier:master May 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@2m 2m deleted the 2m:wip-partition-bootstrapped-args-2m branch May 22, 2018

@2m

This comment has been minimized.

Contributor

2m commented May 22, 2018

Great. Thanks!

@olafurpg

This comment has been minimized.

Contributor

olafurpg commented Jun 19, 2018

This is awesome, thanks @2m !

@rfkm

This comment has been minimized.

Contributor

rfkm commented Jul 4, 2018

This breaks apps which accept quoted arguments:

Expected:

$ your-app-printing-args a b "c d"
# => List("a", "b", "c d")

Actual:

$ your-app-printing-args a b "c d"
# => List("a", "b", "c", "d")
@2m

This comment has been minimized.

Contributor

2m commented Jul 10, 2018

I am not sure how to solve this problem with sh compatible script.

In bash, this can be solved by storing arguments in arrays, like so: 2m@cc59354

@rfkm

This comment has been minimized.

Contributor

rfkm commented Jul 10, 2018

Yeah, it's not an easy task to process args without arrays.
Another solution with sh might be iterating arguments twice...?

#!/bin/sh

nargs=$#

i=1; while [ "$i" -le $nargs ]; do
         eval arg=\${$i}
         case $arg in
             -J-*) set -- "$@" "${arg#-J}" ;;
         esac
         i=$((i + 1))
     done

set -- "$@" -jar "$0"

i=1; while [ "$i" -le $nargs ]; do
         eval arg=\${$i}
         case $arg in
             -J-*) ;;
             *) set -- "$@" "$arg" ;;
         esac
         i=$((i + 1))
     done

shift "$nargs"

java "$@"
@2m

This comment has been minimized.

Contributor

2m commented Jul 11, 2018

Woa. That looks great! Did not know that you can change the arguments with set like that. :)

The only modification this will need is to incorporate the javaOpts that could be passed to the bootstrap command when generating the bootstrap launcher.

val javaCmd = Seq("java") ++
javaOpts
// escaping possibly a bit loose :-|
.map(s => "'" + s.replace("'", "\\'") + "'") ++

@rfkm

This comment has been minimized.

Contributor

rfkm commented Jul 11, 2018

Thank you for checking! I'll try creating PR later.

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