Fix problems with jdk detection

The java tool common routine finds a jdk by doing a filesystem glob.
This had a problem on windows in the case a specific version is requested,
because the format of name of the jdk directory has changed with JDK 9 -
there is a dash between jdk and the version string.  The glob which does
not attempt to match a version was general enough not to trip on this,
but with a version to match it would never match jdk-9 or higher.

The test harness then asks the found javac what version it is, and the
parsing of that did not work as expected once version numbers became
double-digit, as the regex was for a single digit followed by a dot.
The outcome is for 11.0.2 we get back '11' instead of '11.0'.  Change the
regex to match any number of digits followed by dot.

The Repository/RMIC.py change is to align with an earlier change to
Java/RMIC.py, but the may not be needed after the change to regex
just described.

Clean up some of the Java tool routines for consistency (there was
no functional change outside of JavaCommon.py)

Docstrings added or updated in several places.

Signed-off-by: Mats Wichmann <mats@linux.com>

8 files changed, 117 insertions, 51 deletions

diff --git a/src/CHANGES.txt b/src/CHANGES.txt
index 84a8056..bb08e4c 100755
--- a/src/CHANGES.txt
+++ b/src/CHANGES.txt
@@ -13,6 +13,9 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
     - Use importlib to dynamically load tool and platform modules instead of imp module
     - sconsign: default to .sconsign.dblite if no filename is specified.
       Be more informative in case of unsupported pickle protocol (py2 only).
+    - more fixes for newer Java versions (since 9): handle new jdk directory
+      naming (jdk-X.Y instead of jdkX.Y) on Windows; handle two-digit major
+      version. Docstrings improved.
 
   From John Doe:
 
diff --git a/src/engine/SCons/Tool/JavaCommon.py b/src/engine/SCons/Tool/JavaCommon.py
index 853f7f2..c77ace6 100644
--- a/src/engine/SCons/Tool/JavaCommon.py
+++ b/src/engine/SCons/Tool/JavaCommon.py
@@ -70,7 +70,7 @@ if java_parsing:
         def __init__(self, version=default_java_version):
 
             if not version in ('1.1', '1.2', '1.3', '1.4', '1.5', '1.6', '1.7',
-                               '1.8', '5', '6', '9.0', '10.0', '11.0'):
+                               '1.8', '5', '6', '9.0', '10.0', '11.0', '12.0'):
 
                 msg = "Java version %s not supported" % version
                 raise NotImplementedError(msg)
@@ -178,7 +178,7 @@ if java_parsing:
             if self.version in ('1.1', '1.2', '1.3', '1.4'):
                 clazz = self.listClasses[0]
                 self.listOutputs.append('%s$%d' % (clazz, self.nextAnon))
-            elif self.version in ('1.5', '1.6', '1.7', '1.8', '5', '6', '9.0', '10.0', '11.0'):
+            elif self.version in ('1.5', '1.6', '1.7', '1.8', '5', '6', '9.0', '10.0', '11.0', '12.0'):
                 self.stackAnonClassBrackets.append(self.brackets)
                 className = []
                 className.extend(self.listClasses)
@@ -395,30 +395,27 @@ else:
         return os.path.split(fn)
 
 
-
-java_win32_version_dir_glob = 'C:/Program Files*/Java/jdk%s*/bin'
+# Glob patterns for use in finding where the JDK is.
+# These are pairs, *dir_glob used in the general case,
+# *version_dir_glob if matching only a specific version.
+# For now only used for Windows.
 java_win32_dir_glob = 'C:/Program Files*/Java/jdk*/bin'
-
-java_macos_include_dir = '/System/Library/Frameworks/JavaVM.framework/Headers/'
-java_macos_version_include_dir = '/System/Library/Frameworks/JavaVM.framework/Versions/%s*/Headers/'
-
-java_linux_include_dirs = ['/usr/lib/jvm/default-java/include',
-                        '/usr/lib/jvm/java-*/include']
-# Need to match path like below (from Centos 7)
-# /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.191.b12-0.el7_5.x86_64/include/
-java_linux_version_include_dirs = ['/usr/lib/jvm/java-*-sun-%s*/include',
-                                   '/usr/lib/jvm/java-%s*-openjdk*/include',
-                                   '/usr/java/jdk%s*/include']
-
-
+# On windows, since Java 9, there is a dash between 'jdk' and the version
+# string that wasn't there before. this glob should catch either way.
+java_win32_version_dir_glob = 'C:/Program Files*/Java/jdk*%s*/bin'
 
 def get_java_install_dirs(platform, version=None):
     """
-    Using patterns above find the java jdk install dir
-    :param platform:
+    Find the java jdk installation directories.
+
+    This list is intended to supply as "default paths" for use when looking
+    up actual java binaries.
+
+    :param platform: selector for search algorithm.
     :param version: If specified, only look for java sdk's of this version
     :return: list of default paths for java.
     """
+
     paths = []
     if platform == 'win32':
         if version:
@@ -426,25 +423,45 @@ def get_java_install_dirs(platform, version=None):
         else:
             paths = glob.glob(java_win32_dir_glob)
     else:
-        # do nothing for now
+        # other platforms, do nothing for now
         pass
 
-    paths=sorted(paths)
+    return sorted(paths)
 
-    return paths
+
+# Glob patterns for use in finding where the JDK headers are.
+# These are pairs, *dir_glob used in the general case,
+# *version_dir_glob if matching only a specific version.
+java_macos_include_dir = '/System/Library/Frameworks/JavaVM.framework/Headers/'
+java_macos_version_include_dir = '/System/Library/Frameworks/JavaVM.framework/Versions/%s*/Headers/'
+
+java_linux_include_dirs = [
+    '/usr/lib/jvm/default-java/include',
+    '/usr/lib/jvm/java-*/include'
+]
+# Need to match path like below (from Centos 7)
+# /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.191.b12-0.el7_5.x86_64/include/
+java_linux_version_include_dirs = [
+    '/usr/lib/jvm/java-*-sun-%s*/include',
+    '/usr/lib/jvm/java-%s*-openjdk*/include',
+    '/usr/java/jdk%s*/include'
+]
 
 def get_java_include_paths(env, javac, version):
     """
-    Return java include paths
-    :param platform:
-    :param javac:
-    :return:
+    Find java include paths for JNI building.
+
+    :param env: construction environment, used to extract platform.
+    :param javac: path to detected javac.
+    :return: list of paths.
     """
+
     paths = []
     if not javac:
         # there are no paths if we've not detected javac.
         pass
     elif env['PLATFORM'] == 'win32':
+        # on Windows, we have the right path to javac, so look locally
         javac_bin_dir = os.path.dirname(javac)
         java_inc_dir = os.path.normpath(os.path.join(javac_bin_dir, '..', 'include'))
         paths = [java_inc_dir, os.path.join(java_inc_dir, 'win32')]
@@ -469,8 +486,6 @@ def get_java_include_paths(env, javac, version):
     return paths
 
 
-
-
 # Local Variables:
 # tab-width:4
 # indent-tabs-mode:nil
diff --git a/src/engine/SCons/Tool/jar.py b/src/engine/SCons/Tool/jar.py
index e0a6a69..02bda57 100644
--- a/src/engine/SCons/Tool/jar.py
+++ b/src/engine/SCons/Tool/jar.py
@@ -209,9 +209,9 @@ def generate(env):
     env.AddMethod(Jar)
 
     if env['PLATFORM'] == 'win32':
-        # Ensure that we have a proper path for clang
-        jar = SCons.Tool.find_program_path(env, 'jar',
-                                             default_paths=get_java_install_dirs(env['PLATFORM']))
+        # Ensure that we have a proper path for jar
+        paths = get_java_install_dirs('win32')
+        jar = SCons.Tool.find_program_path(env, 'jar', default_paths=paths)
         if jar:
             jar_bin_dir = os.path.dirname(jar)
             env.AppendENVPath('PATH', jar_bin_dir)
diff --git a/src/engine/SCons/Tool/javac.py b/src/engine/SCons/Tool/javac.py
index 8d98b54..537913f 100644
--- a/src/engine/SCons/Tool/javac.py
+++ b/src/engine/SCons/Tool/javac.py
@@ -210,15 +210,15 @@ def generate(env):
 
     version = env.get('JAVAVERSION', None)
 
-    javac = SCons.Tool.find_program_path(env, 'javac')
     if env['PLATFORM'] == 'win32':
         # Ensure that we have a proper path for javac
-        paths=get_java_install_dirs(env['PLATFORM'], version=version)
-        javac = SCons.Tool.find_program_path(env, 'javac',
-                                             default_paths=paths)
+        paths = get_java_install_dirs('win32', version=version)
+        javac = SCons.Tool.find_program_path(env, 'javac', default_paths=paths)
         if javac:
             javac_bin_dir = os.path.dirname(javac)
             env.AppendENVPath('PATH', javac_bin_dir)
+    else:
+        javac = SCons.Tool.find_program_path(env, 'javac')
 
     env['JAVAINCLUDES'] = get_java_include_paths(env, javac, version)
 
diff --git a/src/engine/SCons/Tool/javah.py b/src/engine/SCons/Tool/javah.py
index f514479..80f8a6b 100644
--- a/src/engine/SCons/Tool/javah.py
+++ b/src/engine/SCons/Tool/javah.py
@@ -123,9 +123,9 @@ def generate(env):
     java_javah.emitter = emit_java_headers
 
     if env['PLATFORM'] == 'win32':
-        # Ensure that we have a proper path for clang
-        javah = SCons.Tool.find_program_path(env, 'javah',
-                                             default_paths=get_java_install_dirs(env['PLATFORM']))
+        # Ensure that we have a proper path for javah
+        paths = get_java_install_dirs('win32')
+        javah = SCons.Tool.find_program_path(env, 'javah', default_paths=paths)
         if javah:
             javah_bin_dir = os.path.dirname(javah)
             env.AppendENVPath('PATH', javah_bin_dir)
diff --git a/src/engine/SCons/Tool/rmic.py b/src/engine/SCons/Tool/rmic.py
index 173ef5f..5c7a040 100644
--- a/src/engine/SCons/Tool/rmic.py
+++ b/src/engine/SCons/Tool/rmic.py
@@ -110,11 +110,9 @@ def generate(env):
 
     if env['PLATFORM'] == 'win32':
         version = env.get('JAVAVERSION', None)
-        default_paths=get_java_install_dirs(env['PLATFORM'], version=version)
-
         # Ensure that we have a proper path for rmic
-        rmic = SCons.Tool.find_program_path(env, 'rmic', default_paths=default_paths)
-
+        paths = get_java_install_dirs('win32', version=version)
+        rmic = SCons.Tool.find_program_path(env, 'rmic', default_paths=paths)
         # print("RMIC: %s"%rmic)
         if rmic:
             rmic_bin_dir = os.path.dirname(rmic)
diff --git a/test/Repository/RMIC.py b/test/Repository/RMIC.py
index 433890f..3eeab9b 100644
--- a/test/Repository/RMIC.py
+++ b/test/Repository/RMIC.py
@@ -46,6 +46,12 @@ if java_version.count('.') == 1:
         curver = (int(major), int(minor))
     except:
         pass
+elif java_version.count('.') == 0:
+    # java 11?
+    try:
+        curver = (int(java_version), 0)
+    except:
+        pass
 
 # Check the version of the found Java compiler.
 # If it's 1.8 or higher, we skip the further RMIC test
diff --git a/testing/framework/TestSCons.py b/testing/framework/TestSCons.py
index 91a443e..23e8753 100644
--- a/testing/framework/TestSCons.py
+++ b/testing/framework/TestSCons.py
@@ -682,6 +682,9 @@ class TestSCons(TestCommon):
         """
         Initialize with a default external environment that uses a local
         Java SDK in preference to whatever's found in the default PATH.
+
+        :param version: if set, match only that version
+        :return: the new env.
         """
         if not self.external:
             try:
@@ -698,11 +701,11 @@ class TestSCons(TestCommon):
             if version:
                 if sys.platform == 'win32':
                     patterns = [
-                        'C:/Program Files*/Java/jdk%s*/bin'%version,
+                        'C:/Program Files*/Java/jdk*%s*/bin' % version,
                     ]
                 else:
                     patterns = [
-                        '/usr/java/jdk%s*/bin'    % version,
+                        '/usr/java/jdk%s*/bin' % version,
                         '/usr/lib/jvm/*-%s*/bin' % version,
                         '/usr/local/j2sdk%s*/bin' % version,
                     ]
@@ -727,7 +730,10 @@ class TestSCons(TestCommon):
 
     def java_where_includes(self,version=None):
         """
-        Return java include paths compiling java jni code
+        Find include path needed for compiling java jni code.
+
+        :param version: if set, match only that version
+        :return: path to java headers
         """
         import sys
 
@@ -761,6 +767,14 @@ class TestSCons(TestCommon):
         return result
 
     def java_where_java_home(self, version=None):
+        """
+        Find path to what would be JAVA_HOME.
+
+        SCons does not read JAVA_HOME from the environment, so deduce it.
+
+        :param version: if set, match only that version
+        :return: path where JDK components live
+        """
         if sys.platform[:6] == 'darwin':
             # osx 10.11, 10.12
             home_tool = '/usr/libexec/java_home'
@@ -807,6 +821,12 @@ class TestSCons(TestCommon):
             self.skip_test("Could not find Java " + java_bin_name + ", skipping test(s).\n")
 
     def java_where_jar(self, version=None):
+        """
+        Find java archiver jar.
+
+        :param version: if set, match only that version
+        :return: path to jar
+        """
         ENV = self.java_ENV(version)
         if self.detect_tool('jar', ENV=ENV):
             where_jar = self.detect('JAR', 'jar', ENV=ENV)
@@ -821,7 +841,10 @@ class TestSCons(TestCommon):
 
     def java_where_java(self, version=None):
         """
-        Return a path to the java executable.
+        Find java executable.
+
+        :param version: if set, match only that version
+        :return: path to the java rutime
         """
         ENV = self.java_ENV(version)
         where_java = self.where_is('java', ENV['PATH'])
@@ -835,7 +858,10 @@ class TestSCons(TestCommon):
 
     def java_where_javac(self, version=None):
         """
-        Return a path to the javac compiler.
+        Find java compiler.
+
+        :param version: if set, match only that version
+        :return: path to javac
         """
         ENV = self.java_ENV(version)
         if self.detect_tool('javac'):
@@ -851,15 +877,17 @@ class TestSCons(TestCommon):
                  arguments = '-version',
                  stderr=None,
                  status=None)
+        # Note recent versions output version info to stdout instead of stderr
         if version:
-            if self.stderr().find('javac %s' % version) == -1:
+            verf = 'javac %s' % version
+            if self.stderr().find(verf) == -1 and self.stdout().find(verf) == -1:
                 fmt = "Could not find javac for Java version %s, skipping test(s).\n"
                 self.skip_test(fmt % version)
         else:
-            m = re.search(r'javac (\d\.*\d)', self.stderr())
-            # Java 11 outputs this to stdout
+            version_re = r'javac (\d*\.*\d)'
+            m = re.search(version_re, self.stderr())
             if not m:
-                m = re.search(r'javac (\d\.*\d)', self.stdout())
+                m = re.search(version_re, self.stdout())
 
             if m:
                 version = m.group(1)
@@ -873,6 +901,16 @@ class TestSCons(TestCommon):
         return where_javac, version
 
     def java_where_javah(self, version=None):
+        """
+        Find java header generation tool.
+
+        TODO issue #3347 since JDK10, there is no separate javah command,
+        'javac -h' is used. We should not return a javah from a different
+        installed JDK - how to detect and what to return in this case?
+
+        :param version: if set, match only that version
+        :return: path to javah
+        """
         ENV = self.java_ENV(version)
         if self.detect_tool('javah'):
             where_javah = self.detect('JAVAH', 'javah', ENV=ENV)
@@ -883,6 +921,12 @@ class TestSCons(TestCommon):
         return where_javah
 
     def java_where_rmic(self, version=None):
+        """
+        Find java rmic tool.
+
+        :param version: if set, match only that version
+        :return: path to rmic
+        """
         ENV = self.java_ENV(version)
         if self.detect_tool('rmic'):
             where_rmic = self.detect('RMIC', 'rmic', ENV=ENV)