From 4849fd7183c4ebd81939e886f639e1cdc2c3dfa7 Mon Sep 17 00:00:00 2001 From: Sean Bryan Date: Fri, 1 Mar 2024 10:30:13 +1100 Subject: [PATCH] Disable module load, unload, ... invocations via wrapper This change disables module commands that modify the environment via a wrapper when running a custom build script. This avoids preprocessing the custom build script for module statements which may produce an invalid bash script. This also allows for module commands that do not alter the environment to be executed as we only disable a subset of subcommands in the wrapper. Fixes #249 --- .../data/environment_modules_wrapper.bash | 17 +++++ benchcab/model.py | 40 +++--------- benchcab/utils/__init__.py | 17 +++++ tests/test_model.py | 62 +------------------ 4 files changed, 44 insertions(+), 92 deletions(-) create mode 100644 benchcab/data/environment_modules_wrapper.bash diff --git a/benchcab/data/environment_modules_wrapper.bash b/benchcab/data/environment_modules_wrapper.bash new file mode 100644 index 0000000..064c889 --- /dev/null +++ b/benchcab/data/environment_modules_wrapper.bash @@ -0,0 +1,17 @@ +#!/bin/bash + +# Wrapper around the module (environment modules) command which disables +# commands that modify the current environment. +module() { + args=("$@") + for arg in "${args[@]}"; do + case $arg in + add|load|rm|unload|swap|switch|use|unuse|purge) + echo "command disabled: module ""${args[*]}" 1>&2 + return 1 + ;; + esac + done + _module_raw "${args[@]}" 2>&1 + return $? +} diff --git a/benchcab/model.py b/benchcab/model.py index 1bbf014..691cf9b 100644 --- a/benchcab/model.py +++ b/benchcab/model.py @@ -4,15 +4,12 @@ """Contains functions and data structures relating to CABLE models.""" import os -import shlex -import shutil -import stat from pathlib import Path from typing import Optional from benchcab import internal from benchcab.environment_modules import EnvironmentModules, EnvironmentModulesInterface -from benchcab.utils import get_logger +from benchcab.utils import get_logger, get_package_data_path from benchcab.utils.fs import chdir, copy2, rename from benchcab.utils.repo import GitRepo, Repo from benchcab.utils.subprocess import SubprocessWrapper, SubprocessWrapperInterface @@ -95,22 +92,13 @@ def custom_build(self, modules: list[str]): ) raise FileNotFoundError(msg) - tmp_script_path = build_script_path.parent / "tmp-build.sh" - - self.logger.debug(f"Copying {build_script_path} to {tmp_script_path}") - shutil.copy(build_script_path, tmp_script_path) - - self.logger.debug(f"chmod +x {tmp_script_path}") - tmp_script_path.chmod(tmp_script_path.stat().st_mode | stat.S_IEXEC) - - self.logger.debug( - f"Modifying {tmp_script_path.name}: remove lines that call environment modules" - ) - - remove_module_lines(tmp_script_path) - with chdir(build_script_path.parent), self.modules_handler.load(modules): - self.subprocess_handler.run_cmd(f"./{tmp_script_path.name}") + modules_wrapper_path = get_package_data_path( + Path("environment_modules_wrapper.bash") + ) + self.subprocess_handler.run_cmd( + f"source {modules_wrapper_path}; ./{build_script_path.name}" + ) def pre_build(self): """Runs CABLE pre-build steps.""" @@ -143,7 +131,8 @@ def run_build(self, modules: list[str]): env["FC"] = "mpif90" if internal.MPI else "ifort" self.subprocess_handler.run_cmd( - "make mpi" if internal.MPI else "make", env=env + "make mpi" if internal.MPI else "make", + env=env, ) def post_build(self): @@ -155,14 +144,3 @@ def post_build(self): tmp_dir / internal.CABLE_EXE, path_to_repo / self.src_dir / "offline" / internal.CABLE_EXE, ) - - -def remove_module_lines(file_path: Path) -> None: - """Remove lines from `file_path` that call the environment modules package.""" - with file_path.open("r", encoding="utf-8") as file: - contents = file.read() - with file_path.open("w", encoding="utf-8") as file: - for line in contents.splitlines(True): - cmds = shlex.split(line, comments=True) - if "module" not in cmds: - file.write(line) diff --git a/benchcab/utils/__init__.py b/benchcab/utils/__init__.py index 5d67dc7..e44316c 100644 --- a/benchcab/utils/__init__.py +++ b/benchcab/utils/__init__.py @@ -32,6 +32,23 @@ def get_installed_root() -> Path: return Path(resources.files("benchcab")) +def get_package_data_path(resource: Path) -> Path: + """Return the absolute path to a given resource in the package data directory. + + Parameters + ---------- + resource: Path + Path to the resource relative to the package data directory. + + Returns + ------- + Path + Absolute path to the resource. + + """ + return Path(sys.modules["benchcab"].__file__).parent / "data" / resource + + def load_package_data(filename: str) -> Union[str, dict]: """Load data out of the installed package data directory. diff --git a/tests/test_model.py b/tests/test_model.py index 5ae06df..02a6284 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -6,12 +6,11 @@ """ import os -from pathlib import Path import pytest from benchcab import internal -from benchcab.model import Model, remove_module_lines +from benchcab.model import Model from benchcab.utils.repo import Repo @@ -249,14 +248,6 @@ def modules(self): """Return a list of modules for testing.""" return ["foo", "bar"] - def test_build_command_execution( - self, model, mock_subprocess_handler, build_script, modules - ): - """Success case: execute the build command for a custom build script.""" - model.build_script = str(build_script) - model.custom_build(modules) - assert "./tmp-build.sh" in mock_subprocess_handler.commands - def test_modules_loaded_at_runtime( self, model, mock_environment_modules_handler, build_script, modules ): @@ -282,54 +273,3 @@ def test_file_not_found_exception(self, model, build_script, modules): "option in config.yaml?", ): model.custom_build(modules) - - -class TestRemoveModuleLines: - """Tests for `remove_module_lines()`.""" - - def test_module_lines_removed_from_shell_script(self): - """Success case: test 'module' lines are removed from mock shell script.""" - file_path = Path("test-build.sh") - with file_path.open("w", encoding="utf-8") as file: - file.write( - """#!/bin/bash -module add bar -module purge - -host_gadi() -{ - . /etc/bashrc - module purge - module add intel-compiler/2019.5.281 - module add netcdf/4.6.3 - module load foo - modules - echo foo && module load - echo foo # module load - # module load foo - - if [[ $1 = 'mpi' ]]; then - module add intel-mpi/2019.5.281 - fi -} -""" - ) - - remove_module_lines(file_path) - - with file_path.open("r", encoding="utf-8") as file: - assert file.read() == ( - """#!/bin/bash - -host_gadi() -{ - . /etc/bashrc - modules - echo foo # module load - # module load foo - - if [[ $1 = 'mpi' ]]; then - fi -} -""" - )