Skip to content
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.

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

refactor generation of required environment variables in module files + deprecate make_module_req_guess method in EasyBlock class #4653

Merged
merged 66 commits into from
Jan 20, 2025

Conversation

lexming
Copy link
Contributor

@lexming lexming commented Sep 23, 2024

Goal is to lay groundwork to be able to fix #3331 and add new option to control which search path variables are added in modules by simplifying the code injecting environment variables to module files.

This PR should not alter the resulting module files to what is currently generated by EB. If it changes the list of environment variables it should have no effect (e.g. because those paths are empty).

Simplification of logic handling environment variables for module files:

  1. replace obscure rules defining which search paths require populated dirs with:
  2. replace obscure rules defining which search paths require files in their top level directory with:
    • ModuleEnvironmentVariable need to be explicitly defined with ModEnvVarType.PATH_WITH_TOP_FILES to require populated top dirs
    • this is only applied by default to PATH and LD_LIBRARY_PATH
  3. replace convoluted rules to handle symlinked lib64 that generate duplicate paths and need explicit exceptions with:
    • paths to lib64 are always ignored if lib64 is a symlink to lib
    • paths to lib are always ignored if lib is a symlink to lib64
    • remove lib32 from list of library search paths
  4. allow simple updates of environment variables in child EasyBlocks, replacing the clunky:
    def make_module_req_guess(self):
        guesses = super(EB_Clang, self).make_module_req_guess()
        guesses.update({'SOME_VAR': ['some/path]})
        return guesses
    
    with an elegant:
    self.module_load_environment.SOME_VAR = ['some/path']
    

Changelog:

  • add ModuleEnvironmentVariable pseudo-dataclass to easybuild.tools.modules to hold definitions of environment variables for modules
  • add ModuleLoadEnvironment class to easybuild.tools.modules to hold environment definition for modules on load
  • deprecate EasyBlock.make_module_req_guess in favor of ModuleLoadEnvironment
  • add LibSymlink enum to easybuild.framework.easyblock to define possible linkage states of lib directories
  • add install_lib_symlink attribute to EasyBlcok to keep track of symlink status between lib dirs
  • add globals to easybuild.tools.config that define known directories for binaries, libraries and headers
  • avoid changing the current working directory of the main process in easyblock.make_module_req by working with absolute paths instead

@lexming lexming added the EasyBuild-5.0 EasyBuild 5.0 label Sep 23, 2024
@lexming lexming marked this pull request as draft September 23, 2024 14:46
@lexming lexming marked this pull request as ready for review September 24, 2024 06:15
@lexming
Copy link
Contributor Author

lexming commented Sep 24, 2024

@boegel look all the beautiful green tests!

Compared to what I showed on the meeting yesterday, I have undone all changes to make_module_req_guess. So existing easyblocks continue to work without any further changes.

This seems ready on my side. I tested it with several easyblocks that add custom stuff to make_module_req_guess and I have hit no issues so far. Even things like root.py easyblock that set PYTHONPATH in make_module_req_guess instead of the more common make_module_extra do work. If you have anything in mind that I should test in particular let me know.

easybuild/tools/modules.py Outdated Show resolved Hide resolved
add test to verify that environment variables don't leak into module file of subsequent installations
@boegel
Copy link
Member

boegel commented Nov 6, 2024

@lexming Can you look into fixing the merge conflict, and get the test to pass?

@boegel
Copy link
Member

boegel commented Jan 13, 2025

@lexming What was the trigger for the change you made in 13f735c?

That broke the test_module_environment_variable test, and for good reason I'd say...

Can you enhance the tests to cover the case that made you make the change in 13f735c?

minor changes w.r.t. ModuleLoadEnvironment and ModEnvVarType
@lexming
Copy link
Contributor Author

lexming commented Jan 13, 2025

@boegel actually the tests is what saved us, they properly failed. That change in 13f735c was not needed, args are packed in a list and they need unpacking.

@boegel boegel merged commit a3ee7d4 into easybuilders:5.0.x Jan 20, 2025
39 checks passed
@lexming lexming deleted the cpath-mod branch January 20, 2025 15:57
@lexming
Copy link
Contributor Author

lexming commented Jan 20, 2025

updated description to match state of code at merge time

@boegel boegel added the change label Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants