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

Usage of system-java in linux startup script. #64

Merged
merged 6 commits into from
Mar 1, 2016

Conversation

SKoschnicke
Copy link
Contributor

If no TWS-Java is found, the linux startup script now also searches for
a Java version installed on the system and uses it, if it is from oracle.

As discussed in issue #62.

If no TWS-Java is found, the linux startup script now also searches for
a Java version installed on the system and uses it, if it is from oracle.
@rlktradewright
Copy link
Contributor

Thanks for this. Just a couple of points to be made.

In IBController.sh:

  • the new lines 173-186 are not exactly equivalent to the original code. You've made an assumption that if $tws_java/pref_jre.cfg exists, it will always contain a valid path to the embedded Java. While I suspect that assumption is correct, we don't actually know this.
  • it appears to me that if Oracle Java is installed, your code will use that in preference to the embedded Java.

In userguide.md:

  • grammar error in:

    because other version have shown to cause problems running TWS

  • this sentence is misleading:

    You can also completely override which Java should be used by supplying the javaPath parameter to the startup script

    I want to discourage the average end user from fiddling unnecessarily with the scripts, especially with the IBController.sh and IBController.bat scripts (which is why I've put them in a subdirectory). The startup script that they'll be dealing with doesn't have a javaPath parameter.

Actually I think I'd prefer you to leave the userguide.md alone for the moment, as I need to do a bit of work on it anyway, and I'll consider the best way to approach this issue then.

- both config files are now searched for a valid java path
- the system installed java is not used if a valid java path was found
  in configs
@SKoschnicke
Copy link
Contributor Author

Right. Fixed it.

@rlktradewright
Copy link
Contributor

Sorry for the delay with this: I wanted to properly test it before merging (I'm confident it's correct but better safe than sorry!), but unfortunately my Ubuntu VM has malfunctioned and I've run out of time (I'll be out for the rest of the day).

@SKoschnicke
Copy link
Contributor Author

Absolutely no problem! Have a nice weekend!

@rlktradewright
Copy link
Contributor

I pulled your changes into my clone and copied them across to my Ubuntu VM, and encountered the following problems:

  1. The IBController.sh script appears to have Windows rather than Linux line endings. It's possible I've got some Git setting that caused this when I pulled, but I couldn't see one so could you please make sure that they're right in the pull request.
  2. The script doesn't work properly when the embedded Java can't be found and the system Java is OpenJDK rather than Oracle. It just loads up TWS anyway, using the OpenJDK. The reason is that the output of $system_java -XshowSettings:properties -version contains many instances of "Oracle" even for the OpenJDK version. It would be better to test for "Java(TM) SE Runtime Environment" which only occurs in the Oracle output.

Having corrected these two problems, it all worked fine, but there are a couple of minor points that would improve it somewhat:

  • is it possible to swallow the output from echo "$java_path_from_config/bin" in the read_from_config() function so that it doesn't appear on the console?
  • please make sure all messages sent to the console start with an upper case character. At the moment it all looks a bit inconsistent.

Finally, please allow the --java-path parameter's setting to override all other possible Java installations. In this case, if the specified Java isn't found, we should exit rather than fall back to the other possibilities. This is because this parameter will only be used where the user wants to control precisely which Java version is used (eg the user needs OpenJDK to be the default Java but also needs to run IBController in a specific Oracle Java version other than the embedded one). I'll do the same for the Windows script, and update the higher level scripts as appropriate.

Thanks.

@SKoschnicke
Copy link
Contributor Author

Sorry for the delay. I will look into it in the next couple of days.

- when supplying a path via --java-path, search there for binary named
  "java", not java.exe
- better identifying system java as oracle java
- output starts with upper case character
@SKoschnicke
Copy link
Contributor Author

  1. I checked the line endings, but they are \n, no Windows line endings found. What is your core.autocrlf setting?
  2. I replaced the string to search in java properties to "Java(TM) SE Runtime Environment". The script now refuses to use OpenJRE.
  3. The output from the read_from_config function should be stored in JAVA_PATH and never be printed on the console. I can't reproduce this, how are you calling the script in what environment?
  4. Now all messages start with an upper case character.
  5. When supplying the --java-path option, JAVA_PATH is set at the beginning of the script (line 79) and never changed. In line 129 it is checked if the binary is present in this path and the script terminates otherwise. This is already the behavior you want, right? I had to fix the test by removing a ".exe" though.

@rlktradewright
Copy link
Contributor

Thanks for the update. I haven't had time to look at it yet (will do over the weekend), but I just wanted to respond to your points.

  1. I don't have a core.autocrlf setting. But I just discovered, by cloning the project onto a fresh Windows machine (no prior Git usage at all), that by default all files are converted to have native line endings - ie the LF line endings in the .sh files are converted to CRLF automatically. So I guess core.autocrlf true must be the default, so I'd best change it to core.autocrlf false.

  2. Good, thanks.

  3. See the terminal output below from my Ubuntu system (using the previous version of the .sh, not your latest version):

    Notice the extraneous lines /usr/bin/java and ~/Jts /opt/IBController. I can't make out where they're coming from.

  4. Ok thanks.

  5. Apologies, you're right. Brain fade...

richard@ubby:/opt/IBController$ bash IBControllerStart.sh
=================================
Generating the classpath
Classpath=/home/richard/Jts/952/jars/jts.jar:/home/richard/Jts/952/jars/total.jar:/opt/IBController/IBController.jar

Generating the JAVA VM options
Java VM Options= -Xmx768m

Determining the location of java executable
could not find /home/richard/.i4j_jres/1.8.0_60_64/bin/java
/usr/bin/java
found java executable in PATH
Location of java executable=/usr/bin

~/Jts /opt/IBController
Starting IBController with this command:
/usr/bin/java -cp /home/richard/Jts/952/jars/jts.jar:/home/richard/Jts/952/jars/total.jar:/opt/IBController/IBController.jar -Xmx768m ibcontroller.IBController /home/richard/IBController/IBController.ini

@SKoschnicke
Copy link
Contributor Author

Didn't know autocrlf defaults to true, but makes sense, I guess...

I found the source of the path outputs, it was not the function (see commit). They should be gone in the new version.

@rlktradewright
Copy link
Contributor

I was going to merge this, but then I noticed that it says there are conflicts that need to be resolved. I can't understand how there can be conflicts. The single file changed by this PR hasn't been modified in the main repository since this PR was raised. Is it something to do with PR #67, which would modify the same file if it were merged? If so it just seems odd, as both PRs seem to me to be entirely consistent with the current state of things, even though if either were merged it would render the other in conflict.

Can you throw any light on this, just so I'm sure I understand what I'm doing?

Conflicts:
	resources/Scripts/IBController.sh
@SKoschnicke
Copy link
Contributor Author

My fork was not a clean fork in the sense that our master branches are identical, because I added some things. Therefore I didn't merge the current master of your repository, but I did that now and resolved the conflicts. You can check them in the last commit.

rlktradewright added a commit that referenced this pull request Mar 1, 2016
Usage of system-java in linux startup script.
@rlktradewright rlktradewright merged commit 6071829 into ib-controller:master Mar 1, 2016
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.

2 participants