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

fix #766 #792

Merged
merged 1 commit into from
May 15, 2016
Merged

fix #766 #792

merged 1 commit into from
May 15, 2016

Conversation

giabao
Copy link
Contributor

@giabao giabao commented May 14, 2016

I don't fully understand this: set -- $(loadConfigFile "$script_conf_file") "$opts"
But I tested in alpipe:3.3 docker image:

docker run --rm -ti alpine sh

inside alpipe:

cat <<EOF > application.ini
-Dfoo="-J-Xms512M"
-J-Xms512M
-Xmx512M
EOF

touch test && chmod +x test

cat <<'EOF' > test
#!/bin/sh
loadConfigFile() {
  cat "$1" | sed '/^\#/d'
}
set -- $(loadConfigFile "application.ini") "$opts"
echo "[$opts]"
EOF

# ./test 
[]

cat <<'EOF' > test
#!/bin/sh
loadConfigFile() {
  cat "$1" | sed '/^\#/d'
}
set -- $(loadConfigFile "application.ini") "$@"
echo "[$@]"
EOF

# ./test 
[-Dfoo="-J-Xms512M" -J-Xms512M -Xmx512M]

cat <<'EOF' > test
#!/bin/sh
loadConfigFile() {
  cat "$1" | sed '/^\#/d'
}
set -- $(loadConfigFile "application.ini")
echo "[$@]"
EOF

# ./test 
[-Dfoo="-J-Xms512M" -J-Xms512M -Xmx512M]

cat <<'EOF' > test
#!/bin/sh
loadConfigFile() {
  cat "$1" | sed '/^\#/d' | sed 's/^-J-X/-X/' | tr '\r\n' ' '
}
opts=$(loadConfigFile "application.ini")
echo "[$opts]"
EOF

# ./test 
[-Dfoo="-J-Xms512M" -Xms512M -Xmx512M ]

@muuki88 muuki88 added the docker label May 14, 2016
@muuki88
Copy link
Contributor

muuki88 commented May 14, 2016

Thanks for your pull request. What is the problem you'll trying to fix with this PR?

@giabao
Copy link
Contributor Author

giabao commented May 15, 2016

  1. The current code set -- $(loadConfigFile "$script_conf_file") "$opts" do NOT load configs into $opts
    (the 1st ./test output [])
  2. In the bash-template (not ash-template), the corresponding statement is set -- $(loadConfigFile "application.ini") "$@" - which correctly load configs into $@
    (the 2nd ./test output [-Dfoo="-J-Xms512M" -J-Xms512M -Xmx512M])
  3. When ommit $@ from the statement set -- $(loadConfigFile "application.ini") "$@" - as in the 3rd ./test then $@ is still correctly set. So I said "I don't fully understand.. "
  4. The last ./test is to confirm that the changed code of this PR is correctly.
    Note that in the bash-template, -J-Xms512M is passed to the bash script in which the -J is removed before passing the java options to java.
    So here, in ash-template we should also remove -J. Otherwise the following sbt settings will cause error in ash but not in bash:
    javaOptions in Universal += "-J-Xmx512M"

@muuki88
Copy link
Contributor

muuki88 commented May 15, 2016

Thanks a lot for the clarification.

Regarding point 4.. Would you like to provide another pull request fixing this?

@muuki88 muuki88 merged commit 6ef8956 into sbt:master May 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants