Skip to content
Closed
Changes from 1 commit
Commits
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
19 changes: 6 additions & 13 deletions easybuild/tools/module_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,25 +213,20 @@ def _filter_paths(self, key, paths):
return paths

added_paths = self.added_paths_per_key.setdefault(key, set())
# 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("Suppressed adding the following path(s) to $%s of the module as they were already added: %s",
key, ', '.join(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 paths: single or 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)
"""
Expand All @@ -242,7 +237,7 @@ def prepend_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
Generate prepend-path statements for the given list of paths.

:param key: environment variable to append paths to
:param paths: list of paths to append
:param paths: single or 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)
"""
Expand All @@ -253,13 +248,15 @@ def update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpath
Generate append/prepend-path statements for the given list of paths.

:param key: environment variable to append paths to
:param paths: list of paths to append
:param paths: single or 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)
"""
if isinstance(paths, str):
self.log.debug("Wrapping %s into a list before using it for %s", paths, key)
paths = [paths]
elif not isinstance(paths, list):
paths = list(paths) # coerce any iterator into a list

if key == 'PYTHONPATH':
python_paths = [path for path in paths if re.match(r'lib/python\d+\.\d+/site-packages', path)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking the logic in _filter_paths I see an issue here: You iterate paths multiple times: Here and at https://github.com/easybuilders/easybuild-framework/pull/4496/files#diff-4af72e9777d353325a29df400ab4229e14defd653e413db51786dc2172d3baefR275 but inside _update_paths you call _filter_paths which "coerces a generator into a list" if it isn't a list yet.

This means it this point paths might be a generator which can only be iterated once as otherwise the code in _filter_paths wasn't required.

Similar to my previous reasoning about breaking change in favor of cleaner code: I would simply disallow passing anything but a a string or a list to update_paths and assert that it is a list in _filter_paths instead of trying to convert a potentially exhausted generator to a list here or there.

Quick example:

paths=(i for i in range(10))
[i for i in paths if i==2] # [2]
[i for i in paths if i==2] # []

Expand Down Expand Up @@ -976,14 +973,12 @@ def _update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpat
Generate prepend-path or append-path statements for the given list of paths.

:param key: environment variable to prepend/append paths to
:param paths: list of paths to prepend
:param paths: list of paths to prepend/append
:param prepend: whether to prepend (True) or append (False) paths
: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 ''

if prepend:
update_type = 'prepend'
Expand Down Expand Up @@ -1453,8 +1448,6 @@ def _update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpat
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
"""
paths = self._filter_paths(key, paths)
if paths is None:
return ''

if prepend:
update_type = 'prepend'
Expand Down