From 5a2dbc750778e8628451156de5296744c17a95b4 Mon Sep 17 00:00:00 2001 From: Shahzeb Siddiqui Date: Wed, 23 Oct 2024 12:24:32 -0400 Subject: [PATCH] refactor codebase for BuilderBase by creating modular methods to handle logic where code can be reused and more readable. --- buildtest/builders/base.py | 249 +++++++++++++++++++------------- buildtest/builders/script.py | 53 ++++--- buildtest/buildsystem/checks.py | 19 +-- 3 files changed, 185 insertions(+), 136 deletions(-) diff --git a/buildtest/builders/base.py b/buildtest/builders/base.py index d9442ddc4..b8c4b397e 100644 --- a/buildtest/builders/base.py +++ b/buildtest/builders/base.py @@ -16,6 +16,7 @@ import uuid from abc import ABC, abstractmethod from pathlib import Path +from typing import List from buildtest.buildsystem.checks import ( assert_range_check, @@ -323,6 +324,10 @@ def build(self, modules=None, modulepurge=None, unload_modules=None): self._build_setup() self._write_test() self._write_build_script(modules, modulepurge, unload_modules) + self._display_test_content( + filepath=self.build_script, title="Start of Build Script" + ) + self._display_test_content(filepath=self.testpath, title="Start of Test Script") self._write_post_run_script() def run(self, cmd, timeout=None): @@ -380,22 +385,26 @@ def execute_post_run_script(self): f"[blue]{self}[/]: Post run script exit code: {post_run.returncode()}" ) - if "output" in self.display: - print_content( - output, - title=f"[blue]{self}[/]: Start of Post Run Output", - theme="monokai", - lexer="text", - show_last_lines=10, - ) + self._display_output_content(output, title="Start of Post Run Output") + self._display_output_content(error, title="Start of Post Run Error") - print_content( - error, - title=f"[blue]{self}[/]: Start of Post Run Error", - theme="monokai", - lexer="text", - show_last_lines=10, - ) + def _display_output_content(self, output, title, show_last_lines=10): + """This method will display content of output or error results. + + Args: + output (str): Output content to display + title (str): Title to display before content + show_last_lines (int, optional): Number of lines to display from end of file. Default is 10 + """ + + if "output" in self.display: + print_content( + output, + title=f"[blue]{self}[/]: {title}", + theme="monokai", + lexer="text", + show_last_lines=show_last_lines, + ) def handle_run_result(self, command_result, timeout): """This method will handle the result of running test. If the test is successful we will record endtime, @@ -408,28 +417,16 @@ def handle_run_result(self, command_result, timeout): output_msg = "".join(command_result.get_output()) err_msg = "".join(command_result.get_error()) - if "output" in self.display: - print_content( - output_msg, - title=f"[blue]{self}[/]: Start of Output", - theme="monokai", - lexer="text", - show_last_lines=10, - ) + self._display_output_content(output_msg, title="Start of Output") if not self._retry or ret == 0: return command_result console.print(f"[red]{self}: failed to submit job with returncode: {ret}") - if "output" in self.display: - print_content( - err_msg, - title=f"[blue]{self}[/]: Start of Error", - theme="monokai", - lexer="text", - show_last_lines=30, - ) + self._display_output_content( + output=err_msg, title="Start of Error", show_last_lines=30 + ) console.print( f"[red]{self}: Detected failure in running test, will attempt to retry test: {self._retry} times" @@ -520,69 +517,34 @@ def is_running(self): """Return True if builder fails to run test.""" return self._state == "RUNNING" - def copy_stage_files(self): - """Copy output and error file into test root directory.""" - - shutil.copy2( - os.path.join(self.stage_dir, os.path.basename(self.metadata["outfile"])), - os.path.join(self.test_root, os.path.basename(self.metadata["outfile"])), - ) - shutil.copy2( - os.path.join(self.stage_dir, os.path.basename(self.metadata["errfile"])), - os.path.join(self.test_root, os.path.basename(self.metadata["errfile"])), - ) - - # update outfile and errfile metadata records which show up in report file - self.metadata["outfile"] = os.path.join( - self.test_root, os.path.basename(self.metadata["outfile"]) - ) - self.metadata["errfile"] = os.path.join( - self.test_root, os.path.basename(self.metadata["errfile"]) - ) - - def _build_setup(self): - """This method is the setup operation to get ready to build test which - includes the following: - - 1. Creating Test directory and stage directory - 2. Resolve full path to generated test script and build script - 3. Copy all files from buildspec directory to stage directory - """ + def _build_setup(self) -> None: + """Setup operation to get ready to build test.""" + self._create_directories() + self._resolve_paths() + self._copy_files_to_stage() + def _create_directories(self) -> None: + """Create necessary directories for the build.""" create_dir(self.testdir) - self.test_root = os.path.join(self.testdir, self.testid[:8]) - create_dir(self.test_root) - - msg = f"Creating test directory: {self.test_root}" - self.logger.debug(msg) - - self.metadata["testroot"] = self.test_root - self.stage_dir = os.path.join(self.test_root, "stage") - - # create stage and run directories create_dir(self.stage_dir) - msg = f"Creating the stage directory: {self.stage_dir}" - self.logger.debug(msg) - + self.logger.debug(f"Creating the stage directory: {self.stage_dir}") self.metadata["stagedir"] = self.stage_dir + self.metadata["testroot"] = self.test_root - # Derive the path to the test script + def _resolve_paths(self) -> None: + """Resolve full paths to generated test script and build script.""" self.testpath = ( os.path.join(self.stage_dir, self.name) + "." + self.get_test_extension() ) - self.testpath = os.path.expandvars(self.testpath) - - self.metadata["testpath"] = os.path.join( - self.test_root, os.path.basename(self.testpath) - ) - + self.metadata["testpath"] = self.testpath self.build_script = f"{os.path.join(self.stage_dir, self.name)}_build.sh" - # copy all files relative to buildspec file into stage directory + def _copy_files_to_stage(self) -> None: + """Copy all files from buildspec directory to stage directory.""" for fname in Path(os.path.dirname(self.buildspec)).glob("*"): if fname.is_dir(): shutil.copytree( @@ -590,10 +552,91 @@ def _build_setup(self): ) elif fname.is_file(): shutil.copy2(fname, self.stage_dir) - console.print(f"[blue]{self}[/]: Creating Test Directory: {self.test_root}") - def _write_build_script(self, modules=None, modulepurge=None, unload_modules=None): + def _write_build_script( + self, + modules: List[str] = None, + modulepurge: bool = None, + unload_modules: List[str] = None, + ) -> None: + """Write the content of build script.""" + lines = self._generate_build_script_lines(modules, modulepurge, unload_modules) + write_file(self.build_script, "\n".join(lines)) + self._set_execute_perm(self.build_script) + self._copy_build_script_to_test_root() + + def _display_test_content(self, filepath, title) -> None: + if "test" in self.display: + print_file_content( + file_path=filepath, + title=f"[blue]{self}[/]: {title}", + lexer="bash", + theme="monokai", + ) + + def _generate_build_script_lines( + self, modules: List[str], modulepurge: bool, unload_modules: List[str] + ) -> List[str]: + """Generate lines for the build script.""" + lines = ["#!/bin/bash"] + lines.append(self._generate_trap_message()) + lines += self._set_default_test_variables() + if modulepurge: + lines.append("module purge") + if unload_modules: + lines.append("# Specify list of modules to unload") + lines += [f"module unload {module}" for module in unload_modules] + if modules: + lines.append("# Specify list of modules to load") + lines += [f"module load {module}" for module in modules] + lines.append( + f"source {os.path.join(BUILDTEST_EXECUTOR_DIR, self.executor, 'before_script.sh')}" + ) + lines.append("# Run generated script") + lines += self._get_execution_command() + lines.append("# Get return code") + lines.append("returncode=$?") + lines.append("# Exit with return code") + lines.append("exit $returncode") + return lines + + def _generate_trap_message(self) -> str: + """Generate trap message for the build script.""" + return """ +# Function to handle all signals and perform cleanup +function cleanup() { + echo "Signal trapped. Performing cleanup before exiting." + exitcode=$? + echo "buildtest: command '$BASH_COMMAND' failed (exit code: $exitcode)" + exit $exitcode +} + +# Trap all signals and call the cleanup function +trap cleanup SIGINT SIGTERM SIGHUP SIGQUIT SIGABRT SIGKILL SIGALRM SIGPIPE SIGTERM SIGTSTP SIGTTIN SIGTTOU +""" + + def _get_execution_command(self) -> List[str]: + """Get the command to execute the script.""" + if self.is_local_executor(): + return [" ".join(self._emit_command())] + elif self.is_container_executor(): + return self.get_container_invocation() + else: + launcher = self.buildexecutor.executors[self.executor].launcher_command( + self.testpath + ) + return [" ".join(launcher) + " " + f"{self.testpath}"] + + def _copy_build_script_to_test_root(self) -> None: + """Copy build script to test root directory.""" + dest = os.path.join(self.test_root, os.path.basename(self.build_script)) + shutil.copy2(self.build_script, dest) + self.logger.debug(f"Copying build script to: {dest}") + self.build_script = dest + self.metadata["build_script"] = self.build_script + + def _write_build_script1(self, modules=None, modulepurge=None, unload_modules=None): """This method will write the content of build script that is run for when invoking the builder run method. Upon creating file we set permission of builder script to 755 so test can be run. @@ -697,13 +740,9 @@ def _write_post_run_script(self): console.print( f"[blue]{self}[/]: Writing Post Run Script: {self.post_run_script}" ) - if "test" in self.display: - print_file_content( - file_path=self.post_run_script, - title=f"[blue]{self}[/]: Start of Post Run Script", - lexer="bash", - theme="monokai", - ) + self._display_test_content( + filepath=self.post_run_script, title="Start of Post Run Script" + ) def _write_test(self): """This method is responsible for invoking ``generate_script`` that @@ -730,13 +769,6 @@ def _write_test(self): shutil.copy2( self.testpath, os.path.join(self.test_root, os.path.basename(self.testpath)) ) - if "test" in self.display: - print_file_content( - file_path=self.testpath, - title=f"[blue]{self}[/]: Start of Test Script", - lexer="bash", - theme="monokai", - ) def get_container_invocation(self): """This method returns a list of lines containing the container invocation""" @@ -869,6 +901,12 @@ def get_job_directives(self): lines.append(f"#PBS -o {self.name}.o") lines.append(f"#PBS -e {self.name}.e") + burst_buffer = self._get_burst_buffer(self.burstbuffer) + data_warp = self._get_data_warp(self.datawarp) + if burst_buffer: + lines += burst_buffer + if data_warp: + lines += data_warp return lines def _get_burst_buffer(self, burstbuffer): @@ -1127,9 +1165,24 @@ def post_run_steps(self): self.metadata["output"] = self._output self.metadata["error"] = self._error - self.copy_stage_files() + # copy output and error file from stage directory to top-level test directory + shutil.copy2( + os.path.join(self.stage_dir, os.path.basename(self.metadata["outfile"])), + os.path.join(self.test_root, os.path.basename(self.metadata["outfile"])), + ) + shutil.copy2( + os.path.join(self.stage_dir, os.path.basename(self.metadata["errfile"])), + os.path.join(self.test_root, os.path.basename(self.metadata["errfile"])), + ) + + # update outfile and errfile metadata records which show up in report file + self.metadata["outfile"] = os.path.join( + self.test_root, os.path.basename(self.metadata["outfile"]) + ) + self.metadata["errfile"] = os.path.join( + self.test_root, os.path.basename(self.metadata["errfile"]) + ) - # need these lines after self.copy_stage_files() console.print( f"[blue]{self}[/]: Test completed in {self.metadata['result']['runtime']} seconds with returncode: {self.metadata['result']['returncode']}" ) diff --git a/buildtest/builders/script.py b/buildtest/builders/script.py index 8177133d6..8f4018def 100644 --- a/buildtest/builders/script.py +++ b/buildtest/builders/script.py @@ -117,28 +117,27 @@ def generate_script(self): of shell commands that will be written to file. """ - script_lines = [self.shebang] + lines = [self.shebang] - sched_lines = self.get_job_directives() - if sched_lines: - script_lines += sched_lines + batch_directives = self.get_job_directives() + if batch_directives: + lines += batch_directives - if self.burstbuffer: - burst_buffer_lines = self._get_burst_buffer(self.burstbuffer) - if burst_buffer_lines: - script_lines += burst_buffer_lines + """ + burst_buffer_lines = self._get_burst_buffer(self.burstbuffer) + if burst_buffer_lines: + lines += burst_buffer_lines - if self.datawarp: - data_warp_lines = self._get_data_warp(self.datawarp) - - if data_warp_lines: - script_lines += data_warp_lines + data_warp_lines = self._get_data_warp(self.datawarp) + if data_warp_lines: + lines += data_warp_lines + """ if self.strict: - script_lines.append(self._emit_set_command()) + lines.append(self._emit_set_command()) if self.shell.name == "python": - script_lines = ["#!/bin/bash"] + lines = ["#!/bin/bash"] self.write_python_script() py_script = f"{os.path.join(self.stage_dir, self.name)}.py" python_wrapper = self.buildexecutor.executors[self.executor]._settings[ @@ -149,19 +148,19 @@ def generate_script(self): "python" ) or python_wrapper_buildspec.endswith("python3"): python_wrapper = python_wrapper_buildspec - script_lines.append(f"{python_wrapper} {py_script}") - return script_lines + lines.append(f"{python_wrapper} {py_script}") + return lines # section below is for shell-scripts (bash, sh, csh, zsh, tcsh, zsh) if self.compiler: compiler_variables = self._get_compiler_variables() - script_lines += self._get_variables(compiler_variables) + lines += self._get_variables(compiler_variables) if self.compiler_settings["env"]: - script_lines += self._get_environment(self.compiler_settings["env"]) + lines += self._get_environment(self.compiler_settings["env"]) if self.compiler_settings["vars"]: - script_lines += self._get_variables(self.compiler_settings["vars"]) + lines += self._get_variables(self.compiler_settings["vars"]) env_section = deep_get( self.recipe, "executors", self.executor, "env" @@ -172,24 +171,24 @@ def generate_script(self): env_lines = self._get_environment(env_section) if env_lines: - script_lines += env_lines + lines += env_lines var_lines = self._get_variables(var_section) if var_lines: - script_lines += var_lines + lines += var_lines if self.compiler_settings["modules"]: - script_lines += self.compiler_settings["modules"] + lines += self.compiler_settings["modules"] - script_lines.append("# Content of run section") + lines.append("# Content of run section") if "container" in self.recipe: container_command = self._get_container_command() - script_lines.append(" ".join(container_command)) + lines.append(" ".join(container_command)) # Add run section - script_lines += [self.recipe["run"]] - return script_lines + lines += [self.recipe["run"]] + return lines def _get_container_command(self): """This method is responsible for generating container command for docker, podman, or singularity. This method will return a list of commands to launch container. diff --git a/buildtest/buildsystem/checks.py b/buildtest/buildsystem/checks.py index f56ce3dab..9baafe442 100644 --- a/buildtest/buildsystem/checks.py +++ b/buildtest/buildsystem/checks.py @@ -282,12 +282,10 @@ def is_file_check(builder): Returns: bool: A boolean for is_file status check """ - - assert_is_file = all(is_file(file) for file in builder.status["is_file"]) - console.print( - f"[builder]{builder}[/]: Test all files: {builder.status['is_file']} existences " - ) - for fname in builder.status["is_file"]: + file_list = builder.status["is_file"] + assert_is_file = all(is_file(file) for file in file_list) + console.print(f"[builder]{builder}[/]: Test all files: {file_list} existences ") + for fname in file_list: resolved_fname = resolve_path(fname, exist=True) if is_file(resolved_fname): console.print(f"[blue]{builder}[/]: file: {resolved_fname} is a file ") @@ -309,11 +307,10 @@ def is_dir_check(builder): bool: A boolean for ``is_dir`` status check """ - assert_is_dir = all(is_dir(file) for file in builder.status["is_dir"]) - console.print( - f"[blue]{builder}[/]: Test all files: {builder.status['is_dir']} existences " - ) - for dirname in builder.status["is_dir"]: + dir_list = builder.status["is_dir"] + assert_is_dir = all(is_dir(file) for file in dir_list) + console.print(f"[blue]{builder}[/]: Test all files: {dir_list} existences ") + for dirname in dir_list: resolved_dirname = resolve_path(dirname) if is_dir(resolved_dirname): console.print(