make delattr aware of environment variables in ModuleLoadEnvironment #4784
Conversation
easybuild/tools/modules.py
Outdated
|
|
||
| name = self._unmangle_env_var_name(name) | ||
| del self.__dict__['_env_vars'][name] | ||
| try: | ||
| self.__dict__['_env_vars'][name].log.warning( |
There was a problem hiding this comment.
huh, is this really correct?
There was a problem hiding this comment.
also, maybe we use deprecated here instead, so we could easier find this in the future and remove it for EB 6?
There was a problem hiding this comment.
depends on your definition of correct 🙂 I'm just using the log handle on the attribute being deleted because ModuleLoadEnvironment does not have any. I could add one as well.
There was a problem hiding this comment.
The resulting log looks the same anyway:
== 2025-03-03 19:32:36,517 modules.py:345 WARNING Please use ModuleLoadEnvironment.remove() instead of 'delattr' to remove environment variable: ACLOCAL_PATH
== 2025-03-03 19:32:36,517 modules.py:345 WARNING Please use ModuleLoadEnvironment.remove() instead of 'delattr' to remove environment variable: CLASSPATH
== 2025-03-03 19:32:36,517 modules.py:345 WARNING Please use ModuleLoadEnvironment.remove() instead of 'delattr' to remove environment variable: CMAKE_LIBRARY_PATH
== 2025-03-03 19:32:36,517 modules.py:345 WARNING Please use ModuleLoadEnvironment.remove() instead of 'delattr' to remove environment variable: CMAKE_PREFIX_PATH
== 2025-03-03 19:32:36,517 modules.py:345 WARNING Please use ModuleLoadEnvironment.remove() instead of 'delattr' to remove environment variable: GI_TYPELIB_PATH
== 2025-03-03 19:32:36,517 modules.py:345 WARNING Please use ModuleLoadEnvironment.remove() instead of 'delattr' to remove environment variable: LD_LIBRARY_PATH
== 2025-03-03 19:32:36,517 modules.py:345 WARNING Please use ModuleLoadEnvironment.remove() instead of 'delattr' to remove environment variable: LIBRARY_PATH
== 2025-03-03 19:32:36,517 modules.py:345 WARNING Please use ModuleLoadEnvironment.remove() instead of 'delattr' to remove environment variable: MANPATH
There was a problem hiding this comment.
made it less ugly in cc9d03f
There was a problem hiding this comment.
There is this method specialization you added that does the mapping to delete environment variables. I assumed this PR was added in case people are using delattr in their custom easyblocks, and that shouldn't break with 5.0.x, but don't we want to remove this function down the line once everyone has properly switches to .remove?
There was a problem hiding this comment.
that is the issue with delattr, you cannot remove it. So what is added in this PR has to stay, which is ok. We just warn that .remove() is the better option, but it is not illegal to use delattr.
There was a problem hiding this comment.
on second thought, there is no point in this warning, using delattr is perfectly fine
There was a problem hiding this comment.
updated the description of this PR and the docstring of delattr in ModuleLoadEnvironment
There was a problem hiding this comment.
@Micket ping, can we get this merged please? all tests are green now
easybuild/tools/modules.py
Outdated
| return True | ||
|
|
||
| name = self._unmangle_env_var_name(name) | ||
| self._log.warning( |
There was a problem hiding this comment.
Shouldn't that rather be a log.deprecation instead of "just" a warning?
There was a problem hiding this comment.
We cannot deprecated a builtin like delattr, that only makes sense for things that we could remove from the code. Anyhow, it is not wrong to use it in ModuleLoadEnvironment, so I'm removing that warning altogether to avoid confusion.
There are two ways to remove attributes from
ModuleLoadEnvironment:ModuleLoadEnvironment.remove("ENV_VAR")will ignore missing attributesdelattr(ModuleLoadEnvironment, "ENV_VAR")will error on missing attributes