Merge branch 'develop' into fix_contained_subroutines

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

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

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/psyclone.py

@@ -38,20 +38,10 @@ def check_available(self) -> bool:
 
         # First get the version (and confirm that PSyclone is installed):
         try:
-            # Older versions of PSyclone (2.3.1 and earlier) expect a filename
-            # even when --version is used, and won't produce version info
-            # without this. So provide a dummy file (which does not need to
-            # exist), and check the error for details to see if PSyclone does
-            # not exist, or if the error is because of the non-existing file
-            version_output = self.run(["--version", "does_not_exist"],
-                                      capture_output=True)
-        except RuntimeError as err:
-            # If the command is not found, the error contains the following:
-            if "could not be executed" in str(err):
-                return False
-            # Otherwise, psyclone likely complained about the not existing
-            # file. Continue and try to find version information in the output:
-            version_output = str(err)
+            version_output = self.run(["--version"], capture_output=True)
+        except RuntimeError:
+            # Something is wrong, report as not available
+            return False
 
         # Search for the version info:
         exp = r"PSyclone version: (\d[\d.]+\d)"
@@ -64,29 +54,8 @@ def check_available(self) -> bool:
 
         # Now convert the version info to integer. The regular expression
         # match guarantees that we have integer numbers now:
-        version = tuple(int(x) for x in matches.groups()[0].split('.'))
-
-        if version == (2, 5, 0):
-            # The behaviour of PSyclone changes from 2.5.0 to the next
-            # release. But since head-of-trunk still reports 2.5.0, we
-            # need to run additional tests to see if we have the official
-            # 2.5.0 release, or current trunk (which already has the new
-            # command line options). PSyclone needs an existing file
-            # in order to work, so use __file__ to present this file.
-            # PSyclone will obviously abort since this is not a Fortran
-            # file, but we only need to check the error message to
-            # see if the domain name is incorrect (--> current trunk)
-            # or not (2.5.0 release)
-            try:
-                self.run(["-api", "nemo", __file__], capture_output=True)
-            except RuntimeError as err:
-                if "Unsupported PSyKAL DSL / API 'nemo' specified" in str(err):
-                    # It is current development. Just give it a version number
-                    # greater than 2.5.0 for now, till the official release
-                    # is done.
-                    version = (2, 5, 0, 1)
-
-        self._version = version
+        self._version = tuple(int(x) for x in matches.groups()[0].split('.'))
+
         return True
 
     def process(self,
@@ -156,7 +125,7 @@ def process(self,
         # transformation tool only, so calling PSyclone without api is
         # actually valid.
         if api:
-            if self._version > (2, 5, 0):
+            if self._version >= (3, 0, 0):
                 api_param = "--psykal-dsl"
                 # Mapping from old names to new names:
                 mapping = {"dynamo0.3": "lfric",
@@ -176,7 +145,7 @@ def process(self,
             # Make mypy happy - we tested above that transformed_file is
             # specified when no api is specified.
             assert transformed_file
-            if self._version > (2, 5, 0):
+            if self._version >= (3, 0, 0):
                 # New version: no API, parameter, but -o for output name:
                 parameters.extend(["-o", transformed_file])
             else:

tests/unit_tests/steps/test_link.py

@@ -14,18 +14,19 @@
 
 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.
         '''
+
         config = SimpleNamespace(
             project_workspace=Path('workspace'),
             artefact_store=ArtefactStore(),
@@ -47,14 +48,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 +68,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():