Merge branch 'more_linker_wrapper_improvements' into unify_display_of…

…_compiler_wrapper

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
 -------------------

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,

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))

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.

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)"))

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:

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
 

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

tests/unit_tests/steps/test_link.py

@@ -14,15 +14,15 @@
 
 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
 
 
 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)

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():

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"])