diff --git a/Documentation/source/advanced_config.rst b/Documentation/source/advanced_config.rst index d2b54ebf..ec19008f 100644 --- a/Documentation/source/advanced_config.rst +++ b/Documentation/source/advanced_config.rst @@ -186,20 +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") -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. + 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, libs=['netcdf']) + 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/compile_c.py b/source/fab/steps/compile_c.py index 283d9607..c7d014d7 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -127,6 +127,8 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): 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, diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index fe6f479b..e5767fae 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -136,6 +136,8 @@ def handle_compiler_args(config: BuildConfig, common_flags=None, 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()}') @@ -268,6 +270,8 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ 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/steps/link.py b/source/fab/steps/link.py index e67a8cce..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 List, Optional, Union +from typing import List, Optional from fab.artefacts import ArtefactSet from fab.steps import step @@ -34,8 +34,8 @@ def __call__(self, artefact_store): @step def link_exe(config, - libs: Union[List[str], None] = None, - flags: Union[List[str], None] = None, + libs: Optional[List[str]] = None, + flags: Optional[List[str]] = None, source: Optional[ArtefactsGetter] = None): """ Link object files into an executable for every build target. diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 8e04d011..7448411b 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -69,7 +69,7 @@ def __init__(self, name: str, @property def mpi(self) -> bool: - '''Returns whether this compiler supports MPI or not.''' + ''':returns: whether this compiler supports MPI or not.''' return self._mpi @property @@ -82,7 +82,7 @@ def openmp(self) -> bool: @property def openmp_flag(self) -> str: - '''Returns the flag to enable OpenMP.''' + ''':returns: the flag to enable OpenMP.''' return self._openmp_flag @property @@ -474,21 +474,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)") # ============================================================================ @@ -506,21 +492,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)") # ============================================================================ @@ -545,7 +517,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)") # ============================================================================ @@ -564,4 +536,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)")) diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index d7255306..4806ff43 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -145,6 +145,9 @@ def compile_file(self, input_file: Path, 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: diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 63a3dd2b..0fb0f61d 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -125,16 +125,6 @@ def add_lib_flags(self, lib: str, flags: List[str], # Make a copy to avoid modifying the caller's list self._lib_flags[lib] = flags[:] - def remove_lib_flags(self, lib: str): - '''Remove any flags configured for a standard library - - :param lib: the library name - ''' - try: - del self._lib_flags[lib] - except KeyError: - pass - def add_pre_lib_flags(self, flags: List[str]): '''Add a set of flags to use before any library-specific flags 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/tests/unit_tests/steps/test_link.py b/tests/unit_tests/steps/test_link.py index e9a6750c..dec2045a 100644 --- a/tests/unit_tests/steps/test_link.py +++ b/tests/unit_tests/steps/test_link.py @@ -14,7 +14,7 @@ from fab.artefacts import ArtefactSet, ArtefactStore from fab.steps.link import link_exe -from fab.tools import FortranCompiler, Linker +from fab.tools import FortranCompiler, Linker, ToolBox import pytest @@ -22,7 +22,7 @@ class TestLinkExe: '''Test class for linking an executable. ''' - def test_run(self, tool_box): + def test_run(self, tool_box: ToolBox): '''Ensure the command is formed correctly, with the flags at the end and that environment variable FFLAGS is picked up. ''' @@ -47,14 +47,13 @@ def test_run(self, tool_box): 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 # Add a custom library to the linker - linker.add_lib_flags('mylib', ['-L/my/lib', '-mylib']) + 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', @@ -68,6 +67,6 @@ def test_run(self, tool_box): tool_run.assert_called_with( ['mock_fortran_compiler.exe', '-L/foo1/lib', '-L/foo2/lib', 'bar.o', 'foo.o', - '-L/my/lib', '-mylib', '-fooflag', '-barflag', + '-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_compiler.py b/tests/unit_tests/tools/test_compiler.py index 60c18d78..fbbce38f 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(): diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index c5164017..2e517809 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -7,7 +7,7 @@ '''Tests the compiler wrapper implementation. ''' -from pathlib import Path, PosixPath +from pathlib import Path from unittest import mock import pytest @@ -179,10 +179,10 @@ def test_compiler_wrapper_fortran_with_add_args(): syntax_only=True) # Notice that "-J/b" has been removed mpif90.compiler.run.assert_called_with( - cwd=PosixPath('.'), additional_parameters=['-c', "-O3", - '-fsyntax-only', - '-J', '/module_out', - 'a.f90', '-o', 'a.o']) + 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(): @@ -199,7 +199,7 @@ def test_compiler_wrapper_fortran_with_add_args_unnecessary_openmp(): add_flags=["-fopenmp", "-O3"], openmp=True, syntax_only=True) mpif90.compiler.run.assert_called_with( - cwd=PosixPath('.'), + cwd=Path('.'), additional_parameters=['-c', '-fopenmp', '-fopenmp', '-O3', '-fsyntax-only', '-J', '/module_out', 'a.f90', '-o', 'a.o']) @@ -219,8 +219,8 @@ def test_compiler_wrapper_c_with_add_args(): mpicc.compile_file(Path("a.f90"), "a.o", openmp=False, add_flags=["-O3"]) mpicc.compiler.run.assert_called_with( - cwd=PosixPath('.'), additional_parameters=['-c', "-O3", - 'a.f90', '-o', 'a.o']) + 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: @@ -238,7 +238,7 @@ def test_compiler_wrapper_c_with_add_args(): add_flags=["-fopenmp", "-O3"], openmp=True) mpicc.compiler.run.assert_called_with( - cwd=PosixPath('.'), + cwd=Path('.'), additional_parameters=['-c', '-fopenmp', '-fopenmp', '-O3', 'a.f90', '-o', 'a.o']) @@ -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 @@ -289,7 +293,7 @@ def test_compiler_wrapper_flags_with_add_arg(): mpicc.compile_file(Path("a.f90"), "a.o", add_flags=["-f"], openmp=True) mpicc.compiler.run.assert_called_with( - cwd=PosixPath('.'), + cwd=Path('.'), additional_parameters=["-c", "-fopenmp", "-a", "-b", "-d", "-e", "-f", "a.f90", "-o", "a.o"]) @@ -308,7 +312,7 @@ def test_compiler_wrapper_flags_without_add_arg(): # Test if no add_flags are specified: mpicc.compile_file(Path("a.f90"), "a.o", openmp=True) mpicc.compiler.run.assert_called_with( - cwd=PosixPath('.'), + cwd=Path('.'), additional_parameters=["-c", "-fopenmp", "-a", "-b", "-d", "-e", "a.f90", "-o", "a.o"]) diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index cd2d8dc9..18f27bbd 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -177,20 +177,6 @@ def test_linker_add_lib_flags_overwrite_silent(mock_linker): assert result == ["-t", "-b"] -def test_linker_remove_lib_flags(mock_linker): - """Linker should provide a way to remove the flags for a library""" - mock_linker.remove_lib_flags("netcdf") - - with pytest.raises(RuntimeError) as err: - mock_linker.get_lib_flags("netcdf") - assert "Unknown library name: 'netcdf'" in str(err.value) - - -def test_linker_remove_lib_flags_unknown(mock_linker): - """Linker should silently allow removing flags for unknown library""" - mock_linker.remove_lib_flags("unknown") - - # ==================== # Linking: # ====================