Skip to content

filter out duplicate paths added to module files #3770

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Oct 21, 2021
Merged

filter out duplicate paths added to module files #3770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dff7ec5
Filter out duplicate paths added to module files
Flamefire Jul 7, 2021
e026a09
Use print_warning
Flamefire Jul 7, 2021
a4dd70e
Use a contextmanager instead
Flamefire Jul 7, 2021
75a4195
Add line break
Flamefire Jul 7, 2021
8885102
Check paths per key not globally
Flamefire Jul 8, 2021
eace681
Fix test
Flamefire Jul 8, 2021
cd0a7b3
Fix extensions writing to modules
Flamefire Jul 9, 2021
e7a211c
Fix adding root path (empty string)
Flamefire Jul 16, 2021
3048a49
Update tests to use filtering module generator
Flamefire Jul 16, 2021
146773e
Fix formatting
Flamefire Jul 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions easybuild/framework/easyblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2441,7 +2441,8 @@ def extensions_step(self, fetch=False, install=True):
if install:
try:
ext.prerun()
txt = ext.run()
with self.module_generator.start_module_creation():
txt = ext.run()
if txt:
self.module_extra_extensions += txt
ext.postrun()
Expand Down Expand Up @@ -3210,21 +3211,18 @@ def make_module_step(self, fake=False):
else:
trace_msg("generating module file @ %s" % self.mod_filepath)

txt = self.module_generator.MODULE_SHEBANG
if txt:
txt += '\n'

if self.modules_header:
txt += self.modules_header + '\n'

txt += self.make_module_description()
txt += self.make_module_group_check()
txt += self.make_module_deppaths()
txt += self.make_module_dep()
txt += self.make_module_extend_modpath()
txt += self.make_module_req()
txt += self.make_module_extra()
txt += self.make_module_footer()
with self.module_generator.start_module_creation() as txt:
if self.modules_header:
txt += self.modules_header + '\n'

txt += self.make_module_description()
txt += self.make_module_group_check()
txt += self.make_module_deppaths()
txt += self.make_module_dep()
txt += self.make_module_extend_modpath()
txt += self.make_module_req()
txt += self.make_module_extra()
txt += self.make_module_footer()

hook_txt = run_hook(MODULE_WRITE, self.hooks, args=[self, mod_filepath, txt])
if hook_txt is not None:
Expand Down
75 changes: 67 additions & 8 deletions easybuild/tools/module_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,12 @@
import os
import re
import tempfile
from contextlib import contextmanager
from distutils.version import LooseVersion
from textwrap import wrap

from easybuild.base import fancylogger
from easybuild.tools.build_log import EasyBuildError
from easybuild.tools.build_log import EasyBuildError, print_warning
from easybuild.tools.config import build_option, get_module_syntax, install_path
from easybuild.tools.filetools import convert_name, mkdir, read_file, remove_file, resolve_path, symlink, write_file
from easybuild.tools.modules import ROOT_ENV_VAR_NAME_PREFIX, EnvironmentModulesC, Lmod, modules_tool
Expand Down Expand Up @@ -136,17 +137,29 @@ def __init__(self, application, fake=False):
self.fake_mod_path = tempfile.mkdtemp()

self.modules_tool = modules_tool()
self.added_paths_per_key = None

def append_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
@contextmanager
def start_module_creation(self):
"""
Generate append-path statements for the given list of paths.
Prepares creating a module and returns the file header (shebang) if any including the newline

:param key: environment variable to append paths to
:param paths: list of paths to append
:param allow_abs: allow providing of absolute paths
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
Meant to be used in a with statement:
with generator.start_module_creation() as txt:
# Write txt
"""
return self.update_paths(key, paths, prepend=False, allow_abs=allow_abs, expand_relpaths=expand_relpaths)
if self.added_paths_per_key is not None:
raise EasyBuildError('Module creation already in process. '
'You cannot create multiple modules at the same time!')
# Mapping of keys/env vars to paths already added
self.added_paths_per_key = dict()
txt = self.MODULE_SHEBANG
if txt:
txt += '\n'
try:
yield txt
finally:
self.added_paths_per_key = None

def create_symlinks(self, mod_symlink_paths, fake=False):
"""Create moduleclass symlink(s) to actual module file."""
Expand Down Expand Up @@ -191,6 +204,49 @@ def get_modules_path(self, fake=False, mod_path_suffix=None):

return os.path.join(mod_path, mod_path_suffix)

def _filter_paths(self, key, paths):
"""Filter out paths already added to key and return the remaining ones"""
if self.added_paths_per_key is None:
# For compatibility this is only a warning for now and we don't filter any paths
print_warning('Module creation has not been started. Call start_module_creation first!')
return paths

added_paths = self.added_paths_per_key.setdefault(key, set())
# paths can be a string
if isinstance(paths, string_type):
if paths in added_paths:
filtered_paths = None
else:
added_paths.add(paths)
filtered_paths = paths
else:
# Coerce any iterable/generator into a list
if not isinstance(paths, list):
paths = list(paths)
filtered_paths = [x for x in paths if x not in added_paths and not added_paths.add(x)]
if filtered_paths != paths:
removed_paths = paths if filtered_paths is None else [x for x in paths if x not in filtered_paths]
print_warning("Supressed adding the following path(s) to $%s of the module as they were already added: %s",
Copy link
Member

@boegel boegel Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo here, should be Suppressed (double p); fixed in #3874

Copy link
Contributor Author

@Flamefire Flamefire Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Always forget the double double letter in that word... ;)

key, removed_paths,
log=self.log)
if not filtered_paths:
filtered_paths = None
return filtered_paths

def append_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
"""
Generate append-path statements for the given list of paths.

:param key: environment variable to append paths to
:param paths: list of paths to append
:param allow_abs: allow providing of absolute paths
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
"""
paths = self._filter_paths(key, paths)
if paths is None:
return ''
return self.update_paths(key, paths, prepend=False, allow_abs=allow_abs, expand_relpaths=expand_relpaths)

def prepend_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
"""
Generate prepend-path statements for the given list of paths.
Expand All @@ -200,6 +256,9 @@ def prepend_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
:param allow_abs: allow providing of absolute paths
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
"""
paths = self._filter_paths(key, paths)
if paths is None:
return ''
return self.update_paths(key, paths, prepend=True, allow_abs=allow_abs, expand_relpaths=expand_relpaths)

def _modulerc_check_module_version(self, module_version):
Expand Down
49 changes: 33 additions & 16 deletions test/framework/easyblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,8 @@ def test_make_module_req(self):
# this is not a path that should be picked up
os.mkdir(os.path.join(eb.installdir, 'CPATH'))

guess = eb.make_module_req()
with eb.module_generator.start_module_creation():
guess = eb.make_module_req()

if get_module_syntax() == 'Tcl':
self.assertTrue(re.search(r"^prepend-path\s+CLASSPATH\s+\$root/bla.jar$", guess, re.M))
Expand All @@ -462,7 +463,8 @@ def test_make_module_req(self):

# check that bin is only added to PATH if there are files in there
write_file(os.path.join(eb.installdir, 'bin', 'test'), 'test')
guess = eb.make_module_req()
with eb.module_generator.start_module_creation():
guess = eb.make_module_req()
if get_module_syntax() == 'Tcl':
self.assertTrue(re.search(r"^prepend-path\s+PATH\s+\$root/bin$", guess, re.M))
self.assertFalse(re.search(r"^prepend-path\s+PATH\s+\$root/sbin$", guess, re.M))
Expand All @@ -481,7 +483,8 @@ def test_make_module_req(self):
self.assertFalse('prepend_path("CMAKE_LIBRARY_PATH", pathJoin(root, "lib64"))' in guess)
# -- With files
write_file(os.path.join(eb.installdir, 'lib64', 'libfoo.so'), 'test')
guess = eb.make_module_req()
with eb.module_generator.start_module_creation():
guess = eb.make_module_req()
if get_module_syntax() == 'Tcl':
self.assertTrue(re.search(r"^prepend-path\s+CMAKE_LIBRARY_PATH\s+\$root/lib64$", guess, re.M))
elif get_module_syntax() == 'Lua':
Expand All @@ -490,7 +493,8 @@ def test_make_module_req(self):
write_file(os.path.join(eb.installdir, 'lib', 'libfoo.so'), 'test')
shutil.rmtree(os.path.join(eb.installdir, 'lib64'))
os.symlink('lib', os.path.join(eb.installdir, 'lib64'))
guess = eb.make_module_req()
with eb.module_generator.start_module_creation():
guess = eb.make_module_req()
if get_module_syntax() == 'Tcl':
self.assertFalse(re.search(r"^prepend-path\s+CMAKE_LIBRARY_PATH\s+\$root/lib64$", guess, re.M))
elif get_module_syntax() == 'Lua':
Expand All @@ -509,7 +513,8 @@ def test_make_module_req(self):

# check for behavior when a string value is used as dict value by make_module_req_guesses
eb.make_module_req_guess = lambda: {'PATH': 'bin'}
txt = eb.make_module_req()
with eb.module_generator.start_module_creation():
txt = eb.make_module_req()
if get_module_syntax() == 'Tcl':
self.assertTrue(re.match(r"^\nprepend-path\s+PATH\s+\$root/bin\n$", txt, re.M))
elif get_module_syntax() == 'Lua':
Expand All @@ -520,7 +525,8 @@ def test_make_module_req(self):
# check for correct behaviour if empty string is specified as one of the values
# prepend-path statements should be included for both the 'bin' subdir and the install root
eb.make_module_req_guess = lambda: {'PATH': ['bin', '']}
txt = eb.make_module_req()
with eb.module_generator.start_module_creation():
txt = eb.make_module_req()
if get_module_syntax() == 'Tcl':
self.assertTrue(re.search(r"\nprepend-path\s+PATH\s+\$root/bin\n", txt, re.M))
self.assertTrue(re.search(r"\nprepend-path\s+PATH\s+\$root\n", txt, re.M))
Expand All @@ -535,7 +541,8 @@ def test_make_module_req(self):
for path in ['pathA', 'pathB', 'pathC']:
os.mkdir(os.path.join(eb.installdir, 'lib', path))
write_file(os.path.join(eb.installdir, 'lib', path, 'libfoo.so'), 'test')
txt = eb.make_module_req()
with eb.module_generator.start_module_creation():
txt = eb.make_module_req()
if get_module_syntax() == 'Tcl':
self.assertTrue(re.search(r"\nprepend-path\s+LD_LIBRARY_PATH\s+\$root/lib/pathC\n" +
r"prepend-path\s+LD_LIBRARY_PATH\s+\$root/lib/pathA\n" +
Expand Down Expand Up @@ -1152,7 +1159,7 @@ def test_make_module_step(self):
# purposely use a 'nasty' description, that includes (unbalanced) special chars: [, ], {, }
descr = "This {is a}} [fancy]] [[description]]. {{[[TEST}]"
modextravars = {'PI': '3.1415', 'FOO': 'bar'}
modextrapaths = {'PATH': 'pibin', 'CPATH': 'pi/include'}
modextrapaths = {'PATH': ('bin', 'pibin'), 'CPATH': 'pi/include'}
self.contents = '\n'.join([
'easyblock = "ConfigureMake"',
'name = "%s"' % name,
Expand All @@ -1177,6 +1184,10 @@ def test_make_module_step(self):
eb.make_builddir()
eb.prepare_step()

# Create a dummy file in bin to test if the duplicate entry of modextrapaths is ignored
os.makedirs(os.path.join(eb.installdir, 'bin'))
write_file(os.path.join(eb.installdir, 'bin', 'dummy_exe'), 'hello')

modpath = os.path.join(eb.make_module_step(), name, version)
if get_module_syntax() == 'Lua':
modpath += '.lua'
Expand Down Expand Up @@ -1210,14 +1221,20 @@ def test_make_module_step(self):
self.assertTrue(False, "Unknown module syntax: %s" % get_module_syntax())
self.assertTrue(regex.search(txt), "Pattern %s found in %s" % (regex.pattern, txt))

for (key, val) in modextrapaths.items():
if get_module_syntax() == 'Tcl':
regex = re.compile(r'^prepend-path\s+%s\s+\$root/%s$' % (key, val), re.M)
elif get_module_syntax() == 'Lua':
regex = re.compile(r'^prepend_path\("%s", pathJoin\(root, "%s"\)\)$' % (key, val), re.M)
else:
self.assertTrue(False, "Unknown module syntax: %s" % get_module_syntax())
self.assertTrue(regex.search(txt), "Pattern %s found in %s" % (regex.pattern, txt))
for (key, vals) in modextrapaths.items():
if isinstance(vals, string_type):
vals = [vals]
for val in vals:
if get_module_syntax() == 'Tcl':
regex = re.compile(r'^prepend-path\s+%s\s+\$root/%s$' % (key, val), re.M)
elif get_module_syntax() == 'Lua':
regex = re.compile(r'^prepend_path\("%s", pathJoin\(root, "%s"\)\)$' % (key, val), re.M)
else:
self.assertTrue(False, "Unknown module syntax: %s" % get_module_syntax())
self.assertTrue(regex.search(txt), "Pattern %s found in %s" % (regex.pattern, txt))
# Check for duplicates
num_prepends = len(regex.findall(txt))
self.assertEqual(num_prepends, 1, "Expected exactly 1 %s command in %s" % (regex.pattern, txt))

for (name, ver) in [('GCC', '6.4.0-2.28')]:
if get_module_syntax() == 'Tcl':
Expand Down
Loading