From f799dce77bbe45785d824033b49863597e636854 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 | 37 +++-------- benchcab/utils/__init__.py | 17 ++++++ docs/user_guide/config_options.md | 2 +- tests/test_model.py | 61 +------------------ 5 files changed, 43 insertions(+), 91 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 4e8d221..17f5368 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, LocalRepo, Repo from benchcab.utils.subprocess import SubprocessWrapper, SubprocessWrapperInterface @@ -99,22 +96,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, mpi=False): """Runs CABLE pre-build steps.""" @@ -170,14 +158,3 @@ def post_build(self, mpi=False): tmp_dir / exe, path_to_repo / self.src_dir / "offline" / 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/docs/user_guide/config_options.md b/docs/user_guide/config_options.md index 5dc5880..f515fd3 100644 --- a/docs/user_guide/config_options.md +++ b/docs/user_guide/config_options.md @@ -388,7 +388,7 @@ realisations: ### [build_script](#build_script) -: **Default:** unset, _optional key_. :octicons-dash-24: The path to a custom shell script to build the code in that branch, relative to the repository root directory. **Note:** any lines in the provided shell script that call the [environment modules API][environment-modules] will be ignored. To specify modules to use for the build script, please specify them using the [`modules`](#modules) key. +: **Default:** unset, _optional key_. :octicons-dash-24: The path to a custom shell script to build the code in that branch, relative to the repository root directory. **Note:** invocations of the [`module`][environment-modules] command which modify the user environment (i.e. `add`, `load`, `rm`, `unload`, `purge`, ...) will be ignored. To specify modules to use for the build script, please specify them using the [`modules`](#modules) key. ```yaml realisations: diff --git a/tests/test_model.py b/tests/test_model.py index ece9b09..c5fb1a6 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -11,7 +11,7 @@ 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 @@ -293,14 +293,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 ): @@ -326,54 +318,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 -} -""" - )