From 0e6e41c6f6f50df6d1ee889f6ea3cd0099c8a011 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Mon, 9 Dec 2024 11:46:33 +1100 Subject: [PATCH 01/12] Updated documentation and small issues raised in review. --- Documentation/source/site-specific-config.rst | 7 ++++++- source/fab/tools/tool.py | 3 +-- source/fab/tools/tool_repository.py | 4 +++- tests/unit_tests/tools/test_shell.py | 8 ++++---- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/Documentation/source/site-specific-config.rst b/Documentation/source/site-specific-config.rst index b7481c08..9faaeb04 100644 --- a/Documentation/source/site-specific-config.rst +++ b/Documentation/source/site-specific-config.rst @@ -20,7 +20,12 @@ contains all the tools that will be used during the build process, but it can only store one tool per category. If a certain tool should not be defined in the toolbox, the default from the `ToolRepository` will be used. This is useful for many standard tools like `git`, `rsync` -etc that de-facto will never be changed. +etc that de-facto will never be changed. Fab will check if a tool +is actually available on the system before adding it to a ToolBox. +This is typically done by trying to run the tool with some testing +parameters, for example requesting its version number. If this fails, +the tool is considered not to be available and will not be used (unless +the user explicitly puts a tool into the ToolBox). .. note:: If you need to use for example different compilers for different files, you would implement this as a `meta-compiler`: diff --git a/source/fab/tools/tool.py b/source/fab/tools/tool.py index a870c657..78a8de62 100644 --- a/source/fab/tools/tool.py +++ b/source/fab/tools/tool.py @@ -63,8 +63,7 @@ def check_available(self) -> bool: :returns: whether the tool is working (True) or not. ''' try: - op = self._availability_option - self.run(op) + self.run(self._availability_option) except (RuntimeError, FileNotFoundError): return False return True diff --git a/source/fab/tools/tool_repository.py b/source/fab/tools/tool_repository.py index 0a17d7e6..965e252b 100644 --- a/source/fab/tools/tool_repository.py +++ b/source/fab/tools/tool_repository.py @@ -75,7 +75,7 @@ def __init__(self): # Add the common shells. While Fab itself does not need this, # it is a very convenient tool for user configuration (e.g. to # query nc-config etc) - for shell_name in ["bash", "sh", "ksh", "dash"]: + for shell_name in ["sh", "bash", "ksh", "dash"]: self.add_tool(Shell(shell_name)) # Now create the potential mpif90 and Cray ftn wrapper @@ -174,6 +174,8 @@ def get_default(self, category: Category, OpenMP. :raises KeyError: if the category does not exist. + :raises RuntimeError: if no tool in the requested category is + available on the system. :raises RuntimeError: if no compiler/linker is found with the requested level of MPI support (yes or no). ''' diff --git a/tests/unit_tests/tools/test_shell.py b/tests/unit_tests/tools/test_shell.py index 38ba8c71..deab54d4 100644 --- a/tests/unit_tests/tools/test_shell.py +++ b/tests/unit_tests/tools/test_shell.py @@ -39,11 +39,11 @@ def test_shell_check_available(): def test_shell_exec_single_arg(): '''Test running a shell script without additional parameters.''' - bash = Shell("ksh") + ksh = Shell("ksh") mock_result = mock.Mock(returncode=0) with mock.patch('fab.tools.tool.subprocess.run', return_value=mock_result) as tool_run: - bash.exec("echo") + ksh.exec("echo") tool_run.assert_called_with(['ksh', '-c', 'echo'], capture_output=True, env=None, cwd=None, check=False) @@ -51,11 +51,11 @@ def test_shell_exec_single_arg(): def test_shell_exec_multiple_args(): '''Test running a shell script with parameters.''' - bash = Shell("ksh") + ksh = Shell("ksh") mock_result = mock.Mock(returncode=0) with mock.patch('fab.tools.tool.subprocess.run', return_value=mock_result) as tool_run: - bash.exec(["some", "shell", "function"]) + ksh.exec(["some", "shell", "function"]) tool_run.assert_called_with(['ksh', '-c', 'some', 'shell', 'function'], capture_output=True, env=None, cwd=None, check=False) From 72fb659ebf7ed4e10c090b5c141128729c7ab186 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Mon, 9 Dec 2024 14:09:01 +1100 Subject: [PATCH 02/12] Fixed failing test. --- tests/unit_tests/tools/test_tool_repository.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index 54d15e92..0c7d77e5 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -182,5 +182,5 @@ def test_tool_repository_no_tool_available(): is_available.return_value = False with pytest.raises(RuntimeError) as err: tr.get_default(Category.SHELL) - assert ("Can't find available 'SHELL' tool. Tools are 'bash,sh,ksh," + assert ("Can't find available 'SHELL' tool. Tools are 'sh,bash,ksh," "dash'" in str(err.value)) From e7ae9fea6ac7cbb93a561afb3d20d8e1a813021f Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 19 Dec 2024 22:52:05 +1100 Subject: [PATCH 03/12] Improve support for MPI wrappers (#352) --- Documentation/source/site-specific-config.rst | 63 ++++ source/fab/steps/compile_c.py | 13 +- source/fab/steps/compile_fortran.py | 22 +- source/fab/tools/__init__.py | 11 +- source/fab/tools/compiler.py | 117 ++---- source/fab/tools/compiler_wrapper.py | 209 +++++++++++ source/fab/tools/flags.py | 14 +- source/fab/tools/linker.py | 5 + source/fab/tools/preprocessor.py | 4 +- source/fab/tools/tool.py | 45 ++- source/fab/tools/tool_repository.py | 28 +- source/fab/tools/versioning.py | 2 +- tests/unit_tests/steps/test_compile_c.py | 14 +- .../unit_tests/steps/test_compile_fortran.py | 29 +- tests/unit_tests/tools/test_compiler.py | 159 +++----- .../unit_tests/tools/test_compiler_wrapper.py | 348 ++++++++++++++++++ tests/unit_tests/tools/test_flags.py | 14 + tests/unit_tests/tools/test_psyclone.py | 14 + tests/unit_tests/tools/test_tool.py | 28 +- 19 files changed, 871 insertions(+), 268 deletions(-) create mode 100644 source/fab/tools/compiler_wrapper.py create mode 100644 tests/unit_tests/tools/test_compiler_wrapper.py diff --git a/Documentation/source/site-specific-config.rst b/Documentation/source/site-specific-config.rst index 0fd0f840..b7481c08 100644 --- a/Documentation/source/site-specific-config.rst +++ b/Documentation/source/site-specific-config.rst @@ -161,6 +161,64 @@ On the other hand, if no MPI is requested, an MPI-enabled compiler might be returned, which does not affect the final result, since an MPI compiler just adds include- and library-paths. + +Compiler Wrapper +================ +Fab supports the concept of a compiler wrapper, which is typically +a script that calls the actual compiler. An example for a wrapper is +`mpif90`, which might call a GNU or Intel based compiler (with additional +parameter for accessing the MPI specific include and library paths.). +An example to create a `mpicc` wrapper (note that this wrapper is already +part of Fab, there is no need to explicitly add this yourself): + +.. code-block:: + :linenos: + :caption: Defining an mpicc compiler wrapper + + class Mpicc(CompilerWrapper): + def __init__(self, compiler: Compiler): + super().__init__(name=f"mpicc-{compiler.name}", + exec_name="mpicc", + compiler=compiler, mpi=True) + +The tool system allows several different tools to use the same name +for the executable, as long as the Fab name is different, i.e. the +`mpicc-{compiler.name}`. The tool +repository will automatically add compiler wrapper for `mpicc` and +`mpif90` for any compiler that is added by Fab. If you want to add +a new compiler, which can also be invoked using `mpicc`, you need +to add a compiler wrapper as follows: + +.. code-block:: + :linenos: + :caption: Adding a mpicc wrapper to the tool repository + + my_new_compiler = MyNewCompiler() + ToolRepository().add_tool(my_new_compiler) + my_new_mpicc = Mpicc(MyNewCompiler) + ToolRepository().add_tool(my_new_mpicc) + +When creating a completely new compiler and compiler wrapper +as in the example above, it is strongly recommended to add +the new compiler instance to the tool repository as well. This will +allow the wrapper and the wrapped compiler to share flags. For example, +a user script can query the ToolRepository to get the original compiler +and modify its flags. These modification will then automatically be +applied to the wrapper as well: + +.. code-block:: + :linenos: + :caption: Sharing flags between compiler and compiler wrapper + + tr = ToolRepository() + my_compiler = tr.get_tool(Category.C_COMPILER, "my_compiler") + my_mpicc = tr.get_tool(Category.C_COMPILER, "mpicc-my_compiler") + + my_compiler.add_flags(["-a", "-b"]) + + assert my_mpicc.flags == ["-a", "-b"] + + TODO ==== At this stage compiler flags are still set in the corresponding Fab @@ -169,3 +227,8 @@ definition in the compiler objects. This will allow a site to define their own set of default flags to be used with a certain compiler by replacing or updating a compiler instance in the Tool Repository + +Also, a lot of content in this chapter is not actually about site-specific +configuration. This should likely be renamed or split (once we +have details about the using site-specific configuration, which might be +once the Baf base script is added to Fab). \ No newline at end of file diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index 41332dda..c7d014d7 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -10,7 +10,7 @@ import logging import os from dataclasses import dataclass -from typing import List, Dict, Optional, Tuple +from typing import cast, Dict, List, Optional, Tuple from fab import FabException from fab.artefacts import (ArtefactsGetter, ArtefactSet, ArtefactStore, @@ -19,7 +19,7 @@ from fab.metrics import send_metric from fab.parse.c import AnalysedC from fab.steps import check_for_errors, run_mp, step -from fab.tools import Category, CCompiler, Flags +from fab.tools import Category, Compiler, Flags from fab.util import CompiledFile, log_or_dot, Timer, by_type logger = logging.getLogger(__name__) @@ -124,9 +124,12 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): analysed_file, mp_payload = arg config = mp_payload.config compiler = config.tool_box[Category.C_COMPILER] - if not isinstance(compiler, CCompiler): - raise RuntimeError(f"Unexpected tool '{compiler.name}' of type " - f"'{type(compiler)}' instead of CCompiler") + if compiler.category != Category.C_COMPILER: + raise RuntimeError(f"Unexpected tool '{compiler.name}' of category " + f"'{compiler.category}' instead of CCompiler") + # Tool box returns a Tool, in order to make mypy happy, we need + # to cast it to be a Compiler. + compiler = cast(Compiler, compiler) with Timer() as timer: flags = Flags(mp_payload.flags.flags_for_path(path=analysed_file.fpath, config=config)) diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index 4b065811..e5767fae 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -14,7 +14,7 @@ from dataclasses import dataclass from itertools import chain from pathlib import Path -from typing import List, Set, Dict, Tuple, Optional, Union +from typing import cast, Dict, List, Optional, Set, Tuple, Union from fab.artefacts import (ArtefactsGetter, ArtefactSet, ArtefactStore, FilterBuildTrees) @@ -22,7 +22,7 @@ from fab.metrics import send_metric from fab.parse.fortran import AnalysedFortran from fab.steps import check_for_errors, run_mp, step -from fab.tools import Category, Compiler, Flags, FortranCompiler +from fab.tools import Category, Compiler, Flags from fab.util import (CompiledFile, log_or_dot_finish, log_or_dot, Timer, by_type, file_checksum) @@ -133,9 +133,12 @@ def handle_compiler_args(config: BuildConfig, common_flags=None, # Command line tools are sometimes specified with flags attached. compiler = config.tool_box[Category.FORTRAN_COMPILER] - if not isinstance(compiler, FortranCompiler): - raise RuntimeError(f"Unexpected tool '{compiler.name}' of type " - f"'{type(compiler)}' instead of FortranCompiler") + if compiler.category != Category.FORTRAN_COMPILER: + raise RuntimeError(f"Unexpected tool '{compiler.name}' of category " + f"'{compiler.category}' instead of FortranCompiler") + # The ToolBox returns a Tool. In order to make mypy happy, we need to + # cast this to become a Compiler. + compiler = cast(Compiler, compiler) logger.info( f'Fortran compiler is {compiler} {compiler.get_version_string()}') @@ -263,10 +266,13 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ config = mp_common_args.config compiler = config.tool_box.get_tool(Category.FORTRAN_COMPILER, config.mpi) - if not isinstance(compiler, FortranCompiler): - raise RuntimeError(f"Unexpected tool '{compiler.name}' of type " - f"'{type(compiler)}' instead of " + if compiler.category != Category.FORTRAN_COMPILER: + raise RuntimeError(f"Unexpected tool '{compiler.name}' of " + f"category '{compiler.category}' instead of " f"FortranCompiler") + # The ToolBox returns a Tool, but we need to tell mypy that + # this is a Compiler + compiler = cast(Compiler, compiler) flags = Flags(mp_common_args.flags.flags_for_path( path=analysed_file.fpath, config=config)) diff --git a/source/fab/tools/__init__.py b/source/fab/tools/__init__.py index ed5850c5..45eb666f 100644 --- a/source/fab/tools/__init__.py +++ b/source/fab/tools/__init__.py @@ -11,8 +11,8 @@ from fab.tools.category import Category from fab.tools.compiler import (CCompiler, Compiler, FortranCompiler, Gcc, Gfortran, GnuVersionHandling, Icc, Ifort, - IntelVersionHandling, MpiGcc, MpiGfortran, - MpiIcc, MpiIfort) + IntelVersionHandling) +from fab.tools.compiler_wrapper import CompilerWrapper, Mpicc, Mpif90 from fab.tools.flags import Flags from fab.tools.linker import Linker from fab.tools.psyclone import Psyclone @@ -29,6 +29,7 @@ "CCompiler", "Compiler", "CompilerSuiteTool", + "CompilerWrapper", "Cpp", "CppFortran", "Fcm", @@ -43,10 +44,8 @@ "Ifort", "IntelVersionHandling", "Linker", - "MpiGcc", - "MpiGfortran", - "MpiIcc", - "MpiIfort", + "Mpif90", + "Mpicc", "Preprocessor", "Psyclone", "Rsync", diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 5c2dfea5..7a635d78 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -38,6 +38,9 @@ class Compiler(CompilerSuiteTool): of the output file :param openmp_flag: the flag to use to enable OpenMP. If no flag is specified, it is assumed that the compiler does not support OpenMP. + :param availability_option: a command line option for the tool to test + if the tool is available on the current system. Defaults to + `--version`. ''' # pylint: disable=too-many-arguments @@ -48,19 +51,21 @@ def __init__(self, name: str, mpi: bool = False, compile_flag: Optional[str] = None, output_flag: Optional[str] = None, - openmp_flag: Optional[str] = None): - super().__init__(name, exec_name, suite, mpi=mpi, category=category) + openmp_flag: Optional[str] = None, + availability_option: Optional[str] = None): + super().__init__(name, exec_name, suite, category=category, + availability_option=availability_option) self._version: Union[Tuple[int, ...], None] = None + self._mpi = mpi self._compile_flag = compile_flag if compile_flag else "-c" self._output_flag = output_flag if output_flag else "-o" self._openmp_flag = openmp_flag if openmp_flag else "" self.flags.extend(os.getenv("FFLAGS", "").split()) - def get_hash(self) -> int: - ''':returns: a hash based on the compiler name and version. - ''' - return (zlib.crc32(self.name.encode()) + - zlib.crc32(self.get_version_string().encode())) + @property + def mpi(self) -> bool: + ''':returns: whether this compiler supports MPI or not.''' + return self._mpi @property def openmp(self) -> bool: @@ -70,10 +75,15 @@ def openmp(self) -> bool: @property def openmp_flag(self) -> str: - ''':returns: The flag to enable OpenMP for this compiler. - ''' + ''':returns: the flag to enable OpenMP.''' return self._openmp_flag + def get_hash(self) -> int: + ''':returns: a hash based on the compiler name and version. + ''' + return (zlib.crc32(self.name.encode()) + + zlib.crc32(self.get_version_string().encode())) + def compile_file(self, input_file: Path, output_file: Path, openmp: bool, @@ -93,11 +103,11 @@ def compile_file(self, input_file: Path, params: List[Union[Path, str]] = [self._compile_flag] if openmp: - params.append(self._openmp_flag) + params.append(self.openmp_flag) if add_flags: - if self._openmp_flag in add_flags: + if self.openmp_flag in add_flags: warnings.warn( - f"OpenMP flag '{self._openmp_flag}' explicitly provided. " + f"OpenMP flag '{self.openmp_flag}' explicitly provided. " f"OpenMP should be enabled in the BuildConfiguration " f"instead.") params += add_flags @@ -149,7 +159,6 @@ def get_version(self) -> Tuple[int, ...]: version_string = self.parse_version_output(self.category, output) # Expect the version to be dot-separated integers. - # todo: Not all will be integers? but perhaps major and minor? try: version = tuple(int(x) for x in version_string.split('.')) except ValueError as err: @@ -246,14 +255,14 @@ class FortranCompiler(Compiler): :param name: name of the compiler. :param exec_name: name of the executable to start. :param suite: name of the compiler suite. - :param mpi: whether the compiler or linker support MPI. + :param mpi: whether MPI is supported by this compiler or not. :param compile_flag: the compilation flag to use when only requesting compilation (not linking). :param output_flag: the compilation flag to use to indicate the name of the output file + :param openmp_flag: the flag to use to enable OpenMP :param module_folder_flag: the compiler flag to indicate where to store created module files. - :param openmp_flag: the flag to use to enable OpenMP :param syntax_only_flag: flag to indicate to only do a syntax check. The side effect is that the module files are created. ''' @@ -293,11 +302,12 @@ def compile_file(self, input_file: Path, output_file: Path, openmp: bool, add_flags: Union[None, List[str]] = None, - syntax_only: bool = False): + syntax_only: Optional[bool] = False): '''Compiles a file. :param input_file: the name of the input file. :param output_file: the name of the output file. + :param openmp: if compilation should be done with OpenMP. :param add_flags: additional flags for the compiler. :param syntax_only: if set, the compiler will only do a syntax check @@ -306,7 +316,9 @@ def compile_file(self, input_file: Path, params: List[str] = [] if add_flags: new_flags = Flags(add_flags) - new_flags.remove_flag(self._module_folder_flag, has_parameter=True) + if self._module_folder_flag: + new_flags.remove_flag(self._module_folder_flag, + has_parameter=True) new_flags.remove_flag(self._compile_flag, has_parameter=False) params += new_flags @@ -375,18 +387,6 @@ def __init__(self, openmp_flag="-fopenmp") -# ============================================================================ -class MpiGcc(Gcc): - '''Class for a simple wrapper around gcc that supports MPI. - It calls `mpicc`. - ''' - - def __init__(self): - super().__init__(name="mpicc-gcc", - exec_name="mpicc", - mpi=True) - - # ============================================================================ class Gfortran(GnuVersionHandling, FortranCompiler): '''Class for GNU's gfortran compiler. @@ -396,28 +396,14 @@ class Gfortran(GnuVersionHandling, FortranCompiler): :param mpi: whether the compiler supports MPI. ''' - def __init__(self, - name: str = "gfortran", - exec_name: str = "gfortran", - mpi: bool = False): - super().__init__(name, exec_name, suite="gnu", mpi=mpi, + def __init__(self, name: str = "gfortran", + exec_name: str = "gfortran"): + super().__init__(name, exec_name, suite="gnu", openmp_flag="-fopenmp", module_folder_flag="-J", syntax_only_flag="-fsyntax-only") -# ============================================================================ -class MpiGfortran(Gfortran): - '''Class for a simple wrapper around gfortran that supports MPI. - It calls `mpif90`. - ''' - - def __init__(self): - super().__init__(name="mpif90-gfortran", - exec_name="mpif90", - mpi=True) - - # ============================================================================ class IntelVersionHandling(): '''Mixin to handle version information from Intel compilers''' @@ -460,24 +446,10 @@ class Icc(IntelVersionHandling, CCompiler): :param exec_name: name of the executable. :param mpi: whether the compiler supports MPI. ''' - def __init__(self, - name: str = "icc", - exec_name: str = "icc", - mpi: bool = False): - super().__init__(name, exec_name, suite="intel-classic", mpi=mpi, - openmp_flag="-qopenmp") - -# ============================================================================ -class MpiIcc(Icc): - '''Class for a simple wrapper around icc that supports MPI. - It calls `mpicc`. - ''' - - def __init__(self): - super().__init__(name="mpicc-icc", - exec_name="mpicc", - mpi=True) + def __init__(self, name: str = "icc", exec_name: str = "icc"): + super().__init__(name, exec_name, suite="intel-classic", + openmp_flag="-qopenmp") # ============================================================================ @@ -489,23 +461,8 @@ class Ifort(IntelVersionHandling, FortranCompiler): :param mpi: whether the compiler supports MPI. ''' - def __init__(self, - name: str = "ifort", - exec_name: str = "ifort", - mpi: bool = False): - super().__init__(name, exec_name, suite="intel-classic", mpi=mpi, + def __init__(self, name: str = "ifort", exec_name: str = "ifort"): + super().__init__(name, exec_name, suite="intel-classic", module_folder_flag="-module", openmp_flag="-qopenmp", syntax_only_flag="-syntax-only") - - -# ============================================================================ -class MpiIfort(Ifort): - '''Class for a simple wrapper around ifort that supports MPI. - It calls `mpif90`. - ''' - - def __init__(self): - super().__init__(name="mpif90-ifort", - exec_name="mpif90", - mpi=True) diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py new file mode 100644 index 00000000..19186b2d --- /dev/null +++ b/source/fab/tools/compiler_wrapper.py @@ -0,0 +1,209 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## + +"""This file contains the base class for any compiler, and derived +classes for gcc, gfortran, icc, ifort +""" + +from pathlib import Path +from typing import cast, List, Optional, Tuple, Union + +from fab.tools.category import Category +from fab.tools.compiler import Compiler, FortranCompiler +from fab.tools.flags import Flags + + +class CompilerWrapper(Compiler): + '''A decorator-based compiler wrapper. It basically uses a different + executable name when compiling, but otherwise behaves like the wrapped + compiler. An example of a compiler wrapper is `mpif90` (which can + internally call e.g. gfortran, icc, ...) + + :param name: name of the wrapper. + :param exec_name: name of the executable to call. + :param compiler: the compiler that is decorated. + :param mpi: whether MPI is supported by this compiler or not. + ''' + + def __init__(self, name: str, exec_name: str, + compiler: Compiler, + mpi: bool = False): + self._compiler = compiler + super().__init__( + name=name, exec_name=exec_name, + category=self._compiler.category, + suite=self._compiler.suite, + mpi=mpi, + availability_option=self._compiler.availability_option) + # We need to have the right version to parse the version output + # So we set this function based on the function that the + # wrapped compiler uses: + setattr(self, "parse_version_output", compiler.parse_version_output) + + def __str__(self): + return f"{type(self).__name__}({self._compiler.name})" + + def get_version(self) -> Tuple[int, ...]: + """Determines the version of the compiler. The implementation in the + compiler wrapper additionally ensures that the wrapper compiler and + compiler wrapper report both the same version. This verifies that the + user's build environment is as expected. For example, this will check + if mpif90 from mpif90-ifort does indeed invoke ifort (and not e.g. + gfortran). + + :returns: a tuple of at least 2 integers, representing the version + e.g. (6, 10, 1) for version '6.10.1'. + + :raises RuntimeError: if the compiler was not found, or if it returned + an unrecognised output from the version command. + :raises RuntimeError: if the compiler wrapper and wrapped compiler + have different version numbers. + """ + + if self._version is not None: + return self._version + + try: + compiler_version = self._compiler.get_version() + except RuntimeError as err: + raise RuntimeError(f"Cannot get version of wrapped compiler '" + f"{self._compiler}") from err + + wrapper_version = super().get_version() + if compiler_version != wrapper_version: + compiler_version_string = self._compiler.get_version_string() + # We cannot call super().get_version_string(), since this calls + # calls get_version(), so we get an infinite recursion + wrapper_version_string = ".".join(str(x) for x in wrapper_version) + raise RuntimeError(f"Different version for compiler " + f"'{self._compiler}' " + f"({compiler_version_string}) and compiler " + f"wrapper '{self}' ({wrapper_version_string}).") + self._version = wrapper_version + return wrapper_version + + @property + def compiler(self) -> Compiler: + ''':returns: the compiler that is wrapped by this CompilerWrapper.''' + return self._compiler + + @property + def flags(self) -> Flags: + ''':returns: the flags to be used with this tool.''' + return Flags(self._compiler.flags + self._flags) + + @property + def suite(self) -> str: + ''':returns: the compiler suite of this tool.''' + return self._compiler.suite + + @property + def openmp_flag(self) -> str: + '''Returns the flag to enable OpenMP.''' + return self._compiler.openmp_flag + + @property + def has_syntax_only(self) -> bool: + ''':returns: whether this compiler supports a syntax-only feature. + + :raises RuntimeError: if this function is called for a non-Fortran + wrapped compiler. + ''' + + if self._compiler.category == Category.FORTRAN_COMPILER: + return cast(FortranCompiler, self._compiler).has_syntax_only + + raise RuntimeError(f"Compiler '{self._compiler.name}' has " + f"no has_syntax_only.") + + def set_module_output_path(self, path: Path): + '''Sets the output path for modules. + + :params path: the path to the output directory. + + :raises RuntimeError: if this function is called for a non-Fortran + wrapped compiler. + ''' + + if self._compiler.category != Category.FORTRAN_COMPILER: + raise RuntimeError(f"Compiler '{self._compiler.name}' has no " + f"'set_module_output_path' function.") + cast(FortranCompiler, self._compiler).set_module_output_path(path) + + def compile_file(self, input_file: Path, + output_file: Path, + openmp: bool, + add_flags: Union[None, List[str]] = None, + syntax_only: Optional[bool] = None): + # pylint: disable=too-many-arguments + '''Compiles a file using the wrapper compiler. It will temporarily + change the executable name of the wrapped compiler, and then calls + the original compiler (to get all its parameters) + + :param input_file: the name of the input file. + :param output_file: the name of the output file. + :param openmp: if compilation should be done with OpenMP. + :param add_flags: additional flags for the compiler. + :param syntax_only: if set, the compiler will only do + a syntax check + ''' + + # TODO #370: replace change_exec_name, and instead provide + # a function that returns the whole command line, which can + # then be modified here. + orig_compiler_name = self._compiler.exec_name + self._compiler.change_exec_name(self.exec_name) + if add_flags is None: + add_flags = [] + if self._compiler.category is Category.FORTRAN_COMPILER: + # Mypy complains that self._compiler does not take the syntax + # only parameter. Since we know it's a FortranCompiler. + # do a cast to tell mypy that this is now a Fortran compiler + # (or a CompilerWrapper in case of nested CompilerWrappers, + # which also supports the syntax_only flag anyway) + self._compiler = cast(FortranCompiler, self._compiler) + self._compiler.compile_file(input_file, output_file, openmp=openmp, + add_flags=self.flags + add_flags, + syntax_only=syntax_only, + ) + else: + if syntax_only is not None: + raise RuntimeError(f"Syntax-only cannot be used with compiler " + f"'{self.name}'.") + self._compiler.compile_file(input_file, output_file, openmp=openmp, + add_flags=self.flags+add_flags + ) + self._compiler.change_exec_name(orig_compiler_name) + + +# ============================================================================ +class Mpif90(CompilerWrapper): + '''Class for a simple wrapper for using a compiler driver (like mpif90) + It will be using the name "mpif90-COMPILER_NAME" and calls `mpif90`. + All flags from the original compiler will be used when using the wrapper + as compiler. + + :param compiler: the compiler that the mpif90 wrapper will use. + ''' + + def __init__(self, compiler: Compiler): + super().__init__(name=f"mpif90-{compiler.name}", + exec_name="mpif90", compiler=compiler, mpi=True) + + +# ============================================================================ +class Mpicc(CompilerWrapper): + '''Class for a simple wrapper for using a compiler driver (like mpicc) + It will be using the name "mpicc-COMPILER_NAME" and calls `mpicc`. + All flags from the original compiler will be used when using the wrapper + as compiler. + + :param compiler: the compiler that the mpicc wrapper will use. + ''' + + def __init__(self, compiler: Compiler): + super().__init__(name=f"mpicc-{compiler.name}", + exec_name="mpicc", compiler=compiler, mpi=True) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index 6303a754..d83d57cc 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -10,7 +10,7 @@ ''' import logging -from typing import List, Optional +from typing import List, Optional, Union import warnings from fab.util import string_checksum @@ -38,6 +38,18 @@ def checksum(self) -> str: """ return string_checksum(str(self)) + def add_flags(self, new_flags: Union[str, List[str]]): + '''Adds the specified flags to the list of flags. + + :param new_flags: A single string or list of strings which are the + flags to be added. + ''' + + if isinstance(new_flags, str): + self.append(new_flags) + else: + self.extend(new_flags) + def remove_flag(self, remove_flag: str, has_parameter: bool = False): '''Removes all occurrences of `remove_flag` in flags`. If has_parameter is defined, the next entry in flags will also be diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 02932a18..69cfcec2 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -51,6 +51,11 @@ def __init__(self, name: Optional[str] = None, self._compiler = compiler self.flags.extend(os.getenv("LDFLAGS", "").split()) + @property + def mpi(self) -> bool: + ''':returns: whether the linker supports MPI or not.''' + return self._compiler.mpi + def check_available(self) -> bool: ''' :returns: whether the linker is available or not. We do this diff --git a/source/fab/tools/preprocessor.py b/source/fab/tools/preprocessor.py index be9f9d43..e620ce2a 100644 --- a/source/fab/tools/preprocessor.py +++ b/source/fab/tools/preprocessor.py @@ -26,7 +26,7 @@ class Preprocessor(Tool): def __init__(self, name: str, exec_name: Union[str, Path], category: Category, - availablility_option: Optional[str] = None): + availability_option: Optional[str] = None): super().__init__(name, exec_name, category) self._version = None @@ -74,4 +74,4 @@ def __init__(self): # fpp -V prints version information, but then hangs (i.e. reading # from stdin), so use -what to see if it is available super().__init__("fpp", "fpp", Category.FORTRAN_PREPROCESSOR, - availablility_option="-what") + availability_option="-what") diff --git a/source/fab/tools/tool.py b/source/fab/tools/tool.py index 9eaa42e1..cb8a7a06 100644 --- a/source/fab/tools/tool.py +++ b/source/fab/tools/tool.py @@ -36,14 +36,14 @@ class Tool: def __init__(self, name: str, exec_name: Union[str, Path], category: Category = Category.MISC, - availablility_option: Optional[str] = None): + availability_option: Optional[str] = None): self._logger = logging.getLogger(__name__) self._name = name self._exec_name = str(exec_name) self._flags = Flags() self._category = category - if availablility_option: - self._availability_option = availablility_option + if availability_option: + self._availability_option = availability_option else: self._availability_option = "--version" @@ -91,11 +91,26 @@ def exec_name(self) -> str: ''':returns: the name of the executable.''' return self._exec_name + def change_exec_name(self, exec_name: str): + '''Changes the name of the executable This function should in general + not be used (typically it is better to create a new tool instead). The + function is only provided to support CompilerWrapper (like mpif90), + which need all parameters from the original compiler, but call the + wrapper. The name of the compiler will be changed just before + compilation, and then set back to its original value + ''' + self._exec_name = exec_name + @property def name(self) -> str: ''':returns: the name of the tool.''' return self._name + @property + def availability_option(self) -> str: + ''':returns: the option to use to check if the tool is available.''' + return self._availability_option + @property def category(self) -> Category: ''':returns: the category of this tool.''' @@ -106,6 +121,14 @@ def flags(self) -> Flags: ''':returns: the flags to be used with this tool.''' return self._flags + def add_flags(self, new_flags: Union[str, List[str]]): + '''Adds the specified flags to the list of flags. + + :param new_flags: A single string or list of strings which are the + flags to be added. + ''' + self._flags.add_flags(new_flags) + @property def logger(self) -> logging.Logger: ''':returns: a logger object for convenience.''' @@ -181,20 +204,18 @@ class CompilerSuiteTool(Tool): :param exec_name: name of the executable to start. :param suite: name of the compiler suite. :param category: the Category to which this tool belongs. - :param mpi: whether the compiler or linker support MPI. + :param availability_option: a command line option for the tool to test + if the tool is available on the current system. Defaults to + `--version`. ''' def __init__(self, name: str, exec_name: Union[str, Path], suite: str, - category: Category, mpi: bool = False): - super().__init__(name, exec_name, category) + category: Category, + availability_option: Optional[str] = None): + super().__init__(name, exec_name, category, + availability_option=availability_option) self._suite = suite - self._mpi = mpi @property def suite(self) -> str: ''':returns: the compiler suite of this tool.''' return self._suite - - @property - def mpi(self) -> bool: - ''':returns: whether this tool supports MPI or not.''' - return self._mpi diff --git a/source/fab/tools/tool_repository.py b/source/fab/tools/tool_repository.py index 70479d55..6a077b67 100644 --- a/source/fab/tools/tool_repository.py +++ b/source/fab/tools/tool_repository.py @@ -12,10 +12,11 @@ from __future__ import annotations import logging -from typing import Any, Optional, Type +from typing import cast, Optional from fab.tools.tool import Tool from fab.tools.category import Category +from fab.tools.compiler import Compiler from fab.tools.linker import Linker from fab.tools.versioning import Fcm, Git, Subversion @@ -60,26 +61,30 @@ def __init__(self): # We get circular dependencies if imported at top of the file: # pylint: disable=import-outside-toplevel from fab.tools import (Ar, Cpp, CppFortran, Gcc, Gfortran, - Icc, Ifort, MpiGcc, MpiGfortran, - MpiIcc, MpiIfort, Psyclone, Rsync) + Icc, Ifort, Psyclone, Rsync) for cls in [Gcc, Icc, Gfortran, Ifort, Cpp, CppFortran, - MpiGcc, MpiGfortran, MpiIcc, MpiIfort, Fcm, Git, Subversion, Ar, Psyclone, Rsync]: - self.add_tool(cls) + self.add_tool(cls()) - def add_tool(self, cls: Type[Any]): + from fab.tools.compiler_wrapper import Mpif90, Mpicc + all_fc = self[Category.FORTRAN_COMPILER][:] + for fc in all_fc: + mpif90 = Mpif90(fc) + self.add_tool(mpif90) + + all_cc = self[Category.C_COMPILER][:] + for cc in all_cc: + mpicc = Mpicc(cc) + self.add_tool(mpicc) + + def add_tool(self, tool: Tool): '''Creates an instance of the specified class and adds it to the tool repository. :param cls: the tool to instantiate. ''' - # Note that we cannot declare `cls` to be `Type[Tool]`, since the - # Tool constructor requires arguments, but the classes used here are - # derived from Tool which do not require any arguments (e.g. Ifort) - - tool = cls() # We do not test if a tool is actually available. The ToolRepository # contains the tools that FAB knows about. It is the responsibility # of the ToolBox to make sure only available tools are added. @@ -87,6 +92,7 @@ def add_tool(self, cls: Type[Any]): # If we have a compiler, add the compiler as linker as well if tool.is_compiler: + tool = cast(Compiler, tool) linker = Linker(name=f"linker-{tool.name}", compiler=tool) self[linker.category].append(linker) diff --git a/source/fab/tools/versioning.py b/source/fab/tools/versioning.py index 01c11264..410cc250 100644 --- a/source/fab/tools/versioning.py +++ b/source/fab/tools/versioning.py @@ -29,7 +29,7 @@ def __init__(self, name: str, :param category: Tool belongs to this category. """ super().__init__(name, exec_name, category, - availablility_option="help") + availability_option="help") # ============================================================================= diff --git a/tests/unit_tests/steps/test_compile_c.py b/tests/unit_tests/steps/test_compile_c.py index 8e8c8845..06a309ba 100644 --- a/tests/unit_tests/steps/test_compile_c.py +++ b/tests/unit_tests/steps/test_compile_c.py @@ -42,20 +42,19 @@ def test_compile_c_wrong_compiler(content): config = content[0] tb = config.tool_box # Take the Fortran compiler - fc = tb[Category.FORTRAN_COMPILER] - # And set its category to C_COMPILER - fc._category = Category.C_COMPILER + cc = tb[Category.C_COMPILER] # So overwrite the C compiler with the re-categorised Fortran compiler - tb.add_tool(fc, silent_replace=True) + cc._is_available = True + tb.add_tool(cc, silent_replace=True) + cc._category = Category.FORTRAN_COMPILER # Now check that _compile_file detects the incorrect class of the # C compiler mp_common_args = mock.Mock(config=config) with pytest.raises(RuntimeError) as err: _compile_file((None, mp_common_args)) - assert ("Unexpected tool 'mock_fortran_compiler' of type '' instead of CCompiler" - in str(err.value)) + assert ("Unexpected tool 'mock_c_compiler' of category " + "'FORTRAN_COMPILER' instead of CCompiler" in str(err.value)) # This is more of an integration test than a unit test @@ -66,7 +65,6 @@ def test_vanilla(self, content): '''Ensure the command is formed correctly.''' config, _, expect_hash = content compiler = config.tool_box[Category.C_COMPILER] - print("XX", compiler, type(compiler), compiler.category) # run the step with mock.patch("fab.steps.compile_c.send_metric") as send_metric: with mock.patch('pathlib.Path.mkdir'): diff --git a/tests/unit_tests/steps/test_compile_fortran.py b/tests/unit_tests/steps/test_compile_fortran.py index c9feff49..e3a1168c 100644 --- a/tests/unit_tests/steps/test_compile_fortran.py +++ b/tests/unit_tests/steps/test_compile_fortran.py @@ -35,27 +35,24 @@ def fixture_artefact_store(analysed_files): def test_compile_cc_wrong_compiler(tool_box): '''Test if a non-C compiler is specified as c compiler. ''' - config = BuildConfig('proj', tool_box) - # Take the Fortran compiler - cc = tool_box[Category.C_COMPILER] - # And set its category to C_COMPILER - cc._category = Category.FORTRAN_COMPILER - # So overwrite the C compiler with the re-categories Fortran compiler - tool_box.add_tool(cc, silent_replace=True) - - # Now check that _compile_file detects the incorrect class of the - # C compiler + config = BuildConfig('proj', tool_box, mpi=False, openmp=False) + # Get the default Fortran compiler into the ToolBox + fc = tool_box[Category.FORTRAN_COMPILER] + # But then change its category to be a C compiler: + fc._category = Category.C_COMPILER + + # Now check that _compile_file detects the incorrect category of the + # Fortran compiler mp_common_args = mock.Mock(config=config) with pytest.raises(RuntimeError) as err: process_file((None, mp_common_args)) - assert ("Unexpected tool 'mock_c_compiler' of type '' instead of FortranCompiler" - in str(err.value)) + assert ("Unexpected tool 'mock_fortran_compiler' of category " + "'C_COMPILER' instead of FortranCompiler" in str(err.value)) + with pytest.raises(RuntimeError) as err: handle_compiler_args(config) - assert ("Unexpected tool 'mock_c_compiler' of type '' instead of FortranCompiler" - in str(err.value)) + assert ("Unexpected tool 'mock_fortran_compiler' of category " + "'C_COMPILER' instead of FortranCompiler" in str(err.value)) class TestCompilePass: diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index b9b7c808..c31e480e 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -14,17 +14,17 @@ import pytest -from fab.tools import (Category, CCompiler, FortranCompiler, - Gcc, Gfortran, Icc, Ifort, MpiGcc, MpiGfortran, - MpiIcc, MpiIfort) +from fab.tools import (Category, CCompiler, Compiler, FortranCompiler, + Gcc, Gfortran, Icc, Ifort) def test_compiler(): '''Test the compiler constructor.''' - cc = CCompiler("gcc", "gcc", "gnu", openmp_flag="-fopenmp") + cc = Compiler("gcc", "gcc", "gnu", category=Category.C_COMPILER, openmp_flag="-fopenmp") assert cc.category == Category.C_COMPILER assert cc._compile_flag == "-c" assert cc._output_flag == "-o" + # pylint: disable-next=use-implicit-booleaness-not-comparison assert cc.flags == [] assert cc.suite == "gnu" assert not cc.mpi @@ -40,6 +40,7 @@ def test_compiler(): assert fc._output_flag == "-o" assert fc.category == Category.FORTRAN_COMPILER assert fc.suite == "gnu" + # pylint: disable-next=use-implicit-booleaness-not-comparison assert fc.flags == [] assert not fc.mpi assert fc.openmp_flag == "-fopenmp" @@ -236,6 +237,7 @@ def test_get_version_string(): '''Tests the get_version_string() method. ''' full_output = 'GNU Fortran (gcc) 6.1.0' + c = Gfortran() with mock.patch.object(c, "run", mock.Mock(return_value=full_output)): assert c.get_version_string() == "6.1.0" @@ -401,19 +403,9 @@ def test_gcc(): assert not gcc.mpi -def test_mpi_gcc(): - '''Tests the MPI enables gcc class.''' - mpi_gcc = MpiGcc() - assert mpi_gcc.name == "mpicc-gcc" - assert isinstance(mpi_gcc, CCompiler) - assert mpi_gcc.category == Category.C_COMPILER - assert mpi_gcc.mpi - - -@pytest.mark.parametrize("compiler", [Gcc, MpiGcc]) -def test_gcc_get_version(compiler): +def test_gcc_get_version(): '''Tests the gcc class get_version method.''' - gcc = compiler() + gcc = Gcc() full_output = dedent(""" gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-20) Copyright (C) 2018 Free Software Foundation, Inc. @@ -422,10 +414,9 @@ def test_gcc_get_version(compiler): assert gcc.get_version() == (8, 5, 0) -@pytest.mark.parametrize("compiler", [Gcc, MpiGcc]) -def test_gcc_get_version_with_icc_string(compiler): +def test_gcc_get_version_with_icc_string(): '''Tests the gcc class with an icc version output.''' - gcc = compiler() + gcc = Gcc() full_output = dedent(""" icc (ICC) 2021.10.0 20230609 Copyright (C) 1985-2023 Intel Corporation. All rights reserved. @@ -447,22 +438,12 @@ def test_gfortran(): assert not gfortran.mpi -def test_mpi_gfortran(): - '''Tests the MPI enabled gfortran class.''' - mpi_gfortran = MpiGfortran() - assert mpi_gfortran.name == "mpif90-gfortran" - assert isinstance(mpi_gfortran, FortranCompiler) - assert mpi_gfortran.category == Category.FORTRAN_COMPILER - assert mpi_gfortran.mpi - - # Possibly overkill to cover so many gfortran versions but I had to go # check them so might as well add them. # Note: different sources, e.g conda, change the output slightly... -@pytest.mark.parametrize("compiler", [Gfortran, MpiGfortran]) -def test_gfortran_get_version_4(compiler): +def test_gfortran_get_version_4(): '''Test gfortran 4.8.5 version detection.''' full_output = dedent(""" GNU Fortran (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44) @@ -474,13 +455,12 @@ def test_gfortran_get_version_4(compiler): For more information about these matters, see the file named COPYING """) - gfortran = compiler() + gfortran = Gfortran() with mock.patch.object(gfortran, "run", mock.Mock(return_value=full_output)): assert gfortran.get_version() == (4, 8, 5) -@pytest.mark.parametrize("compiler", [Gfortran, MpiGfortran]) -def test_gfortran_get_version_6(compiler): +def test_gfortran_get_version_6(): '''Test gfortran 6.1.0 version detection.''' full_output = dedent(""" GNU Fortran (GCC) 6.1.0 @@ -489,13 +469,12 @@ def test_gfortran_get_version_6(compiler): warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. """) - gfortran = compiler() + gfortran = Gfortran() with mock.patch.object(gfortran, "run", mock.Mock(return_value=full_output)): assert gfortran.get_version() == (6, 1, 0) -@pytest.mark.parametrize("compiler", [Gfortran, MpiGfortran]) -def test_gfortran_get_version_8(compiler): +def test_gfortran_get_version_8(): '''Test gfortran 8.5.0 version detection.''' full_output = dedent(""" GNU Fortran (conda-forge gcc 8.5.0-16) 8.5.0 @@ -504,13 +483,12 @@ def test_gfortran_get_version_8(compiler): warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. """) - gfortran = compiler() + gfortran = Gfortran() with mock.patch.object(gfortran, "run", mock.Mock(return_value=full_output)): assert gfortran.get_version() == (8, 5, 0) -@pytest.mark.parametrize("compiler", [Gfortran, MpiGfortran]) -def test_gfortran_get_version_10(compiler): +def test_gfortran_get_version_10(): '''Test gfortran 10.4.0 version detection.''' full_output = dedent(""" GNU Fortran (conda-forge gcc 10.4.0-16) 10.4.0 @@ -519,13 +497,12 @@ def test_gfortran_get_version_10(compiler): warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. """) - gfortran = compiler() + gfortran = Gfortran() with mock.patch.object(gfortran, "run", mock.Mock(return_value=full_output)): assert gfortran.get_version() == (10, 4, 0) -@pytest.mark.parametrize("compiler", [Gfortran, MpiGfortran]) -def test_gfortran_get_version_12(compiler): +def test_gfortran_get_version_12(): '''Test gfortran 12.1.0 version detection.''' full_output = dedent(""" GNU Fortran (conda-forge gcc 12.1.0-16) 12.1.0 @@ -534,20 +511,19 @@ def test_gfortran_get_version_12(compiler): warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. """) - gfortran = compiler() + gfortran = Gfortran() with mock.patch.object(gfortran, "run", mock.Mock(return_value=full_output)): assert gfortran.get_version() == (12, 1, 0) -@pytest.mark.parametrize("compiler", [Gfortran, MpiGfortran]) -def test_gfortran_get_version_with_ifort_string(compiler): +def test_gfortran_get_version_with_ifort_string(): '''Tests the gfortran class with an ifort version output.''' full_output = dedent(""" ifort (IFORT) 14.0.3 20140422 Copyright (C) 1985-2014 Intel Corporation. All rights reserved. """) - gfortran = compiler() + gfortran = Gfortran() with mock.patch.object(gfortran, "run", mock.Mock(return_value=full_output)): with pytest.raises(RuntimeError) as err: gfortran.get_version() @@ -564,36 +540,25 @@ def test_icc(): assert not icc.mpi -def test_mpi_icc(): - '''Tests the MPI enabled icc class.''' - mpi_icc = MpiIcc() - assert mpi_icc.name == "mpicc-icc" - assert isinstance(mpi_icc, CCompiler) - assert mpi_icc.category == Category.C_COMPILER - assert mpi_icc.mpi - - -@pytest.mark.parametrize("compiler", [Icc, MpiIcc]) -def test_icc_get_version(compiler): +def test_icc_get_version(): '''Tests the icc class get_version method.''' full_output = dedent(""" icc (ICC) 2021.10.0 20230609 Copyright (C) 1985-2023 Intel Corporation. All rights reserved. """) - icc = compiler() + icc = Icc() with mock.patch.object(icc, "run", mock.Mock(return_value=full_output)): assert icc.get_version() == (2021, 10, 0) -@pytest.mark.parametrize("compiler", [Icc, MpiIcc]) -def test_icc_get_version_with_gcc_string(compiler): +def test_icc_get_version_with_gcc_string(): '''Tests the icc class with a GCC version output.''' full_output = dedent(""" gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-20) Copyright (C) 2018 Free Software Foundation, Inc. """) - icc = compiler() + icc = Icc() with mock.patch.object(icc, "run", mock.Mock(return_value=full_output)): with pytest.raises(RuntimeError) as err: icc.get_version() @@ -610,118 +575,82 @@ def test_ifort(): assert not ifort.mpi -def test_mpi_ifort(): - '''Tests the MPI enabled ifort class.''' - mpi_ifort = MpiIfort() - assert mpi_ifort.name == "mpif90-ifort" - assert isinstance(mpi_ifort, FortranCompiler) - assert mpi_ifort.category == Category.FORTRAN_COMPILER - assert mpi_ifort.mpi - - -@pytest.mark.parametrize("compiler", [Ifort, MpiIfort]) -def test_ifort_get_version_14(compiler): +def test_ifort_get_version_14(): '''Test ifort 14.0.3 version detection.''' full_output = dedent(""" ifort (IFORT) 14.0.3 20140422 Copyright (C) 1985-2014 Intel Corporation. All rights reserved. """) - ifort = compiler() + ifort = Ifort() with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): assert ifort.get_version() == (14, 0, 3) -@pytest.mark.parametrize("compiler", [Ifort, MpiIfort]) -def test_ifort_get_version_15(compiler): +def test_ifort_get_version_15(): '''Test ifort 15.0.2 version detection.''' full_output = dedent(""" ifort (IFORT) 15.0.2 20150121 Copyright (C) 1985-2015 Intel Corporation. All rights reserved. """) - ifort = compiler() + ifort = Ifort() with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): assert ifort.get_version() == (15, 0, 2) -@pytest.mark.parametrize("compiler", [Ifort, MpiIfort]) -def test_ifort_get_version_17(compiler): +def test_ifort_get_version_17(): '''Test ifort 17.0.7 version detection.''' full_output = dedent(""" ifort (IFORT) 17.0.7 20180403 Copyright (C) 1985-2018 Intel Corporation. All rights reserved. """) - ifort = compiler() + ifort = Ifort() with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): assert ifort.get_version() == (17, 0, 7) -@pytest.mark.parametrize("compiler", [Ifort, MpiIfort]) -def test_ifort_get_version_19(compiler): +def test_ifort_get_version_19(): '''Test ifort 19.0.0.117 version detection.''' full_output = dedent(""" ifort (IFORT) 19.0.0.117 20180804 Copyright (C) 1985-2018 Intel Corporation. All rights reserved. """) - ifort = compiler() + ifort = Ifort() with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): assert ifort.get_version() == (19, 0, 0, 117) -@pytest.mark.parametrize("compiler", [Ifort, MpiIfort]) -def test_ifort_get_version_with_icc_string(compiler): +def test_ifort_get_version_with_icc_string(): '''Tests the ifort class with an icc version output.''' full_output = dedent(""" icc (ICC) 2021.10.0 20230609 Copyright (C) 1985-2023 Intel Corporation. All rights reserved. """) - ifort = compiler() + ifort = Ifort() with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): with pytest.raises(RuntimeError) as err: ifort.get_version() assert "Unexpected version output format for compiler" in str(err.value) -@pytest.mark.parametrize("compiler", [Ifort, MpiIfort]) @pytest.mark.parametrize("version", ["5.15f.2", ".0.5.1", "0.5.1.", "0.5..1"]) -def test_ifort_get_version_invalid_version(compiler, version): - '''Tests the icc class with an icc version string that contains an invalid - version number.''' +def test_ifort_get_version_invalid_version(version): + '''Tests the ifort class with an ifort version string that contains an + invalid version number.''' full_output = dedent(f""" - icc (ICC) {version} 20230609 - Copyright (C) 1985-2023 Intel Corporation. All rights reserved. + ifort (IFORT) {version} 20140422 + Copyright (C) 1985-2014 Intel Corporation. All rights reserved. """) - icc = compiler() - with mock.patch.object(icc, "run", mock.Mock(return_value=full_output)): + ifort = Ifort() + with mock.patch.object(ifort, "run", mock.Mock(return_value=full_output)): with pytest.raises(RuntimeError) as err: - icc.get_version() + ifort.get_version() assert "Unexpected version output format for compiler" in str(err.value) - - -# ============================================================================ -def test_compiler_wrapper(): - '''Make sure we can easily create a compiler wrapper.''' - class MpiF90(Ifort): - '''A simple compiler wrapper''' - def __init__(self): - super().__init__(name="mpif90-intel", - exec_name="mpif90") - - @property - def mpi(self): - return True - - mpif90 = MpiF90() - assert mpif90.suite == "intel-classic" - assert mpif90.category == Category.FORTRAN_COMPILER - assert mpif90.name == "mpif90-intel" - assert mpif90.exec_name == "mpif90" - assert mpif90.mpi diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py new file mode 100644 index 00000000..93b26f14 --- /dev/null +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -0,0 +1,348 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## + +'''Tests the compiler wrapper implementation. +''' + +from pathlib import Path +from unittest import mock + +import pytest + +from fab.tools import (Category, CompilerWrapper, Gcc, Gfortran, Icc, Ifort, + Mpicc, Mpif90, ToolRepository) + + +def test_compiler_wrapper_compiler_getter(): + '''Tests that the compiler wrapper getter returns the + wrapper compiler instance. + ''' + gcc = Gcc() + mpicc = Mpicc(gcc) + assert mpicc.compiler is gcc + + +def test_compiler_wrapper_version_and_caching(): + '''Tests that the compiler wrapper reports the right version number + from the actual compiler. + ''' + mpicc = Mpicc(Gcc()) + + # The wrapper should report the version of the wrapped compiler: + with (mock.patch('fab.tools.compiler.Compiler.get_version', + return_value=(123,))): + assert mpicc.get_version() == (123,) + + # Test that the value is cached: + assert mpicc.get_version() == (123,) + + +def test_compiler_wrapper_version_consistency(): + '''Tests that the compiler wrapper and compiler must report the + same version number: + ''' + + # The wrapper must verify that the wrapper compiler and wrapper + # report the same version number, otherwise raise an exception. + # The first patch changes the return value which the compiler wrapper + # will report (since it calls Compiler.get_version), the second + # changes the return value of the wrapper compiler instance only: + + mpicc = Mpicc(Gcc()) + with mock.patch('fab.tools.compiler.Compiler.run_version_command', + return_value="gcc (GCC) 8.6.0 20210514 (Red Hat " + "8.5.0-20)"): + with mock.patch.object(mpicc.compiler, 'run_version_command', + return_value="gcc (GCC) 8.5.0 20210514 (Red " + "Hat 8.5.0-20)"): + with pytest.raises(RuntimeError) as err: + mpicc.get_version() + assert ("Different version for compiler 'Gcc - gcc: gcc' (8.5.0) " + "and compiler wrapper 'Mpicc(gcc)' (8.6.0)" in + str(err.value)) + + +def test_compiler_wrapper_version_compiler_unavailable(): + '''Checks the behaviour if the wrapped compiler is not available. + The wrapper should then report an empty result. + ''' + + mpicc = Mpicc(Gcc()) + with mock.patch.object(mpicc.compiler, '_is_available', False): + with pytest.raises(RuntimeError) as err: + assert mpicc.get_version() == "" + assert "Cannot get version of wrapped compiler" in str(err.value) + + +def test_compiler_is_available_ok(): + '''Check if check_available works as expected. + ''' + mpicc = Mpicc(Gcc()) + + # Just make sure we get the right object: + assert isinstance(mpicc, CompilerWrapper) + assert mpicc._is_available is None + + # Make sure that the compiler-wrapper itself reports that it is available: + # even if mpicc is not installed: + with mock.patch('fab.tools.compiler_wrapper.CompilerWrapper.' + 'check_available', return_value=True) as check_available: + assert mpicc.is_available + assert mpicc.is_available + # Due to caching there should only be one call to check_avail + check_available.assert_called_once_with() + + # Test that the value is indeed cached: + assert mpicc._is_available + + +def test_compiler_is_available_no_version(): + '''Make sure a compiler that does not return a valid version + is marked as not available. + ''' + mpicc = Mpicc(Gcc()) + # Now test if get_version raises an error + with mock.patch.object(mpicc.compiler, "get_version", + side_effect=RuntimeError("")): + assert not mpicc.is_available + + +def test_compiler_hash(): + '''Test the hash functionality.''' + mpicc = ToolRepository().get_tool(Category.C_COMPILER, + "mpicc-gcc") + with mock.patch.object(mpicc, "_version", (567,)): + hash1 = mpicc.get_hash() + assert hash1 == 4702012005 + + # A change in the version number must change the hash: + with mock.patch.object(mpicc, "_version", (89,)): + hash2 = mpicc.get_hash() + assert hash2 != hash1 + + # A change in the name with the original version number + # 567) must change the hash again: + with mock.patch.object(mpicc, "_name", "new_name"): + with mock.patch.object(mpicc, "_version", (567,)): + hash3 = mpicc.get_hash() + assert hash3 not in (hash1, hash2) + + # A change in the name with the modified version number + # must change the hash again: + with mock.patch.object(mpicc, "_name", "new_name"): + with mock.patch.object(mpicc, "_version", (89,)): + hash4 = mpicc.get_hash() + assert hash4 not in (hash1, hash2, hash3) + + +def test_compiler_wrapper_syntax_only(): + '''Tests handling of syntax only flags in wrapper. In case of testing + syntax only for a C compiler an exception must be raised.''' + mpif90 = ToolRepository().get_tool(Category.FORTRAN_COMPILER, + "mpif90-gfortran") + assert mpif90.has_syntax_only + + mpicc = ToolRepository().get_tool(Category.C_COMPILER, "mpicc-gcc") + with pytest.raises(RuntimeError) as err: + _ = mpicc.has_syntax_only + assert "'gcc' has no has_syntax_only" in str(err.value) + + +def test_compiler_wrapper_module_output(): + '''Tests handling of module output_flags in a wrapper. In case of testing + this with a C compiler, an exception must be raised.''' + mpif90 = ToolRepository().get_tool(Category.FORTRAN_COMPILER, + "mpif90-gfortran") + mpif90.set_module_output_path("/somewhere") + assert mpif90.compiler._module_output_path == "/somewhere" + + mpicc = ToolRepository().get_tool(Category.C_COMPILER, "mpicc-gcc") + with pytest.raises(RuntimeError) as err: + mpicc.set_module_output_path("/tmp") + assert "'gcc' has no 'set_module_output_path' function" in str(err.value) + + +def test_compiler_wrapper_fortran_with_add_args(): + '''Tests that additional arguments are handled as expected in + a wrapper.''' + mpif90 = ToolRepository().get_tool(Category.FORTRAN_COMPILER, + "mpif90-gfortran") + mpif90.set_module_output_path("/module_out") + with mock.patch.object(mpif90.compiler, "run", mock.MagicMock()): + with pytest.warns(UserWarning, match="Removing managed flag"): + mpif90.compile_file(Path("a.f90"), "a.o", + add_flags=["-J/b", "-O3"], openmp=False, + syntax_only=True) + # Notice that "-J/b" has been removed + mpif90.compiler.run.assert_called_with( + cwd=Path('.'), additional_parameters=['-c', "-O3", + '-fsyntax-only', + '-J', '/module_out', + 'a.f90', '-o', 'a.o']) + + +def test_compiler_wrapper_fortran_with_add_args_unnecessary_openmp(): + '''Tests that additional arguments are handled as expected in + a wrapper if also the openmp flags are specified.''' + mpif90 = ToolRepository().get_tool(Category.FORTRAN_COMPILER, + "mpif90-gfortran") + mpif90.set_module_output_path("/module_out") + with mock.patch.object(mpif90.compiler, "run", mock.MagicMock()): + with pytest.warns(UserWarning, + match="explicitly provided. OpenMP should be " + "enabled in the BuildConfiguration"): + mpif90.compile_file(Path("a.f90"), "a.o", + add_flags=["-fopenmp", "-O3"], + openmp=True, syntax_only=True) + mpif90.compiler.run.assert_called_with( + cwd=Path('.'), + additional_parameters=['-c', '-fopenmp', '-fopenmp', '-O3', + '-fsyntax-only', '-J', '/module_out', + 'a.f90', '-o', 'a.o']) + + +def test_compiler_wrapper_c_with_add_args(): + '''Tests that additional arguments are handled as expected in a + compiler wrapper. Also verify that requesting Fortran-specific options + like syntax-only with the C compiler raises a runtime error. + ''' + + mpicc = ToolRepository().get_tool(Category.C_COMPILER, + "mpicc-gcc") + with mock.patch.object(mpicc.compiler, "run", mock.MagicMock()): + # Normal invoke of the C compiler, make sure add_flags are + # passed through: + mpicc.compile_file(Path("a.f90"), "a.o", openmp=False, + add_flags=["-O3"]) + mpicc.compiler.run.assert_called_with( + cwd=Path('.'), additional_parameters=['-c', "-O3", 'a.f90', + '-o', 'a.o']) + # Invoke C compiler with syntax-only flag (which is only supported + # by Fortran compilers), which should raise an exception. + with pytest.raises(RuntimeError) as err: + mpicc.compile_file(Path("a.f90"), "a.o", openmp=False, + add_flags=["-O3"], syntax_only=True) + assert ("Syntax-only cannot be used with compiler 'mpicc-gcc'." + in str(err.value)) + + # Check that providing the openmp flag in add_flag raises a warning: + with mock.patch.object(mpicc.compiler, "run", mock.MagicMock()): + with pytest.warns(UserWarning, + match="explicitly provided. OpenMP should be " + "enabled in the BuildConfiguration"): + mpicc.compile_file(Path("a.f90"), "a.o", + add_flags=["-fopenmp", "-O3"], + openmp=True) + mpicc.compiler.run.assert_called_with( + cwd=Path('.'), + additional_parameters=['-c', '-fopenmp', '-fopenmp', '-O3', + 'a.f90', '-o', 'a.o']) + + +def test_compiler_wrapper_flags_independent(): + '''Tests that flags set in the base compiler will be accessed in the + wrapper, but not the other way round.''' + gcc = Gcc() + mpicc = Mpicc(gcc) + # pylint: disable=use-implicit-booleaness-not-comparison + assert gcc.flags == [] + assert mpicc.flags == [] + # Setting flags in gcc must become visible in the wrapper compiler: + gcc.add_flags(["-a", "-b"]) + assert gcc.flags == ["-a", "-b"] + assert mpicc.flags == ["-a", "-b"] + assert mpicc.openmp_flag == gcc.openmp_flag + + # Adding flags to the wrapper should not affect the wrapped compiler: + mpicc.add_flags(["-d", "-e"]) + assert gcc.flags == ["-a", "-b"] + # And the compiler wrapper should reports the wrapped compiler's flag + # followed by the wrapper flag (i.e. the wrapper flag can therefore + # overwrite the wrapped compiler's flags) + assert mpicc.flags == ["-a", "-b", "-d", "-e"] + + +def test_compiler_wrapper_flags_with_add_arg(): + '''Tests that flags set in the base compiler will be accessed in the + wrapper if also additional flags are specified.''' + gcc = Gcc() + mpicc = Mpicc(gcc) + gcc.add_flags(["-a", "-b"]) + mpicc.add_flags(["-d", "-e"]) + + # Check that the flags are assembled in the right order in the + # actual compiler call: first the wrapper compiler flag, then + # the wrapper flag, then additional flags + with mock.patch.object(mpicc.compiler, "run", mock.MagicMock()): + mpicc.compile_file(Path("a.f90"), "a.o", add_flags=["-f"], + openmp=True) + mpicc.compiler.run.assert_called_with( + cwd=Path('.'), + additional_parameters=["-c", "-fopenmp", "-a", "-b", "-d", + "-e", "-f", "a.f90", "-o", "a.o"]) + + +def test_compiler_wrapper_flags_without_add_arg(): + '''Tests that flags set in the base compiler will be accessed in the + wrapper if no additional flags are specified.''' + gcc = Gcc() + mpicc = Mpicc(gcc) + gcc.add_flags(["-a", "-b"]) + mpicc.add_flags(["-d", "-e"]) + # Check that the flags are assembled in the right order in the + # actual compiler call: first the wrapper compiler flag, then + # the wrapper flag, then additional flags + with mock.patch.object(mpicc.compiler, "run", mock.MagicMock()): + # Test if no add_flags are specified: + mpicc.compile_file(Path("a.f90"), "a.o", openmp=True) + mpicc.compiler.run.assert_called_with( + cwd=Path('.'), + additional_parameters=["-c", "-fopenmp", "-a", "-b", "-d", + "-e", "a.f90", "-o", "a.o"]) + + +def test_compiler_wrapper_mpi_gcc(): + '''Tests the MPI enables gcc class.''' + mpi_gcc = Mpicc(Gcc()) + assert mpi_gcc.name == "mpicc-gcc" + assert str(mpi_gcc) == "Mpicc(gcc)" + assert isinstance(mpi_gcc, CompilerWrapper) + assert mpi_gcc.category == Category.C_COMPILER + assert mpi_gcc.mpi + assert mpi_gcc.suite == "gnu" + + +def test_compiler_wrapper_mpi_gfortran(): + '''Tests the MPI enabled gfortran class.''' + mpi_gfortran = Mpif90(Gfortran()) + assert mpi_gfortran.name == "mpif90-gfortran" + assert str(mpi_gfortran) == "Mpif90(gfortran)" + assert isinstance(mpi_gfortran, CompilerWrapper) + assert mpi_gfortran.category == Category.FORTRAN_COMPILER + assert mpi_gfortran.mpi + assert mpi_gfortran.suite == "gnu" + + +def test_compiler_wrapper_mpi_icc(): + '''Tests the MPI enabled icc class.''' + mpi_icc = Mpicc(Icc()) + assert mpi_icc.name == "mpicc-icc" + assert str(mpi_icc) == "Mpicc(icc)" + assert isinstance(mpi_icc, CompilerWrapper) + assert mpi_icc.category == Category.C_COMPILER + assert mpi_icc.mpi + assert mpi_icc.suite == "intel-classic" + + +def test_compiler_wrapper_mpi_ifort(): + '''Tests the MPI enabled ifort class.''' + mpi_ifort = Mpif90(Ifort()) + assert mpi_ifort.name == "mpif90-ifort" + assert str(mpi_ifort) == "Mpif90(ifort)" + assert isinstance(mpi_ifort, CompilerWrapper) + assert mpi_ifort.category == Category.FORTRAN_COMPILER + assert mpi_ifort.mpi + assert mpi_ifort.suite == "intel-classic" diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index b51c691c..0ff97ca2 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -16,16 +16,30 @@ def test_flags_constructor(): '''Tests the constructor of Flags.''' f1 = Flags() assert isinstance(f1, list) + + # pylint: disable-next=use-implicit-booleaness-not-comparison assert f1 == [] f2 = Flags(["a"]) assert isinstance(f2, list) assert f2 == ["a"] +def test_flags_adding(): + '''Tests adding flags.''' + f1 = Flags() + # pylint: disable-next=use-implicit-booleaness-not-comparison + assert f1 == [] + f1.add_flags("-a") + assert f1 == ["-a"] + f1.add_flags(["-b", "-c"]) + assert f1 == ["-a", "-b", "-c"] + + def test_remove_flags(): '''Test remove_flags functionality.''' flags = Flags() flags.remove_flag("-c", False) + # pylint: disable-next=use-implicit-booleaness-not-comparison assert flags == [] all_flags = ['a.f90', '-c', '-o', 'a.o', '-fsyntax-only', "-J", "/tmp"] diff --git a/tests/unit_tests/tools/test_psyclone.py b/tests/unit_tests/tools/test_psyclone.py index ad7817c2..7efc60ec 100644 --- a/tests/unit_tests/tools/test_psyclone.py +++ b/tests/unit_tests/tools/test_psyclone.py @@ -7,6 +7,7 @@ '''Tests the PSyclone implementation. ''' +from importlib import reload from unittest import mock from fab.tools import (Category, Psyclone) @@ -121,3 +122,16 @@ def test_psyclone_process(psyclone_lfric_api): 'psy_file', '-oalg', 'alg_file', '-s', 'script_called', '-c', 'psyclone.cfg', '-d', 'root1', '-d', 'root2', 'x90_file'], capture_output=True, env=None, cwd=None, check=False) + + +def test_type_checking_import(): + '''PSyclone contains an import of TYPE_CHECKING to break a circular + dependency. In order to reach 100% coverage of PSyclone, we set + mock TYPE_CHECKING to be true and force a re-import of the module. + TODO 314: This test can be removed once #314 is fixed. + ''' + with mock.patch('typing.TYPE_CHECKING', True): + # This import will not actually re-import, since the module + # is already imported. But we need this in order to call reload: + import fab.tools.psyclone + reload(fab.tools.psyclone) diff --git a/tests/unit_tests/tools/test_tool.py b/tests/unit_tests/tools/test_tool.py index dd892831..39d18bd8 100644 --- a/tests/unit_tests/tools/test_tool.py +++ b/tests/unit_tests/tools/test_tool.py @@ -50,15 +50,21 @@ def test_tool_constructor(): assert misc.category == Category.MISC +def test_tool_chance_exec_name(): + '''Test that we can change the name of the executable. + ''' + tool = Tool("gfortran", "gfortran", Category.FORTRAN_COMPILER) + assert tool.exec_name == "gfortran" + tool.change_exec_name("start_me_instead") + assert tool.exec_name == "start_me_instead" + + def test_tool_is_available(): '''Test that is_available works as expected.''' tool = Tool("gfortran", "gfortran", Category.FORTRAN_COMPILER) with mock.patch.object(tool, "check_available", return_value=True): assert tool.is_available - # Test the getter tool._is_available = False - assert not tool.is_available - assert tool.is_compiler # Test the exception when trying to use in a non-existent tool: with pytest.raises(RuntimeError) as err: @@ -66,6 +72,22 @@ def test_tool_is_available(): assert ("Tool 'gfortran' is not available to run '['gfortran', '--ops']'" in str(err.value)) + # Test setting the option and the getter + tool = Tool("gfortran", "gfortran", Category.FORTRAN_COMPILER, + availability_option="am_i_here") + assert tool.availability_option == "am_i_here" + + +def test_tool_flags(): + '''Test that flags work as expected''' + tool = Tool("gfortran", "gfortran", Category.FORTRAN_COMPILER) + # pylint: disable-next=use-implicit-booleaness-not-comparison + assert tool.flags == [] + tool.add_flags("-a") + assert tool.flags == ["-a"] + tool.add_flags(["-b", "-c"]) + assert tool.flags == ["-a", "-b", "-c"] + class TestToolRun: '''Test the run method of Tool.''' From 224664c6ac4190b6649fef3372352a1a263b9456 Mon Sep 17 00:00:00 2001 From: Matthew Hambley Date: Sat, 18 Jan 2025 15:32:51 +0000 Subject: [PATCH 04/12] Update clang binding module. (#376) --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index bd6bdb74..ca443aec 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -19,7 +19,7 @@ classifiers = [ ] [project.optional-dependencies] -c-language = ['clang'] +c-language = ['libclang'] plots = ['matplotlib'] tests = ['pytest', 'pytest-cov', 'pytest-mock'] checks = ['flake8>=5.0.4', 'mypy'] From c585ff734a83b97cd945ff96468bcf56eb3ce46c Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 30 Jan 2025 10:18:52 +1100 Subject: [PATCH 05/12] Linker wrapper new (#375) * Support new and old style of PSyclone command line (no more nemo api etc) * Fix mypy errors. * Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa. * Updated comment. * Removed mixing, use a simple regex instead. * Added support for ifx/icx compiler as intel-llvm class. * Added support for nvidia compiler. * Add preliminary support for Cray compiler. * Added Cray compiler wrapper ftn and cc. * Follow a more consistent naming scheme for crays, even though the native compiler names are longer (crayftn-cray, craycc-cray). * Changed names again. * Renamed cray compiler wrapper to be CrayCcWrapper and CrayFtnWrapper, to avoid confusion with Craycc. * Fixed incorrect name in comments. * Additional compilers (#349) * Moved OBJECT_ARCHIVES from constants to ArtefactSet. * Moved PRAGMAD_C from constants to ArtefactSet. * Turned 'all_source' into an enum. * Allow integer as revision. * Fixed flake8 error. * Removed specific functions to add/get fortran source files etc. * Removed non-existing and unneccessary collections. * Try to fix all run_configs. * Fixed rebase issues. * Added replace functionality to ArtefactStore, updated test_artefacts to cover all lines in that file. * Started to replace artefacts when files are pre-processed. * Removed linker argument from linking step in all examples. * Try to get jules to link. * Fixed build_jules. * Fixed other issues raised in reviews. * Try to get jules to link. * Fixed other issues raised in reviews. * Simplify handling of X90 files by replacing the X90 with x90, meaning only one artefact set is involved when running PSyclone. * Make OBJECT_ARCHIVES also a dict, migrate more code to replace/add files to the default build artefact collections. * Fixed some examples. * Fix flake8 error. * Fixed failing tests. * Support empty comments. * Fix preprocessor to not unnecessary remove and add files that are already in the output directory. * Allow find_soure_files to be called more than once by adding files (not replacing artefact). * Updated lfric_common so that files created by configurator are written in build (not source). * Use c_build_files instead of pragmad_c. * Removed unnecessary str. * Documented the new artefact set handling. * Fixed typo. * Make the PSyclone API configurable. * Fixed formatting of documentation, properly used ArtefactSet names. * Support .f and .F Fortran files. * Removed setter for tool.is_available, which was only used for testing. * #3 Fix documentation and coding style issues from review. * Renamed Categories into Category. * Minor coding style cleanup. * Removed more unnecessary (). * Re-added (invalid) grab_pre_build call. * Fixed typo. * Renamed set_default_vendor to set_default_compiler_suite. * Renamed VendorTool to CompilerSuiteTool. * Also accept a Path as exec_name specification for a tool. * Move the check_available function into the base class. * Fixed some types and documentation. * Fix typing error. * Added explanation for meta-compiler. * Improved error handling and documentation. * Replace mpiifort with mpifort to be a tiny bit more portable. * Use classes to group tests for git/svn/fcm together. * Fixed issue in get_transformation script, and moved script into lfric_common to remove code duplication. * Code improvement as suggested by review. * Fixed run config * Added reference to ticket. * Updated type information. * More typing fixes. * Fixed typing warnings. * As requested by reviewer removed is_working_copy functionality. * Issue a warning (which can be silenced) when a tool in a toolbox is replaced. * Fixed flake8. * Fixed flake8. * Fixed failing test. * Addressed issues raised in review. * Removed now unnecessary operations. * Updated some type information. * Fixed all references to APIs to be consistent with PSyclone 2.5. * Added api to the checksum computation. * Fixed type information. * Added test to verify that changing the api changes the checksum. * Make compiler version a tuple of integers * Update some tests to use tuple versions * Explicitly test handling of bad version format * Fix formatting * Tidying up * Make compiler raise an error for any invalid version string Assume these compilers don't need to be hashed. Saves dealing with empty tuples. * Check compiler version string for compiler name * Fix formatting * Add compiler.get_version_string() method Includes other cleanup from PR comments * Add mpi and openmp settings to BuildConfig, made compiler MPI aware. * Looks like the circular dependency has been fixed. * Revert "Looks like the circular dependency has been fixed." ... while it works with the tests, a real application still triggered it. This reverts commit 150dc379af9df8c38e623fae144a0d5196319f10. * Don't even try to find a C compiler if no C files are to be compiled. * Updated gitignore to ignore (recently renamed) documentation. * Fixed failing test. * Return from compile Fortran early if there are no files to compiles. Fixed coding style. * Add MPI enables wrapper for intel and gnu compiler. * Fixed test. * Automatically add openmp flag to compiler and linker based on BuildConfig. * Removed enforcement of keyword parameters, which is not supported in python 3.7. * Fixed failing test. * Support more than one tool of a given suite by sorting them. * Use different version checkout for each compiler vendor with mixins * Refactoring, remove unittest compiler class * Fix some mypy errors * Use 'Union' type hint to fix build checks * Added option to add flags to a tool. * Introduce proper compiler wrapper, used this to implement properly wrapper MPI compiler. * Fixed typo in types. * Return run_version_command to base Compiler class Provides default version command that can be overridden for other compilers. Also fix some incorrect tests Other tidying * Add a missing type hint * Added (somewhat stupid) 'test' to reach 100% coverage of PSyclone tool. * Simplified MPI support in wrapper. * More compiler wrapper coverage. * Removed duplicated function. * Removed debug print. * Removed permanently changing compiler attributes, which can cause test failures later. * More test for C compiler wrapper. * More work on compiler wrapper tests. * Fixed version and availability handling, added missing tests for 100% coverage. * Fixed typing error. * Try to fix python 3.7. * Tried to fix failing tests. * Remove inheritance from mixins and use protocol * Simplify compiler inheritance Mixins have static methods with unique names, overrides only happen in concrete classes * Updated wrapper and tests to handle error raised in get_version. * Simplified regular expressions (now tests cover detection of version numbers with only a major version). * Test for missing mixin. * Use the parsing mixing from the compiler in a compiler wrapper. * Use setattr instead of assignment to make mypy happy. * Simplify usage of compiler-specific parsing mixins. * Minor code cleanup. * Updated documentation. * Simplify usage of compiler-specific parsing mixins. * Test for missing mixin. * Fixed test. * Added missing openmp_flag property to compiler wrapper. * Don't use isinstance for consistency check, which does not work for CompilerWrappers. * Fixed isinstance test for C compilation which doesn't work with a CompilerWrapper. * Use a linker's compiler to determine MPI support. Removed mpi property from CompilerSuite. * Added more tests for invalid version numbers. * Added more test cases for invalid version number, improved regex to work as expected. * Fixed typo in test. * Fixed flake/mypy errors. * Combine wrapper flags with flags from wrapped compiler. * Made mypy happy. * Fixed test. * Split tests into smaller individual ones, fixed missing asssert in test. * Parameterised compiler version tests to also test wrapper. * Added missing MPI parameter when getting the compiler. * Fixed comments. * Order parameters to be in same order for various compiler classes. * Remove stray character * Added getter for wrapped compiler. * Fixed small error that would prevent nested compiler wrappers from being used. * Added a cast to make mypy happy. * Add simple getter for linker library flags * Add getter for linker flags by library * Fix formatting * Add optional libs argument to link function * Reorder and clean up linker tests * Make sure `Linker.link()` raises for unknown lib * Add missing type * Fix typing error * Add 'libs' argument to link_exe function * Try to add documentation for the linker libs feature * Use correct list type in link_exe hint * Add silent replace option to linker.add_lib_flags * Fixed spelling mistake in option. * Clarified documentation. * Removed unnecessary functions in CompilerWrapper. * Fixed failing test triggered by executing them in specific order (tools then steps) * Fixed line lengths. * Add tests for linker LDFLAG * Add pre- and post- lib flags to link function * Fix syntax in built-in lib flags * Remove netcdf as a built-in linker library Bash-style substitution is not currently handled * Configure pre- and post-lib flags on the Linker object Previously they were passed into the Linker.link() function * Use more realistic linker lib flags * Formatting fix * Removed mixing, use a simple regex instead. * Added support for ifx/icx compiler as intel-llvm class. * Added support for nvidia compiler. * Add preliminary support for Cray compiler. * Added Cray compiler wrapper ftn and cc. * Made mpi and openmp default to False in the BuildConfig constructor. * Removed white space. * Follow a more consistent naming scheme for crays, even though the native compiler names are longer (crayftn-cray, craycc-cray). * Changed names again. * Support compilers that do not support OpenMP. * Added documentation for openmp parameter. * Renamed cray compiler wrapper to be CrayCcWrapper and CrayFtnWrapper, to avoid confusion with Craycc. * Fixed incorrect name in comments. --------- Co-authored-by: jasonjunweilyu <161689601+jasonjunweilyu@users.noreply.github.com> Co-authored-by: Luke Hoffmann Co-authored-by: Luke Hoffmann <992315+lukehoffmann@users.noreply.github.com> * Support new and old style of PSyclone command line (no more nemo api etc) * Fix mypy errors. * Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa. * Added shell tool. * Try to make mypy happy. * Removed debug code. * ToolRepository now only returns default that are available. Updated tests to make tools as available. * Fixed typos and coding style. * Support new and old style of PSyclone command line (no more nemo api etc) * Fix mypy errors. * Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa. * Updated comment. * Fixed failing tests. * Updated fparser dependency to version 0.2. * Replace old code for handling sentinels with triggering this behaviour in fparser. Require config in constructor of Analyser classes. * Fixed tests for latest changes. * Removed invalid openmp continuation line - since now fparser fails when trying to parse this line. * Added test for disabled openmp parsing. Updated test to work with new test file. * Coding style changes. * Fix flake issues. * Fixed double _. * Make Linker inherit CompilerWrapper * Fix up tests for new Linker inheritence * Fix a flake error * Use linker wrapping to combine flags from the wrapped linker with the linker wrapper. * Minor code cleanup. * Created linker wrapper in ToolRepository. * Try making linker a CompilerSuiteTool instead of a CompilerWrapper. * Updated tests. * Fix support for post-libs. * Fixed mypy. * Removed more accesses to private members. * Added missing type hint. * Make flake8 happy. * Added missing openmp handling in linker. * Addressed issues raised in review. * Forgot that file in previous commit. --------- Co-authored-by: Luke Hoffmann <992315+lukehoffmann@users.noreply.github.com> Co-authored-by: jasonjunweilyu <161689601+jasonjunweilyu@users.noreply.github.com> Co-authored-by: Luke Hoffmann --- source/fab/tools/compiler.py | 7 +- source/fab/tools/compiler_wrapper.py | 4 +- source/fab/tools/linker.py | 203 +++++++++++++----- source/fab/tools/preprocessor.py | 2 +- source/fab/tools/tool_repository.py | 35 ++- tests/conftest.py | 7 +- tests/unit_tests/steps/test_link.py | 38 +++- .../steps/test_link_shared_object.py | 22 +- tests/unit_tests/tools/test_compiler.py | 4 +- tests/unit_tests/tools/test_linker.py | 197 +++++++++++------ 10 files changed, 369 insertions(+), 150 deletions(-) diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 0b5618de..b937e9d1 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -64,7 +64,7 @@ def __init__(self, name: str, self._compile_flag = compile_flag if compile_flag else "-c" self._output_flag = output_flag if output_flag else "-o" self._openmp_flag = openmp_flag if openmp_flag else "" - self.flags.extend(os.getenv("FFLAGS", "").split()) + self.add_flags(os.getenv("FFLAGS", "").split()) self._version_regex = version_regex @property @@ -83,6 +83,11 @@ def openmp_flag(self) -> str: '''Returns the flag to enable OpenMP.''' return self._openmp_flag + @property + def output_flag(self) -> str: + '''Returns the flag that specifies the output flag.''' + return self._output_flag + def get_hash(self) -> int: ''':returns: a hash based on the compiler name and version. ''' diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index 09ce5015..9338f848 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -4,8 +4,8 @@ # which you should have received as part of this distribution ############################################################################## -"""This file contains the base class for any compiler, and derived -classes for gcc, gfortran, icc, ifort +"""This file contains the base class for any compiler-wrapper, including +the derived classes for mpif90, mpicc, and CrayFtnWrapper and CrayCcWrapper. """ from pathlib import Path diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 8959b3de..2acef01b 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -7,9 +7,11 @@ """This file contains the base class for any Linker. """ +from __future__ import annotations + import os from pathlib import Path -from typing import cast, Dict, List, Optional +from typing import cast, Dict, List, Optional, Union import warnings from fab.tools.category import Category @@ -18,39 +20,51 @@ class Linker(CompilerSuiteTool): - '''This is the base class for any Linker. If a compiler is specified, - its name, executable, and compile suite will be used for the linker (if - not explicitly set in the constructor). - - :param name: the name of the linker. - :param exec_name: the name of the executable. - :param suite: optional, the name of the suite. - :param compiler: optional, a compiler instance - :param output_flag: flag to use to specify the output name. + '''This is the base class for any Linker. It takes either another linker + instance, or a compiler instance as parameter in the constructor. Exactly + one of these must be provided. + + :param compiler: an optional compiler instance + :param linker: an optional linker instance + :param name: name of the linker + + :raises RuntimeError: if both compiler and linker are specified. + :raises RuntimeError: if neither compiler nor linker is specified. ''' - # pylint: disable=too-many-arguments - def __init__(self, name: Optional[str] = None, + def __init__(self, compiler: Optional[Compiler] = None, + linker: Optional[Linker] = None, exec_name: Optional[str] = None, - suite: Optional[str] = None, - compiler: Optional[Compiler] = None, - output_flag: str = "-o"): - if (not name or not exec_name or not suite) and not compiler: - raise RuntimeError("Either specify name, exec name, and suite " - "or a compiler when creating Linker.") - # Make mypy happy, since it can't work out otherwise if these string - # variables might still be None :( - compiler = cast(Compiler, compiler) + name: Optional[str] = None): + + if linker and compiler: + raise RuntimeError("Both compiler and linker is specified in " + "linker constructor.") + if not linker and not compiler: + raise RuntimeError("Neither compiler nor linker is specified in " + "linker constructor.") + self._compiler = compiler + self._linker = linker + + search_linker = self + while search_linker._linker: + search_linker = search_linker._linker + final_compiler = search_linker._compiler if not name: - name = compiler.name + assert final_compiler # make mypy happy + name = f"linker-{final_compiler.name}" + if not exec_name: - exec_name = compiler.exec_name - if not suite: - suite = compiler.suite - self._output_flag = output_flag - super().__init__(name, exec_name, suite, Category.LINKER) - self._compiler = compiler - self.flags.extend(os.getenv("LDFLAGS", "").split()) + # This will search for the name in linker or compiler + exec_name = self.get_exec_name() + + super().__init__( + name=name, + exec_name=exec_name, + suite=self.suite, + category=Category.LINKER) + + self.add_flags(os.getenv("LDFLAGS", "").split()) # Maintain a set of flags for common libraries. self._lib_flags: Dict[str, List[str]] = {} @@ -58,20 +72,55 @@ def __init__(self, name: Optional[str] = None, self._pre_lib_flags: List[str] = [] self._post_lib_flags: List[str] = [] - @property - def mpi(self) -> bool: - ''':returns: whether the linker supports MPI or not.''' - return self._compiler.mpi - def check_available(self) -> bool: - ''' - :returns: whether the linker is available or not. We do this - by requesting the linker version. + ''':returns: whether this linker is available by asking the wrapped + linker or compiler. ''' if self._compiler: return self._compiler.check_available() + assert self._linker # make mypy happy + return self._linker.check_available() + + def get_exec_name(self) -> str: + ''':returns: the name of the executable by asking the wrapped + linker or compiler.''' + if self._compiler: + return self._compiler.exec_name + assert self._linker # make mypy happy + return self._linker.exec_name + + @property + def suite(self) -> str: + ''':returns: the suite this linker belongs to by getting it from + the wrapper compiler or linker.''' + return cast(CompilerSuiteTool, (self._compiler or self._linker)).suite + + @property + def mpi(self) -> bool: + ''':returns" whether this linker supports MPI or not by checking + with the wrapper compiler or linker.''' + if self._compiler: + return self._compiler.mpi + assert self._linker # make mypy happy + return self._linker.mpi - return super().check_available() + @property + def openmp(self) -> bool: + ''':returns" whether this linker supports OpenMP or not by checking + with the wrapper compiler or linker.''' + if self._compiler: + return self._compiler.openmp + assert self._linker # make mypy happy + return self._linker.openmp + + @property + def output_flag(self) -> str: + ''':returns: the flag that is used to specify the output name. + ''' + if self._compiler: + return self._compiler.output_flag + assert self._linker # make mypy happy + return self._linker.output_flag def get_lib_flags(self, lib: str) -> List[str]: '''Gets the standard flags for a standard library @@ -85,6 +134,10 @@ def get_lib_flags(self, lib: str) -> List[str]: try: return self._lib_flags[lib] except KeyError: + # If a lib is not defined here, but this is a wrapper around + # another linker, return the result from the wrapped linker + if self._linker: + return self._linker.get_lib_flags(lib) raise RuntimeError(f"Unknown library name: '{lib}'") def add_lib_flags(self, lib: str, flags: List[str], @@ -128,6 +181,47 @@ def add_post_lib_flags(self, flags: List[str]): ''' self._post_lib_flags.extend(flags) + def get_pre_link_flags(self) -> List[str]: + '''Returns the list of pre-link flags. It will concatenate the + flags for this instance with all potentially wrapped linkers. + This wrapper's flag will come first - the assumption is that + the pre-link flags are likely paths, so we need a wrapper to + be able to put a search path before the paths from a wrapped + linker. + + :returns: List of pre-link flags of this linker and all + wrapped linkers + ''' + params: List[str] = [] + if self._pre_lib_flags: + params.extend(self._pre_lib_flags) + if self._linker: + # If we are wrapping a linker, get the wrapped linker's + # pre-link flags and append them to the end (so the linker + # wrapper's settings come before the setting from the + # wrapped linker). + params.extend(self._linker.get_pre_link_flags()) + return params + + def get_post_link_flags(self) -> List[str]: + '''Returns the list of post-link flags. It will concatenate the + flags for this instance with all potentially wrapped linkers. + This wrapper's flag will be added to the end. + + :returns: List of post-link flags of this linker and all + wrapped linkers + ''' + params: List[str] = [] + if self._linker: + # If we are wrapping a linker, get the wrapped linker's + # post-link flags and add them first (so this linker + # wrapper's settings come after the setting from the + # wrapped linker). + params.extend(self._linker.get_post_link_flags()) + if self._post_lib_flags: + params.extend(self._post_lib_flags) + return params + def link(self, input_files: List[Path], output_file: Path, openmp: bool, libs: Optional[List[str]] = None) -> str: @@ -141,21 +235,30 @@ def link(self, input_files: List[Path], output_file: Path, :returns: the stdout of the link command ''' - if self._compiler: - # Create a copy: - params = self._compiler.flags[:] - if openmp: - params.append(self._compiler.openmp_flag) - else: - params = [] + + params: List[Union[str, Path]] = [] + + # Find the compiler by following the (potentially + # layered) linker wrapper. + linker = self + while linker._linker: + linker = linker._linker + # Now we must have a compiler + compiler = linker._compiler + assert compiler # make mypy happy + params.extend(compiler.flags) + + if openmp: + params.append(compiler.openmp_flag) + # TODO: why are the .o files sorted? That shouldn't matter params.extend(sorted(map(str, input_files))) + params.extend(self.get_pre_link_flags()) - if self._pre_lib_flags: - params.extend(self._pre_lib_flags) for lib in (libs or []): params.extend(self.get_lib_flags(lib)) - if self._post_lib_flags: - params.extend(self._post_lib_flags) - params.extend([self._output_flag, str(output_file)]) + + params.extend(self.get_post_link_flags()) + params.extend([self.output_flag, str(output_file)]) + return self.run(params) diff --git a/source/fab/tools/preprocessor.py b/source/fab/tools/preprocessor.py index e620ce2a..dd037874 100644 --- a/source/fab/tools/preprocessor.py +++ b/source/fab/tools/preprocessor.py @@ -63,7 +63,7 @@ class CppFortran(Preprocessor): ''' def __init__(self): super().__init__("cpp", "cpp", Category.FORTRAN_PREPROCESSOR) - self.flags.extend(["-traditional-cpp", "-P"]) + self.add_flags(["-traditional-cpp", "-P"]) # ============================================================================ diff --git a/source/fab/tools/tool_repository.py b/source/fab/tools/tool_repository.py index 965e252b..1bf839f8 100644 --- a/source/fab/tools/tool_repository.py +++ b/source/fab/tools/tool_repository.py @@ -17,8 +17,8 @@ from fab.tools.tool import Tool from fab.tools.category import Category from fab.tools.compiler import Compiler -from fab.tools.compiler_wrapper import (CrayCcWrapper, CrayFtnWrapper, - Mpif90, Mpicc) +from fab.tools.compiler_wrapper import (CompilerWrapper, CrayCcWrapper, + CrayFtnWrapper, Mpif90, Mpicc) from fab.tools.linker import Linker from fab.tools.versioning import Fcm, Git, Subversion from fab.tools import (Ar, Cpp, CppFortran, Craycc, Crayftn, @@ -81,12 +81,12 @@ def __init__(self): # Now create the potential mpif90 and Cray ftn wrapper all_fc = self[Category.FORTRAN_COMPILER][:] for fc in all_fc: - mpif90 = Mpif90(fc) - self.add_tool(mpif90) + if not fc.mpi: + mpif90 = Mpif90(fc) + self.add_tool(mpif90) # I assume cray has (besides cray) only support for Intel and GNU if fc.name in ["gfortran", "ifort"]: crayftn = CrayFtnWrapper(fc) - print("NEW NAME", crayftn, crayftn.name) self.add_tool(crayftn) # Now create the potential mpicc and Cray cc wrapper @@ -114,9 +114,28 @@ def add_tool(self, tool: Tool): # If we have a compiler, add the compiler as linker as well if tool.is_compiler: - tool = cast(Compiler, tool) - linker = Linker(name=f"linker-{tool.name}", compiler=tool) - self[linker.category].append(linker) + compiler = cast(Compiler, tool) + if isinstance(compiler, CompilerWrapper): + # If we have a compiler wrapper, create a new linker using + # the linker based on the wrappped compiler. For example, when + # creating linker-mpif90-gfortran, we want this to be based on + # linker-gfortran (and not on the compiler mpif90-gfortran), + # since the linker-gfortran might have library definitions + # that should be reused. So we first get the existing linker + # (since the compiler exists, a linker for this compiler was + # already created and must exist). + other_linker = self.get_tool( + category=Category.LINKER, + name=f"linker-{compiler.compiler.name}") + other_linker = cast(Linker, other_linker) + linker = Linker(linker=other_linker, + exec_name=compiler.exec_name, + name=f"linker-{compiler.name}") + self[linker.category].append(linker) + else: + linker = Linker(compiler=compiler, + name=f"linker-{compiler.name}") + self[linker.category].append(linker) def get_tool(self, category: Category, name: str) -> Tool: ''':returns: the tool with a given name in the specified category. diff --git a/tests/conftest.py b/tests/conftest.py index 86de6476..36896de7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,7 +11,7 @@ import pytest -from fab.tools import Category, CCompiler, FortranCompiler, Linker, ToolBox +from fab.tools import CCompiler, FortranCompiler, Linker, ToolBox # This avoids pylint warnings about Redefining names from outer scope @@ -44,10 +44,9 @@ def fixture_mock_fortran_compiler(): @pytest.fixture(name="mock_linker") -def fixture_mock_linker(): +def fixture_mock_linker(mock_fortran_compiler): '''Provides a mock linker.''' - mock_linker = Linker("mock_linker", "mock_linker.exe", - Category.FORTRAN_COMPILER) + mock_linker = Linker(mock_fortran_compiler) mock_linker.run = mock.Mock() mock_linker._version = (1, 2, 3) mock_linker.add_lib_flags("netcdf", ["-lnetcdff", "-lnetcdf"]) diff --git a/tests/unit_tests/steps/test_link.py b/tests/unit_tests/steps/test_link.py index a20c4ff4..e9a6750c 100644 --- a/tests/unit_tests/steps/test_link.py +++ b/tests/unit_tests/steps/test_link.py @@ -3,22 +3,29 @@ # For further details please refer to the file COPYRIGHT # which you should have received as part of this distribution # ############################################################################## + +''' +Tests linking an executable. +''' + from pathlib import Path from types import SimpleNamespace from unittest import mock from fab.artefacts import ArtefactSet, ArtefactStore from fab.steps.link import link_exe -from fab.tools import Linker +from fab.tools import FortranCompiler, Linker import pytest class TestLinkExe: + '''Test class for linking an executable. + ''' def test_run(self, tool_box): - # ensure the command is formed correctly, with the flags at the - # end (why?!) - + '''Ensure the command is formed correctly, with the flags at the + end and that environment variable FFLAGS is picked up. + ''' config = SimpleNamespace( project_workspace=Path('workspace'), artefact_store=ArtefactStore(), @@ -29,9 +36,20 @@ def test_run(self, tool_box): config.artefact_store[ArtefactSet.OBJECT_FILES] = \ {'foo': {'foo.o', 'bar.o'}} - with mock.patch('os.getenv', return_value='-L/foo1/lib -L/foo2/lib'): - # We need to create a linker here to pick up the env var: - linker = Linker("mock_link", "mock_link.exe", "mock-vendor") + with mock.patch.dict("os.environ", + {"FFLAGS": "-L/foo1/lib -L/foo2/lib"}): + # We need to create the compiler here in order to pick + # up the environment + mock_compiler = FortranCompiler("mock_fortran_compiler", + "mock_fortran_compiler.exe", + "suite", module_folder_flag="", + version_regex="something", + syntax_only_flag=None, + compile_flag=None, + output_flag=None, openmp_flag=None) + mock_compiler.run = mock.Mock() + + linker = Linker(compiler=mock_compiler) # Mark the linker as available to it can be added to the tool box linker._is_available = True @@ -44,10 +62,12 @@ def test_run(self, tool_box): pytest.warns(UserWarning, match="_metric_send_conn not " "set, cannot send metrics"): - link_exe(config, libs=['mylib'], flags=['-fooflag', '-barflag']) + link_exe(config, libs=['mylib'], + flags=['-fooflag', '-barflag']) tool_run.assert_called_with( - ['mock_link.exe', '-L/foo1/lib', '-L/foo2/lib', 'bar.o', 'foo.o', + ['mock_fortran_compiler.exe', '-L/foo1/lib', '-L/foo2/lib', + 'bar.o', 'foo.o', '-L/my/lib', '-mylib', '-fooflag', '-barflag', '-o', 'workspace/foo'], capture_output=True, env=None, cwd=None, check=False) diff --git a/tests/unit_tests/steps/test_link_shared_object.py b/tests/unit_tests/steps/test_link_shared_object.py index 700a3de3..c76eb957 100644 --- a/tests/unit_tests/steps/test_link_shared_object.py +++ b/tests/unit_tests/steps/test_link_shared_object.py @@ -13,7 +13,7 @@ from fab.artefacts import ArtefactSet, ArtefactStore from fab.steps.link import link_shared_object -from fab.tools import Linker +from fab.tools import FortranCompiler, Linker import pytest @@ -32,9 +32,18 @@ def test_run(tool_box): config.artefact_store[ArtefactSet.OBJECT_FILES] = \ {None: {'foo.o', 'bar.o'}} - with mock.patch('os.getenv', return_value='-L/foo1/lib -L/foo2/lib'): - # We need to create a linker here to pick up the env var: - linker = Linker("mock_link", "mock_link.exe", "vendor") + with mock.patch.dict("os.environ", {"FFLAGS": "-L/foo1/lib -L/foo2/lib"}): + # We need to create the compiler here in order to pick + # up the environment + mock_compiler = FortranCompiler("mock_fortran_compiler", + "mock_fortran_compiler.exe", + "suite", module_folder_flag="", + version_regex="something", + syntax_only_flag=None, + compile_flag=None, output_flag=None, + openmp_flag=None) + mock_compiler.run = mock.Mock() + linker = Linker(mock_compiler) # Mark the linker as available so it can added to the tool box: linker._is_available = True tool_box.add_tool(linker, silent_replace=True) @@ -47,6 +56,7 @@ def test_run(tool_box): flags=['-fooflag', '-barflag']) tool_run.assert_called_with( - ['mock_link.exe', '-L/foo1/lib', '-L/foo2/lib', 'bar.o', 'foo.o', - '-fooflag', '-barflag', '-fPIC', '-shared', '-o', '/tmp/lib_my.so'], + ['mock_fortran_compiler.exe', '-L/foo1/lib', '-L/foo2/lib', 'bar.o', + 'foo.o', '-fooflag', '-barflag', '-fPIC', '-shared', + '-o', '/tmp/lib_my.so'], capture_output=True, env=None, cwd=None, check=False) diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index f6c7c158..60c18d78 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -25,7 +25,7 @@ def test_compiler(): category=Category.C_COMPILER, openmp_flag="-fopenmp") assert cc.category == Category.C_COMPILER assert cc._compile_flag == "-c" - assert cc._output_flag == "-o" + assert cc.output_flag == "-o" # pylint: disable-next=use-implicit-booleaness-not-comparison assert cc.flags == [] assert cc.suite == "gnu" @@ -35,7 +35,7 @@ def test_compiler(): fc = FortranCompiler("gfortran", "gfortran", "gnu", openmp_flag="-fopenmp", version_regex="something", module_folder_flag="-J") assert fc._compile_flag == "-c" - assert fc._output_flag == "-o" + assert fc.output_flag == "-o" assert fc.category == Category.FORTRAN_COMPILER assert fc.suite == "gnu" # pylint: disable-next=use-implicit-booleaness-not-comparison diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index 6984c790..052af88d 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -13,43 +13,75 @@ import pytest -from fab.tools import (Category, Linker) +from fab.tools import (Category, Linker, ToolRepository) def test_linker(mock_c_compiler, mock_fortran_compiler): '''Test the linker constructor.''' - linker = Linker(name="my_linker", exec_name="my_linker.exe", suite="suite") + assert mock_c_compiler.category == Category.C_COMPILER + assert mock_c_compiler.name == "mock_c_compiler" + + linker = Linker(mock_c_compiler) assert linker.category == Category.LINKER - assert linker.name == "my_linker" - assert linker.exec_name == "my_linker.exe" + assert linker.name == "linker-mock_c_compiler" + assert linker.exec_name == "mock_c_compiler.exe" assert linker.suite == "suite" assert linker.flags == [] + assert linker.output_flag == "-o" - linker = Linker(name="my_linker", compiler=mock_c_compiler) - assert linker.category == Category.LINKER - assert linker.name == "my_linker" - assert linker.exec_name == mock_c_compiler.exec_name - assert linker.suite == mock_c_compiler.suite - assert linker.flags == [] + assert mock_fortran_compiler.category == Category.FORTRAN_COMPILER + assert mock_fortran_compiler.name == "mock_fortran_compiler" - linker = Linker(compiler=mock_c_compiler) + linker = Linker(mock_fortran_compiler) assert linker.category == Category.LINKER - assert linker.name == mock_c_compiler.name - assert linker.exec_name == mock_c_compiler.exec_name - assert linker.suite == mock_c_compiler.suite + assert linker.name == "linker-mock_fortran_compiler" + assert linker.exec_name == "mock_fortran_compiler.exe" + assert linker.suite == "suite" assert linker.flags == [] - linker = Linker(compiler=mock_fortran_compiler) - assert linker.category == Category.LINKER - assert linker.name == mock_fortran_compiler.name - assert linker.exec_name == mock_fortran_compiler.exec_name - assert linker.flags == [] +def test_linker_constructor_error(mock_c_compiler): + '''Test the linker constructor with invalid parameters.''' + + with pytest.raises(RuntimeError) as err: + Linker() + assert ("Neither compiler nor linker is specified in linker constructor." + in str(err.value)) with pytest.raises(RuntimeError) as err: - linker = Linker(name="no-exec-given") - assert ("Either specify name, exec name, and suite or a compiler when " - "creating Linker." in str(err.value)) + Linker(compiler=mock_c_compiler, linker=mock_c_compiler) + assert ("Both compiler and linker is specified in linker constructor." + in str(err.value)) + + +@pytest.mark.parametrize("mpi", [True, False]) +def test_linker_mpi(mock_c_compiler, mpi): + '''Test that linker wrappers handle MPI as expected.''' + + mock_c_compiler._mpi = mpi + linker = Linker(compiler=mock_c_compiler) + assert linker.mpi == mpi + + wrapped_linker = Linker(linker=linker) + assert wrapped_linker.mpi == mpi + + +@pytest.mark.parametrize("openmp", [True, False]) +def test_linker_openmp(mock_c_compiler, openmp): + '''Test that linker wrappers handle openmp as expected. Note that + a compiler detects support for OpenMP by checking if an openmp flag + is defined. + ''' + + if openmp: + mock_c_compiler._openmp_flag = "-some-openmp-flag" + else: + mock_c_compiler._openmp_flag = "" + linker = Linker(compiler=mock_c_compiler) + assert linker.openmp == openmp + + wrapped_linker = Linker(linker=linker) + assert wrapped_linker.openmp == openmp def test_linker_gets_ldflags(mock_c_compiler): @@ -62,31 +94,28 @@ def test_linker_gets_ldflags(mock_c_compiler): def test_linker_check_available(mock_c_compiler): '''Tests the is_available functionality.''' - # First test if a compiler is given. The linker will call the + # First test when a compiler is given. The linker will call the # corresponding function in the compiler: - linker = Linker(compiler=mock_c_compiler) - with mock.patch.object(mock_c_compiler, "check_available", - return_value=True) as comp_run: + linker = Linker(mock_c_compiler) + with mock.patch('fab.tools.compiler.Compiler.get_version', + return_value=(1, 2, 3)): assert linker.check_available() - # It should be called once without any parameter - comp_run.assert_called_once_with() - # Second test, no compiler is given. Mock Tool.run to - # return a success: - linker = Linker("ld", "ld", suite="gnu") - mock_result = mock.Mock(returncode=0) - with mock.patch('fab.tools.tool.subprocess.run', - return_value=mock_result) as tool_run: - linker.check_available() - tool_run.assert_called_once_with( - ["ld", "--version"], capture_output=True, env=None, - cwd=None, check=False) - - # Third test: assume the tool does not exist, check_available - # will return False (and not raise an exception) - linker._is_available = None - with mock.patch("fab.tools.tool.Tool.run", - side_effect=RuntimeError("")) as tool_run: + # Then test the usage of a linker wrapper. The linker will call the + # corresponding function in the wrapper linker: + wrapped_linker = Linker(linker=linker) + with mock.patch('fab.tools.compiler.Compiler.get_version', + return_value=(1, 2, 3)): + assert wrapped_linker.check_available() + + +def test_linker_check_unavailable(mock_c_compiler): + '''Tests the is_available functionality.''' + # assume the tool does not exist, check_available + # will return False (and not raise an exception) + linker = Linker(mock_c_compiler) + with mock.patch('fab.tools.compiler.Compiler.get_version', + side_effect=RuntimeError("")): assert linker.check_available() is False @@ -103,8 +132,8 @@ def test_linker_get_lib_flags(mock_linker): def test_linker_get_lib_flags_unknown(mock_c_compiler): - """Linker should raise an error if flags are requested for a library that is - unknown + """Linker should raise an error if flags are requested for a library + that is unknown. """ linker = Linker(compiler=mock_c_compiler) with pytest.raises(RuntimeError) as err: @@ -123,7 +152,8 @@ def test_linker_add_lib_flags(mock_c_compiler): def test_linker_add_lib_flags_overwrite_defaults(mock_linker): - """Linker should provide a way to replace the default flags for a library""" + """Linker should provide a way to replace the default flags for + a library""" # Initially we have the default netcdf flags result = mock_linker.get_lib_flags("netcdf") @@ -178,7 +208,9 @@ def test_linker_remove_lib_flags_unknown(mock_linker): # Linking: # ==================== def test_linker_c(mock_c_compiler): - '''Test the link command line when no additional libraries are specified.''' + '''Test the link command line when no additional libraries are + specified.''' + linker = Linker(compiler=mock_c_compiler) # Add a library to the linker, but don't use it in the link step linker.add_lib_flags("customlib", ["-lcustom", "-jcustom"]) @@ -264,29 +296,16 @@ def test_compiler_linker_add_compiler_flag(mock_c_compiler): capture_output=True, env=None, cwd=None, check=False) -def test_linker_add_compiler_flag(): - '''Make sure ad-hoc linker flags work if a linker is created without a - compiler: - ''' - linker = Linker("no-compiler", "no-compiler.exe", "suite") - linker.flags.append("-some-other-flag") - mock_result = mock.Mock(returncode=0) - with mock.patch('fab.tools.tool.subprocess.run', - return_value=mock_result) as tool_run: - linker.link([Path("a.o")], Path("a.out"), openmp=False) - tool_run.assert_called_with( - ['no-compiler.exe', '-some-other-flag', 'a.o', '-o', 'a.out'], - capture_output=True, env=None, cwd=None, check=False) - - def test_linker_all_flag_types(mock_c_compiler): """Make sure all possible sources of linker flags are used in the right order""" + + # Environment variables for both the linker with mock.patch.dict("os.environ", {"LDFLAGS": "-ldflag"}): linker = Linker(compiler=mock_c_compiler) - mock_c_compiler.flags.extend(["-compiler-flag1", "-compiler-flag2"]) - linker.flags.extend(["-linker-flag1", "-linker-flag2"]) + mock_c_compiler.add_flags(["-compiler-flag1", "-compiler-flag2"]) + linker.add_flags(["-linker-flag1", "-linker-flag2"]) linker.add_pre_lib_flags(["-prelibflag1", "-prelibflag2"]) linker.add_lib_flags("customlib1", ["-lib1flag1", "lib1flag2"]) linker.add_lib_flags("customlib2", ["-lib2flag1", "lib2flag2"]) @@ -302,8 +321,6 @@ def test_linker_all_flag_types(mock_c_compiler): tool_run.assert_called_with([ "mock_c_compiler.exe", - # Note: compiler flags and linker flags will be switched when the Linker - # becomes a CompilerWrapper in a following PR "-ldflag", "-linker-flag1", "-linker-flag2", "-compiler-flag1", "-compiler-flag2", "-fopenmp", @@ -314,3 +331,49 @@ def test_linker_all_flag_types(mock_c_compiler): "-postlibflag1", "-postlibflag2", "-o", "a.out"], capture_output=True, env=None, cwd=None, check=False) + + +def test_linker_nesting(mock_c_compiler): + """Make sure all possible sources of linker flags are used in the right + order""" + + linker1 = Linker(compiler=mock_c_compiler) + linker1.add_pre_lib_flags(["pre_lib1"]) + linker1.add_lib_flags("lib_a", ["a_from_1"]) + linker1.add_lib_flags("lib_c", ["c_from_1"]) + linker1.add_post_lib_flags(["post_lib1"]) + linker2 = Linker(linker=linker1) + linker2.add_pre_lib_flags(["pre_lib2"]) + linker2.add_lib_flags("lib_b", ["b_from_2"]) + linker2.add_lib_flags("lib_c", ["c_from_2"]) + linker1.add_post_lib_flags(["post_lib2"]) + + mock_result = mock.Mock(returncode=0) + with mock.patch("fab.tools.tool.subprocess.run", + return_value=mock_result) as tool_run: + linker2.link( + [Path("a.o")], Path("a.out"), + libs=["lib_a", "lib_b", "lib_c"], + openmp=True) + tool_run.assert_called_with(["mock_c_compiler.exe", "-fopenmp", + "a.o", "pre_lib2", "pre_lib1", "a_from_1", + "b_from_2", "c_from_2", + "post_lib1", "post_lib2", "-o", "a.out"], + capture_output=True, env=None, cwd=None, + check=False) + + +def test_linker_inheriting(): + '''Make sure that libraries from a wrapper compiler will be + available for a wrapper. + ''' + tr = ToolRepository() + linker_gfortran = tr.get_tool(Category.LINKER, "linker-gfortran") + linker_mpif90 = tr.get_tool(Category.LINKER, "linker-mpif90-gfortran") + + linker_gfortran.add_lib_flags("lib_a", ["a_from_1"]) + assert linker_mpif90.get_lib_flags("lib_a") == ["a_from_1"] + + with pytest.raises(RuntimeError) as err: + linker_mpif90.get_lib_flags("does_not_exist") + assert "Unknown library name: 'does_not_exist'" in str(err.value) From 6e5170d231ecd1bb5ef47fab5f8a3672eed56830 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 5 Feb 2025 00:01:44 +1100 Subject: [PATCH 06/12] Added more description for this test. --- tests/unit_tests/tools/test_compiler_wrapper.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index 64f65589..11096f0c 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -257,10 +257,14 @@ def test_compiler_wrapper_flags_independent(): assert mpicc.flags == ["-a", "-b"] assert mpicc.openmp_flag == gcc.openmp_flag - # Test a compiler wrapper correctly queries the wrapper compiler - # for openmp flag: Set the wrapper to have no _openmp_flag (which - # is actually the default, since it never sets this), but gcc - # does have a flag -s o mpicc should report that is supports openmp + # Test a compiler wrapper correctly queries the wrapper compiler for + # openmp flag: Set the wrapper to have no _openmp_flag (which is + # actually the default, since the wrapper never sets its own flag), but + # gcc does have a flag, so mpicc should report that is supports openmp. + # mpicc.openmp calls openmp of its base class (Compiler), which queries + # if an openmp flag is defined. This query must go to the openmp property, + # since the wrapper overwrites this property to return the wrapped + # compiler's flag (and not the wrapper's flag, which would not be defined) with mock.patch.object(mpicc, "_openmp_flag", ""): assert mpicc._openmp_flag == "" assert mpicc.openmp From ca139c1dc9a1f1cbdf42ee94050a341aff1a96f3 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 5 Feb 2025 20:33:06 +1100 Subject: [PATCH 07/12] Remove support for '-' in nvidia version numbers (which causes problems with compiler wrapper which do not support this). --- source/fab/tools/compiler.py | 32 ++----------------------- tests/unit_tests/tools/test_compiler.py | 4 ++-- 2 files changed, 4 insertions(+), 32 deletions(-) diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 3f8c31e1..7a8406e8 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -467,21 +467,7 @@ class Nvc(CCompiler): def __init__(self, name: str = "nvc", exec_name: str = "nvc"): super().__init__(name, exec_name, suite="nvidia", openmp_flag="-mp", - version_regex=r"nvc (\d[\d\.-]+\d)") - - def run_version_command( - self, version_command: Optional[str] = '--version') -> str: - '''Run the compiler's command to get its version. This implementation - runs the function in the base class, and changes any '-' into a - '.' to support nvidia version numbers which have dashes, e.g. 23.5-0. - - :param version_command: The compiler argument used to get version info. - - :returns: The output from the version command, with any '-' replaced - with '.' - ''' - version_string = super().run_version_command() - return version_string.replace("-", ".") + version_regex=r"nvc (\d[\d\.]+\d)") # ============================================================================ @@ -499,21 +485,7 @@ def __init__(self, name: str = "nvfortran", exec_name: str = "nvfortran"): module_folder_flag="-module", openmp_flag="-mp", syntax_only_flag="-Msyntax-only", - version_regex=r"nvfortran (\d[\d\.-]+\d)") - - def run_version_command( - self, version_command: Optional[str] = '--version') -> str: - '''Run the compiler's command to get its version. This implementation - runs the function in the base class, and changes any '-' into a - '.' to support nvidia version numbers which have dashes, e.g. 23.5-0. - - :param version_command: The compiler argument used to get version info. - - :returns: The output from the version command, with any '-' replaced - with '.' - ''' - version_string = super().run_version_command() - return version_string.replace("-", ".") + version_regex=r"nvfortran (\d[\d\.]+\d)") # ============================================================================ diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index f6c7c158..eaadbbd9 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -778,7 +778,7 @@ def test_nvc_get_version_23_5_0(): """) nvc = Nvc() with mock.patch.object(nvc, "run", mock.Mock(return_value=full_output)): - assert nvc.get_version() == (23, 5, 0) + assert nvc.get_version() == (23, 5) def test_nvc_get_version_with_icc_string(): @@ -819,7 +819,7 @@ def test_nvfortran_get_version_23_5_0(): nvfortran = Nvfortran() with mock.patch.object(nvfortran, "run", mock.Mock(return_value=full_output)): - assert nvfortran.get_version() == (23, 5, 0) + assert nvfortran.get_version() == (23, 5) def test_nvfortran_get_version_with_ifort_string(): From 9218aa8d8b142bf0297af517933d1f01dd4e5106 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 6 Feb 2025 11:20:42 +1100 Subject: [PATCH 08/12] Fix regex for Cray compiler. --- source/fab/tools/compiler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 7a8406e8..039774f9 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -510,7 +510,7 @@ class Craycc(CCompiler): def __init__(self, name: str = "craycc-cc", exec_name: str = "cc"): super().__init__(name, exec_name, suite="cray", mpi=True, openmp_flag="-homp", - version_regex=r"Cray [Cc][^\d]* (\d[\d\.]+\d) ") + version_regex=r"Cray [Cc][^\d]* (\d[\d\.]+\d)$") # ============================================================================ @@ -529,4 +529,4 @@ def __init__(self, name: str = "crayftn-ftn", exec_name: str = "ftn"): openmp_flag="-homp", syntax_only_flag="-syntax-only", version_regex=(r"Cray Fortran : Version " - r"(\d[\d\.]+\d) ")) + r"(\d[\d\.]+\d)$")) From e01f1cec8ecda1cf1ec647b036c120f33580132a Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 6 Feb 2025 16:44:35 +1100 Subject: [PATCH 09/12] More improvements for Cray version regex. --- source/fab/tools/compiler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 039774f9..70ed90a1 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -510,7 +510,7 @@ class Craycc(CCompiler): def __init__(self, name: str = "craycc-cc", exec_name: str = "cc"): super().__init__(name, exec_name, suite="cray", mpi=True, openmp_flag="-homp", - version_regex=r"Cray [Cc][^\d]* (\d[\d\.]+\d)$") + version_regex=r"Cray [Cc][^\d]* (\d[\d\.]+\d)") # ============================================================================ @@ -529,4 +529,4 @@ def __init__(self, name: str = "crayftn-ftn", exec_name: str = "ftn"): openmp_flag="-homp", syntax_only_flag="-syntax-only", version_regex=(r"Cray Fortran : Version " - r"(\d[\d\.]+\d)$")) + r"(\d[\d\.]+\d)")) From fed4feb90820d445fa4008cc903d82946f8f2b48 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Fri, 7 Feb 2025 02:41:58 +1100 Subject: [PATCH 10/12] Cleaner handling of linking external libraries (#374) --- Documentation/source/advanced_config.rst | 67 +++++- source/fab/steps/link.py | 20 +- source/fab/tools/linker.py | 67 +++++- tests/conftest.py | 1 + .../unit_tests/steps/test_archive_objects.py | 14 +- tests/unit_tests/steps/test_link.py | 25 ++- tests/unit_tests/tools/test_linker.py | 196 ++++++++++++++++-- 7 files changed, 348 insertions(+), 42 deletions(-) diff --git a/Documentation/source/advanced_config.rst b/Documentation/source/advanced_config.rst index faaf0b25..ec19008f 100644 --- a/Documentation/source/advanced_config.rst +++ b/Documentation/source/advanced_config.rst @@ -186,12 +186,75 @@ Linker flags ------------ Probably the most common instance of the need to pass additional arguments is -to specify 3rd party libraries at the link stage. +to specify 3rd party libraries at the link stage. The linker tool allow +for the definition of library-specific flags: for each library, the user can +specify the required linker flags for this library. In the linking step, +only the name of the libraries to be linked is then required. The linker +object will then use the required linking flags. Typically, a site-specific +setup set (see for example https://github.com/MetOffice/lfric-baf) will +specify the right flags for each site, and the application itself only +needs to list the name of the libraries. This way, the application-specific +Fab script is independent from any site-specific settings. Still, an +application-specific script can also overwrite any site-specific setting, +for example if a newer version of a dependency is to be evaluated. + +The settings for a library are defined as follows: .. code-block:: :linenos: - link_exe(state, flags=['-lm', '-lnetcdf']) + tr = ToolRepository() + linker = tr.get_tool(Category.LINKER, "linker-ifort") + + linker.add_lib_flags("yaxt", ["-L/some_path", "-lyaxt", "-lyaxt_c"]) + linker.add_lib_flags("xios", ["-lxios"]) + +This will define two libraries called ``yaxt`` and ``xios``. In the link step, +the application only needs to specify the name of the libraries required, e.g.: + +.. code-block:: + :linenos: + + link_exe(state, libs=["yaxt", "xios"]) + +The linker will then use the specified options. + +A linker object also allows to define options that should always be added, +either as options before any library details, or at the very end. For example: + +.. code-block:: + :linenos: + + linker.add_pre_lib_flags(["-L/my/common/library/path"]) + linker.add_post_lib_flags(["-lstdc++"]) + +The pre_lib_flags can be used to specify library paths that contain +several libraries only once, and this makes it easy to evaluate a different +set of libraries. Additionally, this can also be used to add common +linking options, e.g. Cray's ``-Ktrap=fp``. + +The post_lib_flags can be used for additional common libraries that need +to be linked in. For example, if the application contains a dependency to +C++ but it is using the Fortran compiler for linking, then the C++ libraries +need to be explicitly added. But if there are several libraries depending +on it, you would have to specify this several times (forcing the linker to +re-read the library several times). Instead, you can just add it to the +post flags once. + +The linker step itself can also take optional flags: + +.. code-block:: + :linenos: + + link_exe(state, flags=['-Ktrap=fp']) + +These flags will be added to the very end of the the linker options, +i.e. after any other library or post-lib flag. Note that the example above is +not actually recommended to use, since the specified flag is only +valid for certain linker, and a Fab application script should in general +not hard-code flags for a specific linker. Adding the flag to the linker +instance itself, as shown further above, is the better approach. + Path-specific flags ------------------- diff --git a/source/fab/steps/link.py b/source/fab/steps/link.py index 6a14cf64..902defd9 100644 --- a/source/fab/steps/link.py +++ b/source/fab/steps/link.py @@ -9,7 +9,7 @@ """ import logging from string import Template -from typing import Optional +from typing import List, Optional from fab.artefacts import ArtefactSet from fab.steps import step @@ -33,7 +33,10 @@ def __call__(self, artefact_store): @step -def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None): +def link_exe(config, + libs: Optional[List[str]] = None, + flags: Optional[List[str]] = None, + source: Optional[ArtefactsGetter] = None): """ Link object files into an executable for every build target. @@ -49,8 +52,10 @@ def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None): The :class:`fab.build_config.BuildConfig` object where we can read settings such as the project workspace folder or the multiprocessing flag. + :param libs: + A list of required library names to pass to the linker. :param flags: - A list of flags to pass to the linker. + A list of additional flags to pass to the linker. :param source: An optional :class:`~fab.artefacts.ArtefactsGetter`. It defaults to the output from compiler steps, which typically is the expected behaviour. @@ -60,13 +65,15 @@ def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None): openmp=config.openmp) logger.info(f'Linker is {linker.name}') - flags = flags or [] + libs = libs or [] + if flags: + linker.add_post_lib_flags(flags) source_getter = source or DefaultLinkerSource() target_objects = source_getter(config.artefact_store) for root, objects in target_objects.items(): exe_path = config.project_workspace / f'{root}' - linker.link(objects, exe_path, openmp=config.openmp, add_libs=flags) + linker.link(objects, exe_path, openmp=config.openmp, libs=libs) config.artefact_store.add(ArtefactSet.EXECUTABLES, exe_path) @@ -116,4 +123,5 @@ def link_shared_object(config, output_fpath: str, flags=None, objects = target_objects[None] out_name = Template(output_fpath).substitute(output=config.build_output) - linker.link(objects, out_name, openmp=config.openmp, add_libs=flags) + linker.add_post_lib_flags(flags) + linker.link(objects, out_name, openmp=config.openmp) diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 69cfcec2..b0536b69 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -9,7 +9,8 @@ import os from pathlib import Path -from typing import cast, List, Optional +from typing import cast, Dict, List, Optional +import warnings from fab.tools.category import Category from fab.tools.compiler import Compiler @@ -51,6 +52,12 @@ def __init__(self, name: Optional[str] = None, self._compiler = compiler self.flags.extend(os.getenv("LDFLAGS", "").split()) + # Maintain a set of flags for common libraries. + self._lib_flags: Dict[str, List[str]] = {} + # Allow flags to include before or after any library-specific flags. + self._pre_lib_flags: List[str] = [] + self._post_lib_flags: List[str] = [] + @property def mpi(self) -> bool: ''':returns: whether the linker supports MPI or not.''' @@ -66,16 +73,61 @@ def check_available(self) -> bool: return super().check_available() + def get_lib_flags(self, lib: str) -> List[str]: + '''Gets the standard flags for a standard library + + :param lib: the library name + + :returns: a list of flags + + :raises RuntimeError: if lib is not recognised + ''' + try: + return self._lib_flags[lib] + except KeyError: + raise RuntimeError(f"Unknown library name: '{lib}'") + + def add_lib_flags(self, lib: str, flags: List[str], + silent_replace: bool = False): + '''Add a set of flags for a standard library + + :param lib: the library name + :param flags: the flags to use with the library + :param silent_replace: if set, no warning will be printed when an + existing lib is overwritten. + ''' + if lib in self._lib_flags and not silent_replace: + warnings.warn(f"Replacing existing flags for library {lib}: " + f"'{self._lib_flags[lib]}' with " + f"'{flags}'.") + + # Make a copy to avoid modifying the caller's list + self._lib_flags[lib] = flags[:] + + def add_pre_lib_flags(self, flags: List[str]): + '''Add a set of flags to use before any library-specific flags + + :param flags: the flags to include + ''' + self._pre_lib_flags.extend(flags) + + def add_post_lib_flags(self, flags: List[str]): + '''Add a set of flags to use after any library-specific flags + + :param flags: the flags to include + ''' + self._post_lib_flags.extend(flags) + def link(self, input_files: List[Path], output_file: Path, openmp: bool, - add_libs: Optional[List[str]] = None) -> str: + libs: Optional[List[str]] = None) -> str: '''Executes the linker with the specified input files, creating `output_file`. :param input_files: list of input files to link. :param output_file: output file. :param openm: whether OpenMP is requested or not. - :param add_libs: additional linker flags. + :param libs: additional libraries to link with. :returns: the stdout of the link command ''' @@ -88,7 +140,12 @@ def link(self, input_files: List[Path], output_file: Path, params = [] # TODO: why are the .o files sorted? That shouldn't matter params.extend(sorted(map(str, input_files))) - if add_libs: - params += add_libs + + if self._pre_lib_flags: + params.extend(self._pre_lib_flags) + for lib in (libs or []): + params.extend(self.get_lib_flags(lib)) + if self._post_lib_flags: + params.extend(self._post_lib_flags) params.extend([self._output_flag, str(output_file)]) return self.run(params) diff --git a/tests/conftest.py b/tests/conftest.py index 835cd294..559d4f3b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -48,6 +48,7 @@ def fixture_mock_linker(): Category.FORTRAN_COMPILER) mock_linker.run = mock.Mock() mock_linker._version = (1, 2, 3) + mock_linker.add_lib_flags("netcdf", ["-lnetcdff", "-lnetcdf"]) return mock_linker diff --git a/tests/unit_tests/steps/test_archive_objects.py b/tests/unit_tests/steps/test_archive_objects.py index f5b2683e..097200ea 100644 --- a/tests/unit_tests/steps/test_archive_objects.py +++ b/tests/unit_tests/steps/test_archive_objects.py @@ -81,20 +81,20 @@ def test_for_library(self): assert config.artefact_store[ArtefactSet.OBJECT_ARCHIVES] == { None: set([str(config.build_output / 'mylib.a')])} - def test_incorrect_tool(self): + def test_incorrect_tool(self, tool_box): '''Test that an incorrect archive tool is detected ''' - config = BuildConfig('proj', ToolBox()) - tool_box = config.tool_box + config = BuildConfig('proj', tool_box) cc = tool_box.get_tool(Category.C_COMPILER, config.mpi, config.openmp) - # And set its category to C_COMPILER + # And set its category to be AR cc._category = Category.AR - # So overwrite the C compiler with the re-categories Fortran compiler + # Now add this 'ar' tool to the tool box tool_box.add_tool(cc) with pytest.raises(RuntimeError) as err: archive_objects(config=config, output_fpath=config.build_output / 'mylib.a') - assert ("Unexpected tool 'gcc' of type '' instead of Ar" in str(err.value)) + assert ("Unexpected tool 'mock_c_compiler' of type '' instead of Ar" + in str(err.value)) diff --git a/tests/unit_tests/steps/test_link.py b/tests/unit_tests/steps/test_link.py index a675f54c..3d5b643a 100644 --- a/tests/unit_tests/steps/test_link.py +++ b/tests/unit_tests/steps/test_link.py @@ -3,21 +3,29 @@ # For further details please refer to the file COPYRIGHT # which you should have received as part of this distribution # ############################################################################## + +'''This module tests the linking step. +''' + from pathlib import Path from types import SimpleNamespace from unittest import mock from fab.artefacts import ArtefactSet, ArtefactStore from fab.steps.link import link_exe -from fab.tools import Linker +from fab.tools import Linker, ToolBox import pytest class TestLinkExe: - def test_run(self, tool_box): - # ensure the command is formed correctly, with the flags at the - # end (why?!) + '''This class test the linking step. + ''' + + def test_run(self, tool_box: ToolBox): + ''' Ensure the command is formed correctly, with the flags at the + end. + ''' config = SimpleNamespace( project_workspace=Path('workspace'), @@ -34,6 +42,9 @@ def test_run(self, tool_box): linker = Linker("mock_link", "mock_link.exe", "mock-vendor") # Mark the linker as available to it can be added to the tool box linker._is_available = True + + # Add a custom library to the linker + linker.add_lib_flags('mylib', ['-L/my/lib', '-lmylib']) tool_box.add_tool(linker, silent_replace=True) mock_result = mock.Mock(returncode=0, stdout="abc\ndef".encode()) with mock.patch('fab.tools.tool.subprocess.run', @@ -41,9 +52,11 @@ def test_run(self, tool_box): pytest.warns(UserWarning, match="_metric_send_conn not " "set, cannot send metrics"): - link_exe(config, flags=['-fooflag', '-barflag']) + link_exe(config, libs=['mylib'], + flags=['-fooflag', '-barflag']) tool_run.assert_called_with( ['mock_link.exe', '-L/foo1/lib', '-L/foo2/lib', 'bar.o', 'foo.o', - '-fooflag', '-barflag', '-o', 'workspace/foo'], + '-L/my/lib', '-lmylib', '-fooflag', '-barflag', + '-o', 'workspace/foo'], capture_output=True, env=None, cwd=None, check=False) diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index 772cd7ec..243f0c7a 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -9,6 +9,7 @@ from pathlib import Path from unittest import mock +import warnings import pytest @@ -18,8 +19,7 @@ def test_linker(mock_c_compiler, mock_fortran_compiler): '''Test the linker constructor.''' - linker = Linker(name="my_linker", exec_name="my_linker.exe", - suite="suite") + linker = Linker(name="my_linker", exec_name="my_linker.exe", suite="suite") assert linker.category == Category.LINKER assert linker.name == "my_linker" assert linker.exec_name == "my_linker.exe" @@ -52,6 +52,13 @@ def test_linker(mock_c_compiler, mock_fortran_compiler): "creating Linker." in str(err.value)) +def test_linker_gets_ldflags(mock_c_compiler): + """Tests that the linker retrieves env.LDFLAGS""" + with mock.patch.dict("os.environ", {"LDFLAGS": "-lm"}): + linker = Linker(compiler=mock_c_compiler) + assert "-lm" in linker.flags + + def test_linker_check_available(mock_c_compiler): '''Tests the is_available functionality.''' @@ -83,34 +90,154 @@ def test_linker_check_available(mock_c_compiler): assert linker.check_available() is False +# ==================== +# Managing lib flags: +# ==================== +def test_linker_get_lib_flags(mock_linker): + """Linker should provide a map of library names, each leading to a list of + linker flags + """ + # netcdf flags are built in to the mock linker + result = mock_linker.get_lib_flags("netcdf") + assert result == ["-lnetcdff", "-lnetcdf"] + + +def test_linker_get_lib_flags_unknown(mock_c_compiler): + """Linker should raise an error if flags are requested for a library that is + unknown + """ + linker = Linker(compiler=mock_c_compiler) + with pytest.raises(RuntimeError) as err: + linker.get_lib_flags("unknown") + assert "Unknown library name: 'unknown'" in str(err.value) + + +def test_linker_add_lib_flags(mock_c_compiler): + """Linker should provide a way to add a new set of flags for a library""" + linker = Linker(compiler=mock_c_compiler) + linker.add_lib_flags("xios", ["-L", "xios/lib", "-lxios"]) + + # Make sure we can get it back. The order should be maintained. + result = linker.get_lib_flags("xios") + assert result == ["-L", "xios/lib", "-lxios"] + + +def test_linker_add_lib_flags_overwrite_defaults(mock_linker): + """Linker should provide a way to replace the default flags for a library""" + + # Initially we have the default netcdf flags + result = mock_linker.get_lib_flags("netcdf") + assert result == ["-lnetcdff", "-lnetcdf"] + + # Replace them with another set of flags. + warn_message = 'Replacing existing flags for library netcdf' + with pytest.warns(UserWarning, match=warn_message): + mock_linker.add_lib_flags( + "netcdf", ["-L", "netcdf/lib", "-lnetcdf"]) + + # Test that we can see our custom flags + result = mock_linker.get_lib_flags("netcdf") + assert result == ["-L", "netcdf/lib", "-lnetcdf"] + + +def test_linker_add_lib_flags_overwrite_silent(mock_linker): + """Linker should provide the option to replace flags for a library without + generating a warning + """ + + # Initially we have the default netcdf flags + mock_linker.add_lib_flags("customlib", ["-lcustom", "-jcustom"]) + assert mock_linker.get_lib_flags("customlib") == ["-lcustom", "-jcustom"] + + # Replace them with another set of flags. + with warnings.catch_warnings(): + warnings.simplefilter("error") + mock_linker.add_lib_flags("customlib", ["-t", "-b"], + silent_replace=True) + + # Test that we can see our custom flags + result = mock_linker.get_lib_flags("customlib") + assert result == ["-t", "-b"] + + +# ==================== +# Linking: +# ==================== def test_linker_c(mock_c_compiler): - '''Test the link command line when no additional libraries are - specified.''' + '''Test the link command line when no additional libraries are specified.''' linker = Linker(compiler=mock_c_compiler) + # Add a library to the linker, but don't use it in the link step + linker.add_lib_flags("customlib", ["-lcustom", "-jcustom"]) + mock_result = mock.Mock(returncode=0) with mock.patch('fab.tools.tool.subprocess.run', return_value=mock_result) as tool_run: linker.link([Path("a.o")], Path("a.out"), openmp=False) tool_run.assert_called_with( - ["mock_c_compiler.exe", 'a.o', '-o', 'a.out'], capture_output=True, - env=None, cwd=None, check=False) + ["mock_c_compiler.exe", "a.o", "-o", "a.out"], + capture_output=True, env=None, cwd=None, check=False) def test_linker_c_with_libraries(mock_c_compiler): - '''Test the link command line when additional libraries are specified.''' + """Test the link command line when additional libraries are specified.""" linker = Linker(compiler=mock_c_compiler) + linker.add_lib_flags("customlib", ["-lcustom", "-jcustom"]) + + with mock.patch.object(linker, "run") as link_run: + linker.link( + [Path("a.o")], Path("a.out"), libs=["customlib"], openmp=True) + # The order of the 'libs' list should be maintained + link_run.assert_called_with( + ["-fopenmp", "a.o", "-lcustom", "-jcustom", "-o", "a.out"]) + + +def test_linker_c_with_libraries_and_post_flags(mock_c_compiler): + """Test the link command line when a library and additional flags are + specified.""" + linker = Linker(compiler=mock_c_compiler) + linker.add_lib_flags("customlib", ["-lcustom", "-jcustom"]) + linker.add_post_lib_flags(["-extra-flag"]) + with mock.patch.object(linker, "run") as link_run: - linker.link([Path("a.o")], Path("a.out"), add_libs=["-L", "/tmp"], - openmp=True) - link_run.assert_called_with(['-fopenmp', 'a.o', '-L', '/tmp', - '-o', 'a.out']) + linker.link( + [Path("a.o")], Path("a.out"), libs=["customlib"], openmp=False) + link_run.assert_called_with( + ["a.o", "-lcustom", "-jcustom", "-extra-flag", "-o", "a.out"]) + + +def test_linker_c_with_libraries_and_pre_flags(mock_c_compiler): + """Test the link command line when a library and additional flags are + specified.""" + linker = Linker(compiler=mock_c_compiler) + linker.add_lib_flags("customlib", ["-lcustom", "-jcustom"]) + linker.add_pre_lib_flags(["-L", "/common/path/"]) + + with mock.patch.object(linker, "run") as link_run: + linker.link( + [Path("a.o")], Path("a.out"), libs=["customlib"], openmp=False) + link_run.assert_called_with( + ["a.o", "-L", "/common/path/", "-lcustom", "-jcustom", "-o", "a.out"]) + + +def test_linker_c_with_unknown_library(mock_c_compiler): + """Test the link command raises an error when unknow libraries are + specified. + """ + linker = Linker(compiler=mock_c_compiler)\ + + with pytest.raises(RuntimeError) as err: + # Try to use "customlib" when we haven't added it to the linker + linker.link( + [Path("a.o")], Path("a.out"), libs=["customlib"], openmp=True) + + assert "Unknown library name: 'customlib'" in str(err.value) def test_compiler_linker_add_compiler_flag(mock_c_compiler): '''Test that a flag added to the compiler will be automatically - added to the link line (even if the flags are modified after - creating the linker ... in case that the user specifies additional - flags after creating the linker).''' + added to the link line (even if the flags are modified after creating the + linker ... in case that the user specifies additional flags after creating + the linker).''' linker = Linker(compiler=mock_c_compiler) mock_c_compiler.flags.append("-my-flag") @@ -124,8 +251,8 @@ def test_compiler_linker_add_compiler_flag(mock_c_compiler): def test_linker_add_compiler_flag(): - '''Make sure linker flags work if a linker is created without - a compiler: + '''Make sure ad-hoc linker flags work if a linker is created without a + compiler: ''' linker = Linker("no-compiler", "no-compiler.exe", "suite") linker.flags.append("-some-other-flag") @@ -136,3 +263,40 @@ def test_linker_add_compiler_flag(): tool_run.assert_called_with( ['no-compiler.exe', '-some-other-flag', 'a.o', '-o', 'a.out'], capture_output=True, env=None, cwd=None, check=False) + + +def test_linker_all_flag_types(mock_c_compiler): + """Make sure all possible sources of linker flags are used in the right + order""" + with mock.patch.dict("os.environ", {"LDFLAGS": "-ldflag"}): + linker = Linker(compiler=mock_c_compiler) + + mock_c_compiler.flags.extend(["-compiler-flag1", "-compiler-flag2"]) + linker.flags.extend(["-linker-flag1", "-linker-flag2"]) + linker.add_pre_lib_flags(["-prelibflag1", "-prelibflag2"]) + linker.add_lib_flags("customlib1", ["-lib1flag1", "lib1flag2"]) + linker.add_lib_flags("customlib2", ["-lib2flag1", "lib2flag2"]) + linker.add_post_lib_flags(["-postlibflag1", "-postlibflag2"]) + + mock_result = mock.Mock(returncode=0) + with mock.patch("fab.tools.tool.subprocess.run", + return_value=mock_result) as tool_run: + linker.link([ + Path("a.o")], Path("a.out"), + libs=["customlib2", "customlib1"], + openmp=True) + + tool_run.assert_called_with([ + "mock_c_compiler.exe", + # Note: compiler flags and linker flags will be switched when the Linker + # becomes a CompilerWrapper in a following PR + "-ldflag", "-linker-flag1", "-linker-flag2", + "-compiler-flag1", "-compiler-flag2", + "-fopenmp", + "a.o", + "-prelibflag1", "-prelibflag2", + "-lib2flag1", "lib2flag2", + "-lib1flag1", "lib1flag2", + "-postlibflag1", "-postlibflag2", + "-o", "a.out"], + capture_output=True, env=None, cwd=None, check=False) From e1bfe093a04adc46530cccaa184408e06077d605 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Fri, 7 Feb 2025 17:08:44 +1100 Subject: [PATCH 11/12] Removed likely a merge artefact. --- Documentation/source/advanced_config.rst | 8 -------- 1 file changed, 8 deletions(-) diff --git a/Documentation/source/advanced_config.rst b/Documentation/source/advanced_config.rst index 23c96060..ec19008f 100644 --- a/Documentation/source/advanced_config.rst +++ b/Documentation/source/advanced_config.rst @@ -256,14 +256,6 @@ not hard-code flags for a specific linker. Adding the flag to the linker instance itself, as shown further above, is the better approach. -Linkers will be pre-configured with flags for common libraries. Where possible, -a library name should be used to include the required flags for linking. - -.. code-block:: - :linenos: - - link_exe(state, libs=['netcdf']) - Path-specific flags ------------------- From 69243c8a106774bfb67919847fa3750e13d9031d Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Fri, 7 Feb 2025 18:11:43 +1100 Subject: [PATCH 12/12] Removed unnecessary mocking. --- tests/unit_tests/steps/test_link.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit_tests/steps/test_link.py b/tests/unit_tests/steps/test_link.py index bf9d62a7..dec2045a 100644 --- a/tests/unit_tests/steps/test_link.py +++ b/tests/unit_tests/steps/test_link.py @@ -47,7 +47,6 @@ def test_run(self, tool_box: ToolBox): syntax_only_flag=None, compile_flag=None, output_flag=None, openmp_flag=None) - mock_compiler.run = mock.Mock() linker = Linker(compiler=mock_compiler) # Mark the linker as available to it can be added to the tool box