-
Notifications
You must be signed in to change notification settings - Fork 203
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
base: 5.0.x
Are you sure you want to change the base?
Conversation
@boegel look all the beautiful green tests! Compared to what I showed on the meeting yesterday, I have undone all changes to This seems ready on my side. I tested it with several easyblocks that add custom stuff to |
…d_module_search_path
…nment variables for module files
…r modules on load
…ed library directories
…ding paths for environment of modules
…er explicit checks in the module step
…vironmentVariable
easybuild/tools/modules.py
Outdated
|
||
|
||
# singleton metaclass: only one instance is created | ||
BaseModuleEnvironment = create_base_metaclass('BaseModuleEnvironment', Singleton, object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lexming Using a singleton here (so a single instance that is shared across all EasyBlock
instances) seems like a bad idea to me...
If EasyBuild installs two things in the same session (via --robot
, or by being fed multiple easyconfigs on the command line), you'll end up with incorrect stuff in the generated module file due to this, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…file of subsequent installations
add test to verify that environment variables don't leak into module file of subsequent installations
@lexming Can you look into fixing the merge conflict, and get the test to pass? |
… module load environment
…_install_lib_symlink
…path to consider all paths and not give up on first symlink
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easybuild/framework/easyblock.py
Outdated
A dictionary of common search path variables to be loaded by environment modules | ||
Each key contains the list of known directories related to the search path | ||
""" | ||
return self.module_load_environment.environ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we print a deprecation warning here too, nothing should be relying on make_module_req_guess
anymore, but use module_load_environment.environ
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that we need it here to catch the case where make_module_req_guess()
is directly called in some arbitrary location in a custom easyblock.
However, this has the downside of duplicating deprecation warnings in the following common case:
- EB reaches
make_module_req
- custom easyblock has its own
make_module_req_guess()
- first warning triggers
make_module_req
calls custommake_module_req_guess()
- custom
make_module_req_guess()
callssuper()
- generic
make_module_req_guess()
triggers second deprecation warning
But I don't see a way to avoid it, as custom make_module_req_guess()
methods do not necessarily call super
. So we need both to cover all cases.
def environ(self): | ||
""" | ||
Return dict with mapping of ModuleEnvironmentVariables names with their contents | ||
Equivalent in shape to os.environ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit misleading, since the values are actually lists of strings, not string values as they are in os.environ
, so I would remove this:
Equivalent in shape to os.environ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easybuild/tools/modules.py
Outdated
Actual contents of the environment variable are held in self.contents | ||
""" | ||
self.contents = contents | ||
self.top_level_file = bool(top_level_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename this to something more explicit, like requires_files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be misleading. The current behaviour (v4.9) is that all paths require files in them to be declared in the module file. This attribute only changes the strictness of that check to require those files to be in the top directory.
Here there is an underlying design question, because ModuleEnvironmentVariable
can be generic in purpose and used for other things than make_module_req
. So maybe we need a ternary parameter (NO_FILES, ANY_FILES, TOP_LEVEL_FILES).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a nice solution for this, I generalized a bit more ModuleEnvironmentVariable
by adding a type attribute to it which can be one of ModEnvVarType
. A new enum with STRING
, PATH
, PATH_WITH_FILES
and PATH_WITH_TOP_FILES
. Then with a single attribute we can control the behaviour of a specific variable. See 516d6e6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved further the use and tests of ModEnvVarType
in d4a9df0
This is ready on my side.
easybuild/framework/easyblock.py
Outdated
if paths: | ||
lines.append(self.module_generator.prepend_paths(key, paths)) | ||
mod_req_paths = [] | ||
top_level = getattr(self.module_load_environment, env_var).top_level_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes horribly wrong when using a copy of PerlModule
that has not been migrated yet to module_load_environment
(as is done in easybuilders/easybuild-easyblocks#3529):
Traceback (most recent call last):
File "/usr/lib64/python3.6/runpy.py", line 193, in _run_module_as_main
"__main__", mod_spec)
File "/usr/lib64/python3.6/runpy.py", line 85, in _run_code
exec(code, run_globals)
File "/tmp/vsc40023/easybuild-framework/easybuild/main.py", line 801, in <module>
main_with_hooks()
File "/tmp/vsc40023/easybuild-framework/easybuild/main.py", line 787, in main_with_hooks
main(args=args, prepared_cfg_data=(init_session_state, eb_go, cfg_settings))
File "/tmp/vsc40023/easybuild-framework/easybuild/main.py", line 743, in main
hooks, do_build)
File "/tmp/vsc40023/easybuild-framework/easybuild/main.py", line 567, in process_eb_args
exit_on_failure=exit_on_failure)
File "/tmp/vsc40023/easybuild-framework/easybuild/main.py", line 175, in build_and_install_software
raise ec_res['err']
File "/tmp/vsc40023/easybuild-framework/easybuild/main.py", line 136, in build_and_install_software
(ec_res['success'], app_log, err_msg, err_code) = build_and_install_one(ec, init_env)
File "/tmp/vsc40023/easybuild-framework/easybuild/framework/easyblock.py", line 4432, in build_and_install_one
result = app.run_all_steps(run_test_cases=run_test_cases)
File "/tmp/vsc40023/easybuild-framework/easybuild/framework/easyblock.py", line 4300, in run_all_steps
self.run_step(step_name, step_methods)
File "/tmp/vsc40023/easybuild-framework/easybuild/framework/easyblock.py", line 4143, in run_step
step_method(self)()
File "/user/gent/400/vsc40023/easybuild/eb5/easybuild-easyblocks/easybuild/easyblocks/generic/perlmodule.py", line 140, in sanity_check_step
return ExtensionEasyBlock.sanity_check_step(self, EXTS_FILTER_PERL_MODULES, *args, **kwargs)
File "/tmp/vsc40023/easybuild-framework/easybuild/framework/extensioneasyblock.py", line 188, in sanity_check_step
fake_mod_data = self.load_fake_module(purge=True, extra_modules=extra_modules)
File "/tmp/vsc40023/easybuild-framework/easybuild/framework/easyblock.py", line 1796, in load_fake_module
fake_mod_path = self.make_module_step(fake=True)
File "/tmp/vsc40023/easybuild-framework/easybuild/framework/easyblock.py", line 3905, in make_module_step
txt += self.make_module_req(fake=fake)
File "/tmp/vsc40023/easybuild-framework/easybuild/framework/easyblock.py", line 1664, in make_module_req
requires_files = getattr(self.module_load_environment, env_var).requires_files
AttributeError: 'ModuleLoadEnvironment' object has no attribute 'PERL5LIB'
(note that I'm using my updated version here, see also lexming#6)
It's getting PERL5LIB
via PerlModule.make_module_req_guess
, but since that has never been defined as an attribute in module_load_environment
, this fails hard.
That tells me we're missing something above, where we deal with easyblocks that still define a custom make_module_req_guess
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor renaming + style fixes for refactoring generation of required environment variables in module files
…module_req_guess is detected
… property based on new ModEnvVarType enum
…e to control requirements of paths to directories
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:
(fixes directories that don't contain any library files shouldn't be added to
$LD_LIBRARY_PATH
#3504)top_level_file
attribute ofModuleEnvironmentVariable
lib64
that generate duplicate paths and need explicit exceptions with:lib64
are always ignored iflib64
is a symlink tolib
lib
are always ignored iflib
is a symlink tolib64
lib32
from list of library search pathsChangelog:
ModuleEnvironmentVariable
pseudo-dataclass toeasybuild.tools.modules
to hold definitions of environment variables for modulesModuleLoadEnvironment
singleton class toeasybuild.tools.modules
to hold environment definition for modules on loadEasyBlock.make_module_req_guess
in favor ofModuleLoadEnvironment
easybuild.framework.easyblock
to define possible linkage states of lib directoriesinstall_lib_symlink
attribute to EasyBlcok to keep track of symlink status between lib dirseasybuild.tools.config
that define known directories for binaries, libraries and headerseasyblock.make_module_req
by working with absolute paths instead