search

igvteam / igv

Integrative Genomics Viewer. Fast, efficient, scalable visualization tool for genomics data and annotations
https://igv.org
MIT License
646 stars 387 forks source link

WISH: Have 'igv.sh' report on Java version upfront + explicitly say Java 17 is required #1475

Closed HenrikBengtsson closed 9 months ago

HenrikBengtsson commented 9 months ago

Background

When launching IGV at the command line, it reports Using system JDK etc., when running the "generic" version that doesn't come with a built-in JDK, e.g.

$ /path/to/IGV-2.17.1/igv.sh --version
Using system JDK.
WARNING: Unknown module: jide.common specified to --add-exports
WARNING: Unknown module: jide.common specified to --add-exports
WARNING: Unknown module: jide.common specified to --add-exports
WARNING: Unknown module: jide.common specified to --add-exports
WARNING: Unknown module: jide.common specified to --add-exports
WARNING: package com.sun.java.swing.plaf.windows not in java.desktop
WARNING: package sun.awt.windows not in java.desktop
WARNING: Unknown module: jide.common specified to --add-exports
WARNING: Unknown module: jide.common specified to --add-exports
java version "17.0.9" 2023-10-17 LTS
Java(TM) SE Runtime Environment (build 17.0.9+11-LTS-201)
Java HotSpot(TM) 64-Bit Server VM (build 17.0.9+11-LTS-201, mixed mode, sharing)
2.17.1

Now, when using an insufficient version of Java, say Java 11, we only get:

$ /path/to/IGV-2.17.1/igv.sh --version 
Using system JDK.
Caused by: java.lang.module.InvalidModuleDescriptorException: Unsupported major.minor version 61.0

With Java 1.8, we get:

$ /path/to/IGV-2.17.1/igv.sh --version
Using system JDK.
Unrecognized option: --module-path=/home/henrik/shared/software/CBI/IGV-2.17.1/lib
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

Wish

It would be useful if igv.sh reported on the Java version up-front, e.g.

$ /path/to/IGV-2.17.1/igv.sh --version
Using system JDK.
openjdk version "1.8.0_392"
OpenJDK Runtime Environment (build 1.8.0_392-8u392-ga-1~22.04-b08)
OpenJDK 64-Bit Server VM (build 25.392-b08, mixed mode)
Unrecognized option: --module-path=/home/henrik/shared/software/CBI/IGV-2.17.1/lib
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

This information alone would help support teams when a user reports they get an error, and spare one or more back'and'forth email communications.

In addition to reporting the Java version up front, I think it would be beneficial to also update:

https://github.com/igvteam/igv/blob/0acb332139744b4c096d0f8ccfa1c5793251491a/scripts/igv.sh#L19

to say:

Using system JDK. IGV requires Java 17.

This way, a user who uses a non-supported Java version would see:

$ /path/to/IGV-2.17.1/igv.sh --version
Using system JDK. IGV requires Java 17.
openjdk version "1.8.0_392"
OpenJDK Runtime Environment (build 1.8.0_392-8u392-ga-1~22.04-b08)
OpenJDK 64-Bit Server VM (build 25.392-b08, mixed mode)
Unrecognized option: --module-path=/home/henrik/shared/software/CBI/IGV-2.17.1/lib
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

and hopefully be able to resolve the problem themselves, e.g. module load openjdk/17.

See also

This was triggered by Issue #1472, but also from helping users on our HPC environment to use IGV that run into obscure Java errors due to too old versions of Java.

jrobinso commented 9 months ago

It would be useful, but I don't see any straightforward way to do it. Do you have suggestions? IGV can't report it because IGV code never gets executed. It would have be some check in the shell script.

HenrikBengtsson commented 9 months ago

... It would have be some check in the shell script.

Yes, I'm talking about igv.sh (and eventually also igv.bat etc.) here.

It already reports Using system JDK. Replacing that with Using system JDK. IGV requires Java 17. would be straightforward.

For the part reporting on the Java version, one approach would be to replace:

https://github.com/igvteam/igv/blob/0acb332139744b4c096d0f8ccfa1c5793251491a/scripts/igv.sh#L22-L38

with

# Report on Java version
java -version

# Check if there is a user-specified Java arguments file
if [ -e "$HOME/.igv/java_arguments" ]; then
    java --module-path="${prefix}/lib" -Xmx8g \
        @"${prefix}/igv.args" \
        -Dapple.laf.useScreenMenuBar=true \
        -Djava.net.preferIPv4Stack=true \
        -Djava.net.useSystemProxies=true \
        @"$HOME/.igv/java_arguments" \
        --module=org.igv/org.broad.igv.ui.Main "$@"
else
    java --module-path="${prefix}/lib" -Xmx8g \
        @"${prefix}/igv.args" \
        -Dapple.laf.useScreenMenuBar=true \
        -Djava.net.preferIPv4Stack=true \
        -Djava.net.useSystemProxies=true \
        --module=org.igv/org.broad.igv.ui.Main "$@"
fi
jrobinso commented 9 months ago

IGV reports "Using system JDK", but IGV itself is never ran when using Java 11. The failure is early before any of our code runs. So if you get to the printout "Using System JDK" you are already running Java 17, or later.

I don't see anything in your proposed change that checks the java version and reports an error, just a line that will print the version. We could do that but I'm not sure what it accomplishes.

HenrikBengtsson commented 9 months ago

So if you get to the printout "Using System JDK" you are already running Java 17, or later.

No, that's not correct. Here are the first non-comment lines in igv.sh:

https://github.com/igvteam/igv/blob/288cf7f6772bddda091b3102e2fd10c2cf1074a0/scripts/igv.sh#L11-L20

So, if I call /path/to/igv.sh as-is, I'll get to echo "Using system JDK." without involving Java (if I didn't install IGV with the built-in JDK). The different output in my top comment shows this.

HenrikBengtsson commented 9 months ago

I don't see anything in your proposed change that checks the java version and reports an error, just a line that will print the version. We could do that but I'm not sure what it accomplishes.

I was punting on the version assertion, because that requires parsing the output of java -version, which requires much more work given there are different Java implementations out there. As I argue above, just being explicit about (i) the Java 17 requirement, and (ii) always showing the Java version to be used, will help a lot when it comes to troubleshooting and users asking for help.

PS. I'm reporting this based on user FAQs on a system with 1,800 users. I've already added wrappers on our end that implements this feature request, and I'm also asserting the Java version, so the user gets an informative error message. We can parse the Java version, because we are in control of the different SDKs that the user might be running. We don't have to worry about all possible Java SDKs, as you would here.

jrobinso commented 9 months ago

Yes, you're completely correct, my mistake. We did at one time print this information from IGV.

So your request is to simply add this to the beginning of the script? I can certainly do that.

# Report on Java version
java -version
HenrikBengtsson commented 9 months ago

Great. Yes, and drop the existing -showversion, because otherwise the Java version will be reported twice.

jrobinso commented 9 months ago

OK done, thanks for the suggestion.

HenrikBengtsson commented 9 months ago

Thanks. Would you mind also adding the required Java version, i.e.

else
    echo "Using system JDK. IGV requires Java 17."
fi