diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index b937e9d1..8e04d011 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -76,7 +76,9 @@ def mpi(self) -> bool: def openmp(self) -> bool: ''':returns: if the compiler supports openmp or not ''' - return self._openmp_flag != "" + # It is important not to use `_openmp_flag` directly, since a compiler + # wrapper overwrites `openmp_flag`. + return self.openmp_flag != "" @property def openmp_flag(self) -> str: diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 2acef01b..63a3dd2b 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -11,7 +11,7 @@ import os from pathlib import Path -from typing import cast, Dict, List, Optional, Union +from typing import Dict, List, Optional, Union import warnings from fab.tools.category import Category @@ -20,11 +20,15 @@ class Linker(CompilerSuiteTool): - '''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 + '''This is the base class for any Linker. It takes an existing compiler + instance as parameter, and optional another linker. The latter is used + to get linker settings - for example, linker-mpif90-gfortran will use + mpif90-gfortran as compiler (i.e. to test if it is available and get + compilation flags), and linker-gfortran as linker. This way a user + only has to specify linker flags in the most basic class (gfortran), + all other linker wrapper will inherit the settings. + + :param compiler: a compiler instance :param linker: an optional linker instance :param name: name of the linker @@ -32,35 +36,19 @@ class Linker(CompilerSuiteTool): :raises RuntimeError: if neither compiler nor linker is specified. ''' - def __init__(self, compiler: Optional[Compiler] = None, + def __init__(self, compiler: Compiler, linker: Optional[Linker] = None, - exec_name: Optional[str] = None, 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: - assert final_compiler # make mypy happy - name = f"linker-{final_compiler.name}" - - if not exec_name: - # This will search for the name in linker or compiler - exec_name = self.get_exec_name() + name = f"linker-{compiler.name}" super().__init__( name=name, - exec_name=exec_name, + exec_name=compiler.exec_name, suite=self.suite, category=Category.LINKER) @@ -76,51 +64,31 @@ def check_available(self) -> bool: ''':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 + return self._compiler.check_available() @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 + the wrapped compiler.''' + return self._compiler.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 + with the wrapped compiler.''' + return self._compiler.mpi @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 + ''':returns: whether this linker supports OpenMP or not by checking + with the wrapped compiler.''' + return self._compiler.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 + return self._compiler.output_flag def get_lib_flags(self, lib: str) -> List[str]: '''Gets the standard flags for a standard library @@ -238,18 +206,10 @@ def link(self, input_files: List[Path], output_file: Path, 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) + params.extend(self._compiler.flags) if openmp: - params.append(compiler.openmp_flag) + params.append(self._compiler.openmp_flag) # TODO: why are the .o files sorted? That shouldn't matter params.extend(sorted(map(str, input_files))) diff --git a/source/fab/tools/tool_repository.py b/source/fab/tools/tool_repository.py index 1bf839f8..a9749757 100644 --- a/source/fab/tools/tool_repository.py +++ b/source/fab/tools/tool_repository.py @@ -117,19 +117,19 @@ def add_tool(self, tool: Tool): 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 + # the linker based on the wrapped 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). + # linker-gfortran. The compiler mpif90-gfortran will be the + # wrapper compiler. Reason is that e.g. 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, + linker = Linker(compiler, + linker=other_linker, name=f"linker-{compiler.name}") self[linker.category].append(linker) else: diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index 07f9a08b..11096f0c 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -257,6 +257,18 @@ 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 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 + # Adding flags to the wrapper should not affect the wrapped compiler: mpicc.add_flags(["-d", "-e"]) assert gcc.flags == ["-a", "-b"] diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index 052af88d..cd2d8dc9 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -41,28 +41,15 @@ def test_linker(mock_c_compiler, mock_fortran_compiler): 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(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) + linker = Linker(mock_c_compiler) assert linker.mpi == mpi - wrapped_linker = Linker(linker=linker) + wrapped_linker = Linker(mock_c_compiler, linker=linker) assert wrapped_linker.mpi == mpi @@ -80,7 +67,7 @@ def test_linker_openmp(mock_c_compiler, openmp): linker = Linker(compiler=mock_c_compiler) assert linker.openmp == openmp - wrapped_linker = Linker(linker=linker) + wrapped_linker = Linker(mock_c_compiler, linker=linker) assert wrapped_linker.openmp == openmp @@ -103,7 +90,7 @@ def test_linker_check_available(mock_c_compiler): # Then test the usage of a linker wrapper. The linker will call the # corresponding function in the wrapper linker: - wrapped_linker = Linker(linker=linker) + wrapped_linker = Linker(mock_c_compiler, linker=linker) with mock.patch('fab.tools.compiler.Compiler.get_version', return_value=(1, 2, 3)): assert wrapped_linker.check_available() @@ -342,7 +329,7 @@ def test_linker_nesting(mock_c_compiler): 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 = Linker(mock_c_compiler, 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"]) diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index 0c7d77e5..012487d4 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -11,7 +11,7 @@ import pytest from fab.tools import (Ar, Category, FortranCompiler, Gcc, Gfortran, Ifort, - Linker, ToolRepository) + ToolRepository) def test_tool_repository_get_singleton_new(): @@ -62,10 +62,6 @@ def test_tool_repository_get_default(): openmp=False) assert isinstance(gfortran, Gfortran) - gcc_linker = tr.get_default(Category.LINKER, mpi=False, openmp=False) - assert isinstance(gcc_linker, Linker) - assert gcc_linker.name == "linker-gcc" - gcc = tr.get_default(Category.C_COMPILER, mpi=False, openmp=False) assert isinstance(gcc, Gcc)