Linker lib flags (#348)

* #3 Make it easier to create wrapper around standard compiler.

* #3 Added documentation for all tool related classes and their usage.

* #3 Added MISC category.

* Addressed reviewer's comments.

* Updated cli to properly use ToolBox etc, removing hard-coded gnu command linker option.

* Fixed mypy failures, including changes to import statement to avoid cyclic imports :(.

* #3 Fix circular import.

* Added #TODO so that this can be removed once fparser supports sentinels.

* Fix typing problems by ignoring fparser.

* Replaced more string names for artefacts with enums.

* Removed EXECUTABLES from constants.

* Moved Artefact class out of ArtefactStore and renamed it to ArtefactSet.

* Moved OBJECT_FILES from constants into ArtefactSet.

* Moved OBJECT_ARCHIVES from constants to ArtefactSet.

* Moved PRAGMAD_C from constants to ArtefactSet.

* Turned 'all_source' into an enum.

* Allow integer as revision.

* Fixed flake8 error.

* Removed specific functions to add/get fortran source files etc.

* Removed non-existing and unneccessary collections.

* Try to fix all run_configs.

* Fixed rebase issues.

* Added replace functionality to ArtefactStore, updated test_artefacts to cover all lines in that file.

* Started to replace artefacts when files are pre-processed.

* Removed linker argument from linking step in all examples.

* Try to get jules to link.

* Fixed build_jules.

* Fixed other issues raised in reviews.

* Try to get jules to link.

* Fixed other issues raised in reviews.

* Simplify handling of X90 files by replacing the X90 with x90, meaning only one artefact set is involved when running PSyclone.

* Make OBJECT_ARCHIVES also a dict, migrate more code to replace/add files to the default build artefact collections.

* Fixed some examples.

* Fix flake8 error.

* Fixed failing tests.

* Support empty comments.

* Fix preprocessor to not unnecessary remove and add files that are already in the output directory.

* Allow find_soure_files to be called more than once by adding files (not replacing artefact).

* Updated lfric_common so that files created by configurator are written in build (not source).

* Use c_build_files instead of pragmad_c.

* Removed unnecessary str.

* Documented the new artefact set handling.

* Fixed typo.

* Make the PSyclone API configurable.

* Fixed formatting of documentation, properly used ArtefactSet names.

* Support .f and .F Fortran files.

* Removed setter for tool.is_available, which was only used for testing.

* #3 Fix documentation and coding style issues from review.

* Renamed Categories into Category.

* Minor coding style cleanup.

* Removed more unnecessary ().

* Re-added (invalid) grab_pre_build call.

* Fixed typo.

* Renamed set_default_vendor to set_default_compiler_suite.

* Renamed VendorTool to CompilerSuiteTool.

* Also accept a Path as exec_name specification for a tool.

* Move the check_available function into the base class.

* Fixed some types and documentation.

* Fix typing error.

* Added explanation for meta-compiler.

* Improved error handling and documentation.

* Replace mpiifort with mpifort to be a tiny bit more portable.

* Use classes to group tests for git/svn/fcm together.

* Fixed issue in get_transformation script, and moved script into lfric_common to remove code duplication.

* Code improvement as suggested by review.

* Fixed run config

* Added reference to ticket.

* Updated type information.

* More typing fixes.

* Fixed typing warnings.

* As requested by reviewer removed is_working_copy functionality.

* Issue a warning (which can be silenced) when a tool in a toolbox is replaced.

* Fixed flake8.

* Fixed flake8.

* Fixed failing test.

* Addressed issues raised in review.

* Removed now unnecessary operations.

* Updated some type information.

* Fixed all references to APIs to be consistent with PSyclone 2.5.

* Added api to the checksum computation.

* Fixed type information.

* Added test to verify that changing the api changes the checksum.

* Make compiler version a tuple of integers

* Update some tests to use tuple versions

* Explicitly test handling of bad version format

* Fix formatting

* Tidying up

* Make compiler raise an error for any invalid version string

Assume these compilers don't need to be hashed.
Saves dealing with empty tuples.

* Check compiler version string for compiler name

* Fix formatting

* Add compiler.get_version_string() method

Includes other cleanup from PR comments

* Add mpi and openmp settings to BuildConfig, made compiler MPI aware.

* Looks like the circular dependency has been fixed.

* Revert "Looks like the circular dependency has been fixed." ...
while it works with the tests, a real application still triggered it.

This reverts commit 150dc37.

* Don't even try to find a C compiler if no C files are to be compiled.

* Updated gitignore to ignore (recently renamed) documentation.

* Fixed failing test.

* Return from compile Fortran early if there are no files to compiles. Fixed coding style.

* Add MPI enables wrapper for intel and gnu compiler.

* Fixed test.

* Automatically add openmp flag to compiler and linker based on BuildConfig.

* Removed enforcement of keyword parameters, which is not supported in python 3.7.

* Fixed failing test.

* Support more than one tool of a given suite by sorting them.

* Use different version checkout for each compiler vendor with mixins

* Refactoring, remove unittest compiler class

* Fix some mypy errors

* Use 'Union' type hint to fix build checks

* Added option to add flags to a tool.

* Introduce proper compiler wrapper, used this to implement properly wrapper MPI compiler.

* Fixed typo in types.

* Return run_version_command to base Compiler class

Provides default version command that can be overridden for other compilers.
Also fix some incorrect tests
Other tidying

* Add a missing type hint

* Added (somewhat stupid) 'test' to reach 100% coverage of PSyclone tool.

* Simplified MPI support in wrapper.

* More compiler wrapper coverage.

* Removed duplicated function.

* Removed debug print.

* Removed permanently changing compiler attributes, which can cause test failures later.

* More test for C compiler wrapper.

* More work on compiler wrapper tests.

* Fixed version and availability handling, added missing tests for 100% coverage.

* Fixed typing error.

* Try to fix python 3.7.

* Tried to fix failing tests.

* Remove inheritance from mixins and use protocol

* Simplify compiler inheritance

Mixins have static methods with unique names,
overrides only happen in concrete classes

* Updated wrapper and tests to handle error raised in get_version.

* Simplified regular expressions (now tests cover detection of version numbers with only a major version).

* Test for missing mixin.

* Use the parsing mixing from the compiler in a compiler wrapper.

* Use setattr instead of assignment to make mypy happy.

* Simplify usage of compiler-specific parsing mixins.

* Minor code cleanup.

* Updated documentation.

* Simplify usage of compiler-specific parsing mixins.

* Test for missing mixin.

* Fixed test.

* Added missing openmp_flag property to compiler wrapper.

* Don't use isinstance for consistency check, which does not work for CompilerWrappers.

* Fixed isinstance test for C compilation which doesn't work with a CompilerWrapper.

* Use a linker's compiler to determine MPI support. Removed mpi property from CompilerSuite.

* Added more tests for invalid version numbers.

* Added more test cases for invalid version number, improved regex to work as expected.

* Fixed typo in test.

* Fixed flake/mypy errors.

* Combine wrapper flags with flags from wrapped compiler.

* Made mypy happy.

* Fixed test.

* Split tests into smaller individual ones, fixed missing asssert in test.

* Parameterised compiler version tests to also test wrapper.

* Added missing MPI parameter when getting the compiler.

* Fixed comments.

* Order parameters to be in same order for various compiler classes.

* Remove stray character

* Added getter for wrapped compiler.

* Fixed small error that would prevent nested compiler wrappers from being used.

* Added a cast to make mypy happy.

* Add simple getter for linker library flags

* Add getter for linker flags by library

* Fix formatting

* Add optional libs argument to link function

* Reorder and clean up linker tests

* Make sure `Linker.link()` raises for unknown lib

* Add missing type

* Fix typing error

* Add 'libs' argument to link_exe function

* Try to add documentation for the linker libs feature

* Use correct list type in link_exe hint

* Add silent replace option to linker.add_lib_flags

* Fixed spelling mistake in option.

* Clarified documentation.

* Removed unnecessary functions in CompilerWrapper.

* Fixed failing test triggered by executing them in specific order (tools then steps)

* Fixed line lengths.

* Add tests for linker LDFLAG

* Add pre- and post- lib flags to link function

* Fix syntax in built-in lib flags

* Remove netcdf as a built-in linker library

Bash-style substitution is not currently handled

* Configure pre- and post-lib flags on the Linker object

Previously they were passed into the Linker.link() function

* Use more realistic linker lib flags

* Formatting fix

* Made mpi and openmp default to False in the BuildConfig constructor.

* Removed white space.

* Support compilers that do not support OpenMP.

* Added documentation for openmp parameter.

---------

Co-authored-by: jasonjunweilyu <[email protected]>
Co-authored-by: Luke Hoffmann <[email protected]>
Co-authored-by: Luke Hoffmann <[email protected]>

Documentation/source/advanced_config.rst

@@ -193,6 +193,14 @@ to specify 3rd party libraries at the link stage.
 
     link_exe(state, flags=['-lm', '-lnetcdf'])
 
+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.
+
+.. code-block::
+    :linenos:
+
+    link_exe(state, libs=['netcdf'])
+
 Path-specific flags
 -------------------
 

source/fab/steps/link.py

@@ -9,7 +9,7 @@
 """
 import logging
 from string import Template
-from typing import Optional
+from typing import List, Optional, Union
 
 from fab.artefacts import ArtefactSet
 from fab.steps import step
@@ -33,7 +33,10 @@ def __call__(self, artefact_store):
 
 
 @step
-def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None):
+def link_exe(config,
+             libs: Union[List[str], None] = None,
+             flags: Union[List[str], None] = None,
+             source: Optional[ArtefactsGetter] = None):
     """
     Link object files into an executable for every build target.
 
@@ -49,8 +52,10 @@ def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None):
         The :class:`fab.build_config.BuildConfig` object where we can read
         settings such as the project workspace folder or the multiprocessing
         flag.
+    :param libs:
+        A list of required library names to pass to the linker.
     :param flags:
-        A list of flags to pass to the linker.
+        A list of additional flags to pass to the linker.
     :param source:
         An optional :class:`~fab.artefacts.ArtefactsGetter`. It defaults to the
         output from compiler steps, which typically is the expected behaviour.
@@ -60,13 +65,15 @@ def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None):
                                       openmp=config.openmp)
     logger.info(f'Linker is {linker.name}')
 
-    flags = flags or []
+    libs = libs or []
+    if flags:
+        linker.add_post_lib_flags(flags)
     source_getter = source or DefaultLinkerSource()
 
     target_objects = source_getter(config.artefact_store)
     for root, objects in target_objects.items():
         exe_path = config.project_workspace / f'{root}'
-        linker.link(objects, exe_path, openmp=config.openmp, add_libs=flags)
+        linker.link(objects, exe_path, openmp=config.openmp, libs=libs)
         config.artefact_store.add(ArtefactSet.EXECUTABLES, exe_path)
 
 
@@ -116,4 +123,5 @@ def link_shared_object(config, output_fpath: str, flags=None,
 
     objects = target_objects[None]
     out_name = Template(output_fpath).substitute(output=config.build_output)
-    linker.link(objects, out_name, openmp=config.openmp, add_libs=flags)
+    linker.add_post_lib_flags(flags)
+    linker.link(objects, out_name, openmp=config.openmp)

source/fab/tools/linker.py

@@ -9,7 +9,8 @@
 
 import os
 from pathlib import Path
-from typing import cast, List, Optional
+from typing import cast, Dict, List, Optional
+import warnings
 
 from fab.tools.category import Category
 from fab.tools.compiler import Compiler
@@ -51,6 +52,12 @@ def __init__(self, name: Optional[str] = None,
         self._compiler = compiler
         self.flags.extend(os.getenv("LDFLAGS", "").split())
 
+        # Maintain a set of flags for common libraries.
+        self._lib_flags: Dict[str, List[str]] = {}
+        # Allow flags to include before or after any library-specific flags.
+        self._pre_lib_flags: List[str] = []
+        self._post_lib_flags: List[str] = []
+
     @property
     def mpi(self) -> bool:
         ''':returns: whether the linker supports MPI or not.'''
@@ -66,16 +73,71 @@ def check_available(self) -> bool:
 
         return super().check_available()
 
+    def get_lib_flags(self, lib: str) -> List[str]:
+        '''Gets the standard flags for a standard library
+
+        :param lib: the library name
+
+        :returns: a list of flags
+
+        :raises RuntimeError: if lib is not recognised
+        '''
+        try:
+            return self._lib_flags[lib]
+        except KeyError:
+            raise RuntimeError(f"Unknown library name: '{lib}'")
+
+    def add_lib_flags(self, lib: str, flags: List[str],
+                      silent_replace: bool = False):
+        '''Add a set of flags for a standard library
+
+        :param lib: the library name
+        :param flags: the flags to use with the library
+        :param silent_replace: if set, no warning will be printed when an
+            existing lib is overwritten.
+        '''
+        if lib in self._lib_flags and not silent_replace:
+            warnings.warn(f"Replacing existing flags for library {lib}: "
+                          f"'{self._lib_flags[lib]}' with "
+                          f"'{flags}'.")
+
+        # 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
+
+        :param flags: the flags to include
+        '''
+        self._pre_lib_flags.extend(flags)
+
+    def add_post_lib_flags(self, flags: List[str]):
+        '''Add a set of flags to use after any library-specific flags
+
+        :param flags: the flags to include
+        '''
+        self._post_lib_flags.extend(flags)
+
     def link(self, input_files: List[Path], output_file: Path,
              openmp: bool,
-             add_libs: Optional[List[str]] = None) -> str:
+             libs: Optional[List[str]] = None) -> str:
         '''Executes the linker with the specified input files,
         creating `output_file`.
 
         :param input_files: list of input files to link.
         :param output_file: output file.
         :param openm: whether OpenMP is requested or not.
-        :param add_libs: additional linker flags.
+        :param libs: additional libraries to link with.
 
         :returns: the stdout of the link command
         '''
@@ -88,7 +150,12 @@ def link(self, input_files: List[Path], output_file: Path,
             params = []
         # TODO: why are the .o files sorted? That shouldn't matter
         params.extend(sorted(map(str, input_files)))
-        if add_libs:
-            params += add_libs
+
+        if self._pre_lib_flags:
+            params.extend(self._pre_lib_flags)
+        for lib in (libs or []):
+            params.extend(self.get_lib_flags(lib))
+        if self._post_lib_flags:
+            params.extend(self._post_lib_flags)
         params.extend([self._output_flag, str(output_file)])
         return self.run(params)

tests/conftest.py

@@ -48,6 +48,7 @@ def fixture_mock_linker():
                          Category.FORTRAN_COMPILER)
     mock_linker.run = mock.Mock()
     mock_linker._version = (1, 2, 3)
+    mock_linker.add_lib_flags("netcdf", ["-lnetcdff", "-lnetcdf"])
     return mock_linker
 
 

tests/unit_tests/steps/test_archive_objects.py

@@ -81,20 +81,20 @@ def test_for_library(self):
         assert config.artefact_store[ArtefactSet.OBJECT_ARCHIVES] == {
             None: set([str(config.build_output / 'mylib.a')])}
 
-    def test_incorrect_tool(self):
+    def test_incorrect_tool(self, tool_box):
         '''Test that an incorrect archive tool is detected
         '''
 
-        config = BuildConfig('proj', ToolBox())
-        tool_box = config.tool_box
+        config = BuildConfig('proj', tool_box)
         cc = tool_box.get_tool(Category.C_COMPILER, config.mpi, config.openmp)
-        # And set its category to C_COMPILER
+        # And set its category to be AR
         cc._category = Category.AR
-        # So overwrite the C compiler with the re-categories Fortran compiler
+        # Now add this 'ar' tool to the tool box
         tool_box.add_tool(cc)
 
         with pytest.raises(RuntimeError) as err:
             archive_objects(config=config,
                             output_fpath=config.build_output / 'mylib.a')
-        assert ("Unexpected tool 'gcc' of type '<class "
-                "'fab.tools.compiler.Gcc'>' instead of Ar" in str(err.value))
+        assert ("Unexpected tool 'mock_c_compiler' of type '<class "
+                "'fab.tools.compiler.CCompiler'>' instead of Ar"
+                in str(err.value))

tests/unit_tests/steps/test_link.py

@@ -34,16 +34,20 @@ def test_run(self, tool_box):
             linker = Linker("mock_link", "mock_link.exe", "mock-vendor")
             # 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'])
             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',
                             return_value=mock_result) as tool_run, \
                     pytest.warns(UserWarning,
                                  match="_metric_send_conn not "
                                        "set, cannot send metrics"):
-                link_exe(config, 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',
-             '-fooflag', '-barflag', '-o', 'workspace/foo'],
+             '-L/my/lib', '-mylib', '-fooflag', '-barflag',
+             '-o', 'workspace/foo'],
             capture_output=True, env=None, cwd=None, check=False)