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/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'] 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 a53fc434..ea3f1e77 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 @@ -80,7 +80,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 def get_hash(self) -> int: @@ -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)")) diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index 09ce5015..3c5d2997 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -148,6 +148,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 8959b3de..b0536b69 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -104,16 +104,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/tests/unit_tests/steps/test_link.py b/tests/unit_tests/steps/test_link.py index a20c4ff4..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'), @@ -36,7 +44,7 @@ def test_run(self, 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', @@ -44,10 +52,11 @@ 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', - '-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_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index 07f9a08b..bcf8d4a7 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']) @@ -281,7 +281,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"]) @@ -300,7 +300,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 6984c790..243f0c7a 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -160,20 +160,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: # ====================