module parsing: make heuristic to get paths from modules more robust (#12693)

* module parsing: make heuristic to get paths from modules more robust

* refactor module parsing

2 files changed, 87 insertions, 36 deletions

diff --git a/lib/spack/spack/test/module_parsing.py b/lib/spack/spack/test/module_parsing.py
index 0594724eaf..95a65ab33f 100644
--- a/lib/spack/spack/test/module_parsing.py
+++ b/lib/spack/spack/test/module_parsing.py
@@ -10,7 +10,7 @@ import spack
 from spack.util.module_cmd import (
     module,
     get_path_from_module,
-    get_path_arg_from_module_line,
+    get_path_args_from_module_line,
     get_path_from_module_contents
 )
 
@@ -89,14 +89,23 @@ whatis("Name: CMake")
 whatis("Version: 3.9.2")
 whatis("Category: Tools")
 whatis("URL: https://cmake.org/")
-prepend_path("PATH","/path/to/cmake-3.9.2/bin")
+prepend_path("LD_LIBRARY_PATH","/bad/path")
+prepend_path("PATH","/path/to/cmake-3.9.2/bin:/other/bad/path")
 prepend_path("MANPATH","/path/to/cmake/cmake-3.9.2/share/man")
+prepend_path("LD_LIBRARY_PATH","/path/to/cmake-3.9.2/lib64")
 """
     module_show_lines = module_show_output.split('\n')
+
+    # PATH and LD_LIBRARY_PATH outvote MANPATH and the other PATH and
+    # LD_LIBRARY_PATH entries
     assert (get_path_from_module_contents(module_show_lines, 'cmake-3.9.2') ==
             '/path/to/cmake-3.9.2')
 
 
+def test_get_path_from_empty_module():
+    assert get_path_from_module_contents('', 'test') is None
+
+
 def test_pkg_dir_from_module_name():
     module_show_lines = ['setenv FOO_BAR_DIR /path/to/foo-bar']
 
@@ -108,16 +117,25 @@ def test_pkg_dir_from_module_name():
 
 
 def test_get_argument_from_module_line():
-    lines = ['prepend-path LD_LIBRARY_PATH /lib/path',
-             'prepend-path  LD_LIBRARY_PATH  /lib/path',
-             "prepend_path('PATH' , '/lib/path')",
-             'prepend_path( "PATH" , "/lib/path" )',
-             'prepend_path("PATH",' + "'/lib/path')"]
+    simple_lines = ['prepend-path LD_LIBRARY_PATH /lib/path',
+                    'prepend-path  LD_LIBRARY_PATH  /lib/path',
+                    "prepend_path('PATH' , '/lib/path')",
+                    'prepend_path( "PATH" , "/lib/path" )',
+                    'prepend_path("PATH",' + "'/lib/path')"]
+
+    complex_lines = ['prepend-path LD_LIBRARY_PATH /lib/path:/pkg/path',
+                     'prepend-path  LD_LIBRARY_PATH  /lib/path:/pkg/path',
+                     "prepend_path('PATH' , '/lib/path:/pkg/path')",
+                     'prepend_path( "PATH" , "/lib/path:/pkg/path" )',
+                     'prepend_path("PATH",' + "'/lib/path:/pkg/path')"]
 
     bad_lines = ['prepend_path(PATH,/lib/path)',
                  'prepend-path (LD_LIBRARY_PATH) /lib/path']
 
-    assert all(get_path_arg_from_module_line(l) == '/lib/path' for l in lines)
+    assert all(get_path_args_from_module_line(l) == ['/lib/path']
+               for l in simple_lines)
+    assert all(get_path_args_from_module_line(l) == ['/lib/path', '/pkg/path']
+               for l in complex_lines)
     for bl in bad_lines:
         with pytest.raises(ValueError):
-            get_path_arg_from_module_line(bl)
+            get_path_args_from_module_line(bl)
diff --git a/lib/spack/spack/util/module_cmd.py b/lib/spack/spack/util/module_cmd.py
index 581fae5540..75f61a6466 100644
--- a/lib/spack/spack/util/module_cmd.py
+++ b/lib/spack/spack/util/module_cmd.py
@@ -77,7 +77,7 @@ def load_module(mod):
     module('load', mod)
 
 
-def get_path_arg_from_module_line(line):
+def get_path_args_from_module_line(line):
     if '(' in line and ')' in line:
         # Determine which lua quote symbol is being used for the argument
         comma_index = line.index(',')
@@ -92,7 +92,9 @@ def get_path_arg_from_module_line(line):
         path_arg = words_and_symbols[-2]
     else:
         path_arg = line.split()[2]
-    return path_arg
+
+    paths = path_arg.split(':')
+    return paths
 
 
 def get_path_from_module(mod):
@@ -119,37 +121,68 @@ def get_path_from_module_contents(text, module_name):
         pkg_var_prefix = components[-2]
     tty.debug("Package directory variable prefix: " + pkg_var_prefix)
 
-    # If it sets the LD_LIBRARY_PATH or CRAY_LD_LIBRARY_PATH, use that
-    for line in text:
-        pattern = r'\W(CRAY_)?LD_LIBRARY_PATH'
-        if re.search(pattern, line):
-            path = get_path_arg_from_module_line(line)
-            return path[:path.find('/lib')]
+    path_occurrences = {}
 
-    # If it lists its package directory, return that
-    for line in text:
-        pattern = r'\W{0}_DIR'.format(pkg_var_prefix)
+    def strip_path(path, endings):
+        for ending in endings:
+            if path.endswith(ending):
+                return path[:-len(ending)]
+            if path.endswith(ending + '/'):
+                return path[:-(len(ending) + 1)]
+        return path
+
+    def match_pattern_and_strip(line, pattern, strip=[]):
         if re.search(pattern, line):
-            return get_path_arg_from_module_line(line)
+            paths = get_path_args_from_module_line(line)
+            for path in paths:
+                path = strip_path(path, strip)
+                path_occurrences[path] = path_occurrences.get(path, 0) + 1
+
+    def match_flag_and_strip(line, flag, strip=[]):
+        flag_idx = line.find(flag)
+        if flag_idx >= 0:
+            end = line.find(' ', flag_idx)
+            if end >= 0:
+                path = line[flag_idx + len(flag):end]
+            else:
+                path = line[flag_idx + len(flag):]
+            path = strip_path(path, strip)
+            path_occurrences[path] = path_occurrences.get(path, 0) + 1
+
+    lib_endings = ['/lib64', '/lib']
+    bin_endings = ['/bin']
+    man_endings = ['/share/man', '/man']
 
-    # If it lists a -rpath instruction, use that
     for line in text:
-        rpath = line.find('-rpath/')
-        if rpath >= 0:
-            return line[rpath + 6:line.find('/lib')]
+        # Check entries of LD_LIBRARY_PATH and CRAY_LD_LIBRARY_PATH
+        pattern = r'\W(CRAY_)?LD_LIBRARY_PATH'
+        match_pattern_and_strip(line, pattern, lib_endings)
 
-    # If it lists a -L instruction, use that
-    for line in text:
-        lib_paths = line.find('-L/')
-        if lib_paths >= 0:
-            return line[lib_paths + 2:line.find('/lib')]
+        # Check {name}_DIR entries
+        pattern = r'\W{0}_DIR'.format(pkg_var_prefix)
+        match_pattern_and_strip(line, pattern)
 
-    # If it sets the PATH, use it
-    for line in text:
+        # Check {name}_ROOT entries
+        pattern = r'\W{0}_ROOT'.format(pkg_var_prefix)
+        match_pattern_and_strip(line, pattern)
+
+        # Check entries that update the PATH variable
         pattern = r'\WPATH'
-        if re.search(pattern, line):
-            path = get_path_arg_from_module_line(line)
-            return path[:path.find('/bin')]
+        match_pattern_and_strip(line, pattern, bin_endings)
+
+        # Check entries that update the MANPATH variable
+        pattern = r'MANPATH'
+        match_pattern_and_strip(line, pattern, man_endings)
+
+        # Check entries that add a `-rpath` flag to a variable
+        match_flag_and_strip(line, '-rpath', lib_endings)
+
+        # Check entries that add a `-L` flag to a variable
+        match_flag_and_strip(line, '-L', lib_endings)
+
+    # Whichever path appeared most in the module, we assume is the correct path
+    if len(path_occurrences) > 0:
+        return max(path_occurrences.items(), key=lambda x: x[1])[0]
 
-    # Unable to find module path
+    # Unable to find path in module
     return None