From 0a7c6c510d3286552f1c92a51f1349167b78a0e5 Mon Sep 17 00:00:00 2001 From: Sean Bryan Date: Fri, 15 Sep 2023 10:15:08 +1000 Subject: [PATCH] Remove dependence on build script Currently benchcab uses the `offline/build3.sh` script to build the CABLE executable. The `build3.sh` is a wrapper around `Makefile`, `parallel_cable` and `serial_cable` scripts. Using `build3.sh` is convenient for CABLE developers, however it has its draw backs when building executables for benchcab, these are outlined in #138. This change removes the dependence on `build3.sh` from the build and instead invokes the `Makefile`, `parallel_cable` and `serial_cable` scripts directly. Fixes #138 --- benchcab/internal.py | 17 +++++++ benchcab/repository.py | 99 ++++++++++++++++++++++++++++++------ benchcab/utils/subprocess.py | 5 ++ tests/common.py | 6 +++ tests/test_repository.py | 90 +++++++++++++++++++++++--------- tests/test_subprocess.py | 7 +++ 6 files changed, 185 insertions(+), 39 deletions(-) diff --git a/benchcab/internal.py b/benchcab/internal.py index 7512fcd9..bb08b7e3 100644 --- a/benchcab/internal.py +++ b/benchcab/internal.py @@ -100,6 +100,23 @@ # CABLE standard output filename CABLE_STDOUT_FILENAME = "out.txt" +OFFLINE_SOURCE_FILES = [ + "science/albedo/*90", + "science/radiation/*90", + "science/canopy/*90", + "science/casa-cnp/*90", + "science/gw_hydro/*90", + "science/misc/*90", + "science/roughness/*90", + "science/soilsnow/*90", + "science/landuse/*90", + "offline/*90", + "util/*90", + "params/*90", + "science/sli/*90", + "science/pop/*90", +] + # Contains the default science configurations used to run the CABLE test suite # (when a science config file is not provided by the user) DEFAULT_SCIENCE_CONFIGURATIONS = [ diff --git a/benchcab/repository.py b/benchcab/repository.py index 987e43c5..c9ca3fbe 100644 --- a/benchcab/repository.py +++ b/benchcab/repository.py @@ -1,5 +1,6 @@ """A module containing functions and data structures for manipulating CABLE repositories.""" +import os import shlex import shutil import stat @@ -74,25 +75,97 @@ def svn_info_show_item(self, item: str) -> str: ) return proc.stdout.strip() - def build(self, modules: list[str], verbose=False) -> None: + def build( + self, + modules: list[str], + offline_source_files: Optional[list[str]] = None, + verbose=False, + ) -> None: """Build CABLE using the default build script or a custom build script.""" if self.build_script: + self._custom_build(modules=modules, verbose=verbose) + return + + mpi = internal.MPI + print( + f"Compiling CABLE {'with MPI' if mpi else 'serially'} for " + f"realisation {self.name}..." + ) + + path_to_repo = self.root_dir / internal.SRC_DIR / self.name + tmp_dir = path_to_repo / "offline" / ".tmp" + if not tmp_dir.exists(): + if verbose: + print(f"mkdir {tmp_dir}") + tmp_dir.mkdir() + + for pattern in ( + offline_source_files + if offline_source_files + else internal.OFFLINE_SOURCE_FILES + ): + for path in path_to_repo.glob(pattern): + if not path.is_file(): + continue + if verbose: + print( + f"cp -p {path.relative_to(self.root_dir)} " + f"{tmp_dir.relative_to(self.root_dir)}" + ) + shutil.copy2(path, tmp_dir) + + src, dest = path_to_repo / "offline" / "Makefile", tmp_dir + if verbose: print( - "Compiling CABLE using custom build script for " - f"realisation {self.name}..." + f"cp -p {src.relative_to(self.root_dir)} {dest.relative_to(self.root_dir)}" ) - else: + shutil.copy2(src, dest) + + src, dest = path_to_repo / "offline" / "parallel_cable", tmp_dir + if verbose: print( - f"Compiling CABLE {'with MPI' if internal.MPI else 'serially'} for " - f"realisation {self.name}..." + f"cp -p {src.relative_to(self.root_dir)} {dest.relative_to(self.root_dir)}" ) + shutil.copy2(src, dest) + + src, dest = path_to_repo / "offline" / "serial_cable", tmp_dir + if verbose: + print( + f"cp -p {src.relative_to(self.root_dir)} {dest.relative_to(self.root_dir)}" + ) + shutil.copy2(src, dest) + + with chdir(tmp_dir), self.modules_handler.load(modules, verbose=verbose): + env = os.environ.copy() + env["NCDIR"] = f"{env['NETCDF_ROOT']}/lib/Intel" + env["NCMOD"] = f"{env['NETCDF_ROOT']}/include/Intel" + env["CFLAGS"] = "-O2 -fp-model precise" + env["LDFLAGS"] = f"-L{env['NETCDF_ROOT']}/lib/Intel -O0" + env["LD"] = "-lnetcdf -lnetcdff" + env["FC"] = "mpif90" if mpi else "ifort" + + self.subprocess_handler.run_cmd( + "make -f Makefile", env=env, verbose=verbose + ) + self.subprocess_handler.run_cmd( + f"./{'parallel_cable' if mpi else 'serial_cable'} \"{env['FC']}\" " + f"\"{env['CFLAGS']}\" \"{env['LDFLAGS']}\" \"{env['LD']}\" \"{env['NCMOD']}\"", + env=env, + verbose=verbose, + ) + + exe = internal.CABLE_EXE + (tmp_dir / exe).rename(path_to_repo / "offline" / exe) + + def _custom_build(self, modules: list[str], verbose=False): + print( + "Compiling CABLE using custom build script for " + f"realisation {self.name}..." + ) build_script_path = ( - self.root_dir - / internal.SRC_DIR - / self.name - / (self.build_script if self.build_script else "offline/build3.sh") + self.root_dir / internal.SRC_DIR / self.name / self.build_script ) if not build_script_path.is_file(): @@ -119,15 +192,11 @@ def build(self, modules: list[str], verbose=False) -> None: ) remove_module_lines(tmp_script_path) - args: list[str] = [] - if internal.MPI and self.build_script is None: - args.append("mpi") - with chdir(build_script_path.parent), self.modules_handler.load( modules, verbose=verbose ): self.subprocess_handler.run_cmd( - shlex.join([f"./{tmp_script_path.name}", *args]), + f"./{tmp_script_path.name}", verbose=verbose, ) diff --git a/benchcab/utils/subprocess.py b/benchcab/utils/subprocess.py index ba8b5109..3418bacb 100644 --- a/benchcab/utils/subprocess.py +++ b/benchcab/utils/subprocess.py @@ -22,6 +22,7 @@ def run_cmd( capture_output: bool = False, output_file: Optional[pathlib.Path] = None, verbose: bool = False, + env: Optional[dict] = None, ) -> subprocess.CompletedProcess: """A wrapper around the `subprocess.run` function for executing system commands.""" @@ -35,6 +36,7 @@ def run_cmd( capture_output: bool = False, output_file: Optional[pathlib.Path] = None, verbose: bool = False, + env: Optional[dict] = None, ) -> subprocess.CompletedProcess: kwargs: Any = {} with contextlib.ExitStack() as stack: @@ -49,6 +51,9 @@ def run_cmd( kwargs["stdout"] = None if verbose else subprocess.DEVNULL kwargs["stderr"] = subprocess.STDOUT + if env: + kwargs["env"] = env + if verbose: print(cmd) proc = subprocess.run(cmd, shell=True, check=True, **kwargs) diff --git a/tests/common.py b/tests/common.py index 2cec52cf..ac127462 100644 --- a/tests/common.py +++ b/tests/common.py @@ -77,6 +77,8 @@ def __init__(self) -> None: self.commands: list[str] = [] self.stdout = "mock standard output" self.error_on_call = False + self.env = {} + self.side_effect = lambda: None def run_cmd( self, @@ -84,12 +86,16 @@ def run_cmd( capture_output: bool = False, output_file: Optional[Path] = None, verbose: bool = False, + env: Optional[dict] = None, ) -> CompletedProcess: self.commands.append(cmd) + self.side_effect() if self.error_on_call: raise CalledProcessError(returncode=1, cmd=cmd, output=self.stdout) if output_file: output_file.touch() + if env: + self.env = env return CompletedProcess(cmd, returncode=0, stdout=self.stdout) diff --git a/tests/test_repository.py b/tests/test_repository.py index 1c965a28..f84363c5 100644 --- a/tests/test_repository.py +++ b/tests/test_repository.py @@ -1,5 +1,6 @@ """`pytest` tests for repository.py""" +import os import io import contextlib import pytest @@ -111,24 +112,48 @@ def test_svn_info_show_item(): def test_build(): """Tests for `CableRepository.build()`.""" repo_dir = MOCK_CWD / internal.SRC_DIR / "trunk" - build_script_path = repo_dir / "offline" / "build3.sh" + offline_dir = repo_dir / "offline" custom_build_script_path = repo_dir / "my-custom-build.sh" mock_modules = ["foo", "bar"] - - # Success case: execute the default build command - build_script_path.parent.mkdir(parents=True, exist_ok=True) - build_script_path.touch(exist_ok=True) + mock_netcdf_root = "/mock/path/to/root" + default_env = { + "NCDIR": f"{mock_netcdf_root}/lib/Intel", + "NCMOD": f"{mock_netcdf_root}/include/Intel", + "CFLAGS": "-O2 -fp-model precise", + "LDFLAGS": f"-L{mock_netcdf_root}/lib/Intel -O0", + "LD": "-lnetcdf -lnetcdff", + "FC": "ifort", + } + + # Success case: test default build + offline_dir.mkdir(parents=True, exist_ok=True) + (offline_dir / "Makefile").touch(exist_ok=True) + (offline_dir / "parallel_cable").touch(exist_ok=True) + (offline_dir / "serial_cable").touch(exist_ok=True) + (offline_dir / "foo.f90").touch(exist_ok=True) + os.environ["NETCDF_ROOT"] = mock_netcdf_root mock_subprocess = MockSubprocessWrapper() + mock_subprocess.side_effect = lambda: ( + offline_dir / ".tmp" / internal.CABLE_EXE + ).touch(exist_ok=True) mock_environment_modules = MockEnvironmentModules() repo = get_mock_repo(mock_subprocess, mock_environment_modules) - repo.build(mock_modules) - assert "./tmp-build.sh" in mock_subprocess.commands + repo.build(mock_modules, offline_source_files=["offline/*90"]) + assert (offline_dir / ".tmp" / "foo.f90").exists() + assert "make -f Makefile" in mock_subprocess.commands + assert ( + f"./serial_cable \"{default_env['FC']}\" \"{default_env['CFLAGS']}\"" + f" \"{default_env['LDFLAGS']}\" \"{default_env['LD']}\" \"{default_env['NCMOD']}\"" + in mock_subprocess.commands + ) + assert all(kv in mock_subprocess.env.items() for kv in default_env.items()) assert ( "module load " + " ".join(mock_modules) ) in mock_environment_modules.commands assert ( "module unload " + " ".join(mock_modules) ) in mock_environment_modules.commands + assert (offline_dir / internal.CABLE_EXE).exists() # Success case: execute the build command for a custom build script custom_build_script_path.parent.mkdir(parents=True, exist_ok=True) @@ -147,11 +172,19 @@ def test_build(): ) in mock_environment_modules.commands # Success case: test non-verbose standard output - build_script_path.parent.mkdir(parents=True, exist_ok=True) - build_script_path.touch(exist_ok=True) - repo = get_mock_repo() + offline_dir.mkdir(parents=True, exist_ok=True) + (offline_dir / "Makefile").touch(exist_ok=True) + (offline_dir / "parallel_cable").touch(exist_ok=True) + (offline_dir / "serial_cable").touch(exist_ok=True) + (offline_dir / "foo.f90").touch(exist_ok=True) + os.environ["NETCDF_ROOT"] = mock_netcdf_root + mock_subprocess = MockSubprocessWrapper() + mock_subprocess.side_effect = lambda: ( + offline_dir / ".tmp" / internal.CABLE_EXE + ).touch(exist_ok=True) + repo = get_mock_repo(subprocess_handler=mock_subprocess) with contextlib.redirect_stdout(io.StringIO()) as buf: - repo.build(mock_modules) + repo.build(mock_modules, offline_source_files=["offline/*90"]) assert buf.getvalue() == "Compiling CABLE serially for realisation trunk...\n" # Success case: test non-verbose standard output for a custom build script @@ -166,17 +199,25 @@ def test_build(): ) # Success case: test verbose standard output - build_script_path.parent.mkdir(parents=True, exist_ok=True) - build_script_path.touch(exist_ok=True) - repo = get_mock_repo() + offline_dir.mkdir(parents=True, exist_ok=True) + (offline_dir / "Makefile").touch(exist_ok=True) + (offline_dir / "parallel_cable").touch(exist_ok=True) + (offline_dir / "serial_cable").touch(exist_ok=True) + (offline_dir / "foo.f90").touch(exist_ok=True) + os.environ["NETCDF_ROOT"] = mock_netcdf_root + mock_subprocess = MockSubprocessWrapper() + mock_subprocess.side_effect = lambda: ( + offline_dir / ".tmp" / internal.CABLE_EXE + ).touch(exist_ok=True) + repo = get_mock_repo(subprocess_handler=mock_subprocess) with contextlib.redirect_stdout(io.StringIO()) as buf: - repo.build(mock_modules, verbose=True) + repo.build(mock_modules, offline_source_files=["offline/*90"], verbose=True) assert buf.getvalue() == ( "Compiling CABLE serially for realisation trunk...\n" - f"Copying {build_script_path} to {build_script_path.parent}/tmp-build.sh\n" - f"chmod +x {build_script_path.parent}/tmp-build.sh\n" - "Modifying tmp-build.sh: remove lines that call environment " - "modules\n" + "cp -p src/trunk/offline/foo.f90 src/trunk/offline/.tmp\n" + "cp -p src/trunk/offline/Makefile src/trunk/offline/.tmp\n" + "cp -p src/trunk/offline/parallel_cable src/trunk/offline/.tmp\n" + "cp -p src/trunk/offline/serial_cable src/trunk/offline/.tmp\n" f"Loading modules: {' '.join(mock_modules)}\n" f"Unloading modules: {' '.join(mock_modules)}\n" ) @@ -198,14 +239,15 @@ def test_build(): f"Unloading modules: {' '.join(mock_modules)}\n" ) - # Failure case: cannot find default build script - build_script_path.parent.mkdir(parents=True, exist_ok=True) - build_script_path.touch(exist_ok=True) - build_script_path.unlink() + # Failure case: cannot find custom build script + custom_build_script_path.parent.mkdir(parents=True, exist_ok=True) + custom_build_script_path.touch(exist_ok=True) + custom_build_script_path.unlink() repo = get_mock_repo() + repo.build_script = str(custom_build_script_path.relative_to(repo_dir)) with pytest.raises( FileNotFoundError, - match=f"The build script, {MOCK_CWD}/src/trunk/offline/build3.sh, could not be " + match=f"The build script, {custom_build_script_path}, could not be " "found. Do you need to specify a different build script with the 'build_script' " "option in config.yaml?", ): diff --git a/tests/test_subprocess.py b/tests/test_subprocess.py index 860ecbaa..bbedc66b 100644 --- a/tests/test_subprocess.py +++ b/tests/test_subprocess.py @@ -1,5 +1,6 @@ """`pytest` tests for utils/subprocess.py""" +import os import subprocess import pytest @@ -79,6 +80,12 @@ def test_run_cmd(capfd): assert captured.out == "echo foo\n" assert not captured.err + # Success case: test command is run with environment + proc = subprocess_handler.run_cmd( + "echo $FOO", capture_output=True, env={"FOO": "bar", **os.environ} + ) + assert proc.stdout == "bar\n" + # Failure case: check non-zero return code throws an exception with pytest.raises(subprocess.CalledProcessError): subprocess_handler.run_cmd("exit 1")