Skip to content

fix filtering of duplicate paths for a particular environment variable #4760

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

Closed
boegel wants to merge 8 commits into from
Closed

fix filtering of duplicate paths for a particular environment variable #4760

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
67a335f
fix filtering of duplicate paths for a particular environment variable
boegel Feb 10, 2025
78b8837
remove EasyBlock.install_lib_symlink, no longer needed
boegel Feb 10, 2025
8754f3d
fix logic for filtering duplicate paths in EasyBlock.make_module_req …
boegel Feb 10, 2025
5bd211a
get rid of unused idx in EasyBlock.make_module_req
boegel Feb 10, 2025
5e3111e
fix crash in EasyBlock.make_module_req when using 'eb -x'
boegel Feb 10, 2025
88c3f7d
remove LibSymlink, no longer used
boegel Feb 10, 2025
c57021f
add extra test case for make_module_req
boegel Feb 10, 2025
a95be5b
fix long lines in test_make_module_req
boegel Feb 10, 2025
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
67 changes: 32 additions & 35 deletions easybuild/framework/easyblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,6 @@ def __init__(self, ec, logfile=None):
# determine install subdirectory, based on module name
self.install_subdir = None

# track status of symlink between library directories
self._install_lib_symlink = None

# indicates whether build should be performed in installation dir
self.build_in_installdir = self.cfg['buildininstalldir']

Expand Down Expand Up @@ -311,13 +308,6 @@ def __init__(self, ec, logfile=None):

self.log.info("Init completed for application name %s version %s" % (self.name, self.version))

@property
def install_lib_symlink(self):
"""Return symlink state of lib/lib64 folders"""
if self._install_lib_symlink is None:
self.check_install_lib_symlink()
return self._install_lib_symlink

def post_init(self):
"""
Run post-initialization tasks.
Expand Down Expand Up @@ -1702,7 +1692,38 @@ def make_module_req(self):
mod_req_paths.extend(self.expand_module_search_path(path, path_type=search_paths.type))

if mod_req_paths:
mod_req_paths = nub(mod_req_paths) # remove duplicates
# find duplicate paths (taking into account possible symlinks)
dup_paths = []
# always retain first entry
retained_paths = [mod_req_paths[0]]
full_retained_paths = [os.path.join(self.installdir, retained_paths[0])]

for idx, path in enumerate(mod_req_paths[1:]):
full_path = os.path.join(self.installdir, path)
# retain all paths in dry run mode (since then paths may not exist)
if self.dry_run:
retained_paths.append(path)
elif os.path.exists(full_path) and any(os.path.samefile(full_path, p) for p in full_retained_paths):
dup_paths.append(path)
else:
retained_paths.append(path)
full_retained_paths = [os.path.join(self.installdir, p) for p in retained_paths]

if dup_paths:
self.log.info(f"Filtering out duplicate paths for ${env_var}: {dup_paths}")
mod_req_paths = retained_paths
self.log.info(f"Retained paths for ${env_var}: {mod_req_paths}")
else:
self.log.info(f"No duplicate paths found for ${env_var}: {mod_req_paths}")

# for $CMAKE_LIBRARY_PATH, only retain 'lib64' if it's standalone (*not* a symlink to 'lib')
if env_var == 'CMAKE_LIBRARY_PATH' and 'lib64' in mod_req_paths:
full_lib = os.path.join(self.installdir, 'lib')
full_lib64 = os.path.join(self.installdir, 'lib64')
if os.path.exists(full_lib64) and os.path.exists(full_lib):
if os.path.samefile(full_lib64, full_lib):
mod_req_paths.remove('lib64')

mod_lines.append(self.module_generator.prepend_paths(env_var, mod_req_paths))

if self.dry_run:
Expand Down Expand Up @@ -1732,15 +1753,6 @@ def expand_module_search_path(self, search_path, path_type=ModEnvVarType.PATH_WI
tentative_path = os.path.relpath(abs_path, start=self.installdir)
tentative_path = '' if tentative_path == '.' else tentative_path # use empty string instead of dot

# avoid duplicate entries between symlinked library dirs
tent_path_sep = tentative_path + os.path.sep
if self.install_lib_symlink == LibSymlink.LIB64_TO_LIB and tent_path_sep.startswith('lib64' + os.path.sep):
self.log.debug("Discarded search path to symlinked lib64 directory: %s", tentative_path)
continue
if self.install_lib_symlink == LibSymlink.LIB_TO_LIB64 and tent_path_sep.startswith('lib' + os.path.sep):
self.log.debug("Discarded search path to symlinked lib directory: %s", tentative_path)
continue

check_dir_files = path_type in (ModEnvVarType.PATH_WITH_FILES, ModEnvVarType.PATH_WITH_TOP_FILES)
if os.path.isdir(abs_path) and check_dir_files:
# only retain paths to directories that contain at least one file
Expand All @@ -1753,18 +1765,6 @@ def expand_module_search_path(self, search_path, path_type=ModEnvVarType.PATH_WI

return retained_search_paths

def check_install_lib_symlink(self):
"""Update the symlink state between library directories in installation prefix"""
lib_dir = os.path.join(self.installdir, 'lib')
lib64_dir = os.path.join(self.installdir, 'lib64')

self._install_lib_symlink = LibSymlink.NEITHER
if os.path.exists(lib_dir) and os.path.exists(lib64_dir):
if os.path.islink(lib_dir) and os.path.samefile(lib_dir, lib64_dir):
self._install_lib_symlink = LibSymlink.LIB_TO_LIB64
elif os.path.islink(lib64_dir) and os.path.samefile(lib_dir, lib64_dir):
self._install_lib_symlink = LibSymlink.LIB64_TO_LIB

def make_module_req_guess(self):
"""
A dictionary of common search path variables to be loaded by environment modules
Expand Down Expand Up @@ -3218,9 +3218,6 @@ def post_install_step(self):
# create *relative* 'lib' symlink to 'lib64';
symlink('lib64', lib_dir, use_abspath_source=False)

# refresh symlink state in install_lib_symlink class variable
self.check_install_lib_symlink()

self.run_post_install_commands()
self.apply_post_install_patches()
self.print_post_install_messages()
Expand Down
40 changes: 12 additions & 28 deletions test/framework/easyblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,6 @@ def test_make_module_req(self):
for path in ('bin', ('bin', 'testdir'), 'sbin', 'share', ('share', 'man'), 'lib', 'lib64'):
path_components = (path, ) if isinstance(path, str) else path
os.mkdir(os.path.join(eb.installdir, *path_components))
eb.check_install_lib_symlink()

write_file(os.path.join(eb.installdir, 'foo.jar'), 'foo.jar')
write_file(os.path.join(eb.installdir, 'bla.jar'), 'bla.jar')
Expand Down Expand Up @@ -505,7 +504,6 @@ 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'))
eb.check_install_lib_symlink()
with eb.module_generator.start_module_creation():
guess = eb.make_module_req()
if get_module_syntax() == 'Tcl':
Expand Down Expand Up @@ -3215,9 +3213,6 @@ def test_expand_module_search_path(self):
write_file(os.path.join(eb.installdir, 'dir_full_subdirs', 'subdir1', 'file12.txt'), 'test file 1.2')
write_file(os.path.join(eb.installdir, 'dir_full_subdirs', 'subdir2', 'file21.txt'), 'test file 2.1')

eb.check_install_lib_symlink()
self.assertEqual(eb.install_lib_symlink, LibSymlink.NEITHER)

self.assertEqual(test_emsp("nonexistent", ModEnvVarType.PATH), [])
self.assertEqual(test_emsp("nonexistent", ModEnvVarType.PATH_WITH_FILES), [])
self.assertEqual(test_emsp("nonexistent", ModEnvVarType.PATH_WITH_TOP_FILES), [])
Expand Down Expand Up @@ -3252,13 +3247,8 @@ def test_expand_module_search_path(self):
self.assertEqual(test_emsp("nonexistent/*", ModEnvVarType.PATH_WITH_FILES), [])
self.assertEqual(test_emsp("nonexistent/*", ModEnvVarType.PATH_WITH_TOP_FILES), [])

# state of install_lib_symlink should not have changed
self.assertEqual(eb.install_lib_symlink, LibSymlink.NEITHER)

# test just one lib directory
os.mkdir(os.path.join(eb.installdir, "lib"))
eb.check_install_lib_symlink()
self.assertEqual(eb.install_lib_symlink, LibSymlink.NEITHER)
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH), ["lib"])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH_WITH_FILES), [])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH_WITH_TOP_FILES), [])
Expand All @@ -3269,8 +3259,6 @@ def test_expand_module_search_path(self):

# test both lib and lib64 directories
os.mkdir(os.path.join(eb.installdir, "lib64"))
eb.check_install_lib_symlink()
self.assertEqual(eb.install_lib_symlink, LibSymlink.NEITHER)
self.assertEqual(sorted(test_emsp("lib*", ModEnvVarType.PATH)), ["lib", "lib64"])
self.assertEqual(test_emsp("lib*", ModEnvVarType.PATH_WITH_FILES), ["lib"])
self.assertEqual(test_emsp("lib*", ModEnvVarType.PATH_WITH_TOP_FILES), ["lib"])
Expand All @@ -3282,35 +3270,31 @@ def test_expand_module_search_path(self):
# test lib64 symlinked to lib
remove_dir(os.path.join(eb.installdir, "lib64"))
os.symlink("lib", os.path.join(eb.installdir, "lib64"))
eb.check_install_lib_symlink()
self.assertEqual(eb.install_lib_symlink, LibSymlink.LIB64_TO_LIB)
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH), ["lib"])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH_WITH_FILES), ["lib"])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH_WITH_TOP_FILES), ["lib"])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH), [])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH_WITH_FILES), [])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH_WITH_TOP_FILES), [])
self.assertEqual(test_emsp("lib*", ModEnvVarType.PATH), ["lib"])
self.assertEqual(test_emsp("lib*", ModEnvVarType.PATH_WITH_FILES), ["lib"])
self.assertEqual(test_emsp("lib*", ModEnvVarType.PATH_WITH_TOP_FILES), ["lib"])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH), ["lib64"])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH_WITH_FILES), ["lib64"])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH_WITH_TOP_FILES), ["lib64"])
self.assertEqual(sorted(test_emsp("lib*", ModEnvVarType.PATH)), ["lib", "lib64"])
self.assertEqual(sorted(test_emsp("lib*", ModEnvVarType.PATH_WITH_FILES)), ["lib", "lib64"])
self.assertEqual(sorted(test_emsp("lib*", ModEnvVarType.PATH_WITH_TOP_FILES)), ["lib", "lib64"])
Comment on lines +3316 to +3318
Copy link
Contributor

@lexming lexming Feb 10, 2025

Choose a reason for hiding this comment

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

this is wrong, if lib64 is symlinked to lib we do not want both paths in the environment

Copy link
Member Author

@boegel boegel Feb 10, 2025

Choose a reason for hiding this comment

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

There won't be, the logic to handle that is now in EasyBlock.make_module_req, not in expand_module_search_path.

We verify this in test_make_module_req


# test lib symlinked to lib64
remove_dir(os.path.join(eb.installdir, "lib"))
remove_file(os.path.join(eb.installdir, "lib64"))
os.mkdir(os.path.join(eb.installdir, "lib64"))
write_file(os.path.join(eb.installdir, "lib64", "libtest.so"), "not actually a lib")
os.symlink("lib64", os.path.join(eb.installdir, "lib"))
eb.check_install_lib_symlink()
self.assertEqual(eb.install_lib_symlink, LibSymlink.LIB_TO_LIB64)
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH), [])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH_WITH_FILES), [])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH_WITH_TOP_FILES), [])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH), ["lib"])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH_WITH_FILES), ["lib"])
self.assertEqual(test_emsp("lib", ModEnvVarType.PATH_WITH_TOP_FILES), ["lib"])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH), ["lib64"])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH_WITH_FILES), ["lib64"])
self.assertEqual(test_emsp("lib64", ModEnvVarType.PATH_WITH_TOP_FILES), ["lib64"])
self.assertEqual(test_emsp("lib*", ModEnvVarType.PATH), ["lib64"])
self.assertEqual(test_emsp("lib*", ModEnvVarType.PATH_WITH_FILES), ["lib64"])
self.assertEqual(test_emsp("lib*", ModEnvVarType.PATH_WITH_TOP_FILES), ["lib64"])
self.assertEqual(sorted(test_emsp("lib*", ModEnvVarType.PATH)), ["lib", "lib64"])
self.assertEqual(sorted(test_emsp("lib*", ModEnvVarType.PATH_WITH_FILES)), ["lib", "lib64"])
self.assertEqual(sorted(test_emsp("lib*", ModEnvVarType.PATH_WITH_TOP_FILES)), ["lib", "lib64"])


def suite():
Expand Down
Loading