More linker wrapper improvements (#383)

* Support new and old style of PSyclone command line (no more nemo api etc)

* Fix mypy errors.

* Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa.

* Updated comment.

* Removed mixing, use a simple regex instead.

* Added support for ifx/icx compiler as intel-llvm class.

* Added support for nvidia compiler.

* Add preliminary support for Cray compiler.

* Added Cray compiler wrapper ftn and cc.

* Follow a more consistent naming scheme for crays, even though the native compiler names are longer (crayftn-cray, craycc-cray).

* Changed names again.

* Renamed cray compiler wrapper to be CrayCcWrapper and CrayFtnWrapper, to avoid confusion with Craycc.

* Fixed incorrect name in comments.

* Additional compilers (#349)

* 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

* Removed mixing, use a simple regex instead.

* Added support for ifx/icx compiler as intel-llvm class.

* Added support for nvidia compiler.

* Add preliminary support for Cray compiler.

* Added Cray compiler wrapper ftn and cc.

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

* Removed white space.

* Follow a more consistent naming scheme for crays, even though the native compiler names are longer (crayftn-cray, craycc-cray).

* Changed names again.

* Support compilers that do not support OpenMP.

* Added documentation for openmp parameter.

* Renamed cray compiler wrapper to be CrayCcWrapper and CrayFtnWrapper, to avoid confusion with Craycc.

* Fixed incorrect name in comments.

---------

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

* Support new and old style of PSyclone command line (no more nemo api etc)

* Fix mypy errors.

* Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa.

* Added shell tool.

* Try to make mypy happy.

* Removed debug code.

* ToolRepository now only returns default that are available. Updated tests to make tools as available.

* Fixed typos and coding style.

* Support new and old style of PSyclone command line (no more nemo api etc)

* Fix mypy errors.

* Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa.

* Updated comment.

* Fixed failing tests.

* Updated fparser dependency to version 0.2.

* Replace old code for handling sentinels with triggering this behaviour in fparser. Require config in constructor of Analyser classes.

* Fixed tests for latest changes.

* Removed invalid openmp continuation line - since now fparser fails when trying to parse this line.

* Added test for disabled openmp parsing. Updated test to work with new test file.

* Coding style changes.

* Fix flake issues.

* Fixed double _.

* Make Linker inherit CompilerWrapper

* Fix up tests for new Linker inheritence

* Fix a flake error

* Use linker wrapping to combine flags from the wrapped linker with the linker wrapper.

* Minor code cleanup.

* Created linker wrapper in ToolRepository.

* Try making linker a CompilerSuiteTool instead of a CompilerWrapper.

* Updated tests.

* Fix support for post-libs.

* Fixed mypy.

* Removed more accesses to private members.

* Added missing type hint.

* Make flake8 happy.

* Added missing openmp handling in linker.

* Addressed issues raised in review.

* Forgot that file in previous commit.

* Updated linker to always require a compiler (previously it was actually never checked if the wrapped compiler actually exists, it only checked the linker).

* Remove unreliable test, since there is no guarantee that a C linker is returned, see issue #379.

* Fixed bug that a wrapper would not report openmp status based on the compiler.

* Added more description for this test.

---------

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

Author: 3 people

Diff for: source/fab/tools/compiler.py

@@ -76,7 +76,9 @@ def mpi(self) -> bool:
     def openmp(self) -> bool:
         ''':returns: if the compiler supports openmp or not
         '''
-        return self._openmp_flag != ""
+        # It is important not to use `_openmp_flag` directly, since a compiler
+        # wrapper overwrites `openmp_flag`.
+        return self.openmp_flag != ""
 
     @property
     def openmp_flag(self) -> str:

Diff for: source/fab/tools/linker.py

@@ -11,7 +11,7 @@
 
 import os
 from pathlib import Path
-from typing import cast, Dict, List, Optional, Union
+from typing import Dict, List, Optional, Union
 import warnings
 
 from fab.tools.category import Category
@@ -20,47 +20,35 @@
 
 
 class Linker(CompilerSuiteTool):
-    '''This is the base class for any Linker. It takes either another linker
-    instance, or a compiler instance as parameter in the constructor. Exactly
-    one of these must be provided.
-
-    :param compiler: an optional compiler instance
+    '''This is the base class for any Linker. It takes an existing compiler
+    instance as parameter, and optional another linker. The latter is used
+    to get linker settings - for example, linker-mpif90-gfortran will use
+    mpif90-gfortran as compiler (i.e. to test if it is available and get
+    compilation flags), and linker-gfortran as linker. This way a user
+    only has to specify linker flags in the most basic class (gfortran),
+    all other linker wrapper will inherit the settings.
+
+    :param compiler: a compiler instance
     :param linker: an optional linker instance
     :param name: name of the linker
 
     :raises RuntimeError: if both compiler and linker are specified.
     :raises RuntimeError: if neither compiler nor linker is specified.
     '''
 
-    def __init__(self, compiler: Optional[Compiler] = None,
+    def __init__(self, compiler: Compiler,
                  linker: Optional[Linker] = None,
-                 exec_name: Optional[str] = None,
                  name: Optional[str] = None):
 
-        if linker and compiler:
-            raise RuntimeError("Both compiler and linker is specified in "
-                               "linker constructor.")
-        if not linker and not compiler:
-            raise RuntimeError("Neither compiler nor linker is specified in "
-                               "linker constructor.")
         self._compiler = compiler
         self._linker = linker
 
-        search_linker = self
-        while search_linker._linker:
-            search_linker = search_linker._linker
-        final_compiler = search_linker._compiler
         if not name:
-            assert final_compiler   # make mypy happy
-            name = f"linker-{final_compiler.name}"
-
-        if not exec_name:
-            # This will search for the name in linker or compiler
-            exec_name = self.get_exec_name()
+            name = f"linker-{compiler.name}"
 
         super().__init__(
             name=name,
-            exec_name=exec_name,
+            exec_name=compiler.exec_name,
             suite=self.suite,
             category=Category.LINKER)
 
@@ -76,51 +64,31 @@ def check_available(self) -> bool:
         ''':returns: whether this linker is available by asking the wrapped
             linker or compiler.
         '''
-        if self._compiler:
-            return self._compiler.check_available()
-        assert self._linker   # make mypy happy
-        return self._linker.check_available()
-
-    def get_exec_name(self) -> str:
-        ''':returns: the name of the executable by asking the wrapped
-        linker or compiler.'''
-        if self._compiler:
-            return self._compiler.exec_name
-        assert self._linker   # make mypy happy
-        return self._linker.exec_name
+        return self._compiler.check_available()
 
     @property
     def suite(self) -> str:
         ''':returns: the suite this linker belongs to by getting it from
-            the wrapper compiler or linker.'''
-        return cast(CompilerSuiteTool, (self._compiler or self._linker)).suite
+            the wrapped compiler.'''
+        return self._compiler.suite
 
     @property
     def mpi(self) -> bool:
         ''':returns" whether this linker supports MPI or not by checking
-            with the wrapper compiler or linker.'''
-        if self._compiler:
-            return self._compiler.mpi
-        assert self._linker   # make mypy happy
-        return self._linker.mpi
+            with the wrapped compiler.'''
+        return self._compiler.mpi
 
     @property
     def openmp(self) -> bool:
-        ''':returns" whether this linker supports OpenMP or not by checking
-            with the wrapper compiler or linker.'''
-        if self._compiler:
-            return self._compiler.openmp
-        assert self._linker   # make mypy happy
-        return self._linker.openmp
+        ''':returns: whether this linker supports OpenMP or not by checking
+            with the wrapped compiler.'''
+        return self._compiler.openmp
 
     @property
     def output_flag(self) -> str:
         ''':returns: the flag that is used to specify the output name.
         '''
-        if self._compiler:
-            return self._compiler.output_flag
-        assert self._linker   # make mypy happy
-        return self._linker.output_flag
+        return self._compiler.output_flag
 
     def get_lib_flags(self, lib: str) -> List[str]:
         '''Gets the standard flags for a standard library
@@ -238,18 +206,10 @@ def link(self, input_files: List[Path], output_file: Path,
 
         params: List[Union[str, Path]] = []
 
-        # Find the compiler by following the (potentially
-        # layered) linker wrapper.
-        linker = self
-        while linker._linker:
-            linker = linker._linker
-        # Now we must have a compiler
-        compiler = linker._compiler
-        assert compiler   # make mypy happy
-        params.extend(compiler.flags)
+        params.extend(self._compiler.flags)
 
         if openmp:
-            params.append(compiler.openmp_flag)
+            params.append(self._compiler.openmp_flag)
 
         # TODO: why are the .o files sorted? That shouldn't matter
         params.extend(sorted(map(str, input_files)))

Diff for: source/fab/tools/tool_repository.py

@@ -117,19 +117,19 @@ def add_tool(self, tool: Tool):
             compiler = cast(Compiler, tool)
             if isinstance(compiler, CompilerWrapper):
                 # If we have a compiler wrapper, create a new linker using
-                # the linker based on the wrappped compiler. For example, when
+                # the linker based on the wrapped compiler. For example, when
                 # creating linker-mpif90-gfortran, we want this to be based on
-                # linker-gfortran (and not on the compiler mpif90-gfortran),
-                # since the linker-gfortran might have library definitions
-                # that should be reused. So we first get the existing linker
-                # (since the compiler exists, a linker for this compiler was
-                # already created and must exist).
+                # linker-gfortran. The compiler mpif90-gfortran will be the
+                # wrapper compiler. Reason is that e.g. linker-gfortran might
+                # have library definitions that should be reused. So we first
+                # get the existing linker (since the compiler exists, a linker
+                # for this compiler was already created and must exist).
                 other_linker = self.get_tool(
                     category=Category.LINKER,
                     name=f"linker-{compiler.compiler.name}")
                 other_linker = cast(Linker, other_linker)
-                linker = Linker(linker=other_linker,
-                                exec_name=compiler.exec_name,
+                linker = Linker(compiler,
+                                linker=other_linker,
                                 name=f"linker-{compiler.name}")
                 self[linker.category].append(linker)
             else:

Diff for: tests/unit_tests/tools/test_compiler_wrapper.py

@@ -257,6 +257,18 @@ 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 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
+
     # Adding flags to the wrapper should not affect the wrapped compiler:
     mpicc.add_flags(["-d", "-e"])
     assert gcc.flags == ["-a", "-b"]

Diff for: tests/unit_tests/tools/test_linker.py

@@ -41,28 +41,15 @@ def test_linker(mock_c_compiler, mock_fortran_compiler):
     assert linker.flags == []
 
 
-def test_linker_constructor_error(mock_c_compiler):
-    '''Test the linker constructor with invalid parameters.'''
-
-    with pytest.raises(RuntimeError) as err:
-        Linker()
-    assert ("Neither compiler nor linker is specified in linker constructor."
-            in str(err.value))
-    with pytest.raises(RuntimeError) as err:
-        Linker(compiler=mock_c_compiler, linker=mock_c_compiler)
-    assert ("Both compiler and linker is specified in linker constructor."
-            in str(err.value))
-
-
 @pytest.mark.parametrize("mpi", [True, False])
 def test_linker_mpi(mock_c_compiler, mpi):
     '''Test that linker wrappers handle MPI as expected.'''
 
     mock_c_compiler._mpi = mpi
-    linker = Linker(compiler=mock_c_compiler)
+    linker = Linker(mock_c_compiler)
     assert linker.mpi == mpi
 
-    wrapped_linker = Linker(linker=linker)
+    wrapped_linker = Linker(mock_c_compiler, linker=linker)
     assert wrapped_linker.mpi == mpi
 
 
@@ -80,7 +67,7 @@ def test_linker_openmp(mock_c_compiler, openmp):
     linker = Linker(compiler=mock_c_compiler)
     assert linker.openmp == openmp
 
-    wrapped_linker = Linker(linker=linker)
+    wrapped_linker = Linker(mock_c_compiler, linker=linker)
     assert wrapped_linker.openmp == openmp
 
 
@@ -103,7 +90,7 @@ def test_linker_check_available(mock_c_compiler):
 
     # Then test the usage of a linker wrapper. The linker will call the
     # corresponding function in the wrapper linker:
-    wrapped_linker = Linker(linker=linker)
+    wrapped_linker = Linker(mock_c_compiler, linker=linker)
     with mock.patch('fab.tools.compiler.Compiler.get_version',
                     return_value=(1, 2, 3)):
         assert wrapped_linker.check_available()
@@ -342,7 +329,7 @@ def test_linker_nesting(mock_c_compiler):
     linker1.add_lib_flags("lib_a", ["a_from_1"])
     linker1.add_lib_flags("lib_c", ["c_from_1"])
     linker1.add_post_lib_flags(["post_lib1"])
-    linker2 = Linker(linker=linker1)
+    linker2 = Linker(mock_c_compiler, linker=linker1)
     linker2.add_pre_lib_flags(["pre_lib2"])
     linker2.add_lib_flags("lib_b", ["b_from_2"])
     linker2.add_lib_flags("lib_c", ["c_from_2"])

Diff for: tests/unit_tests/tools/test_tool_repository.py

@@ -11,7 +11,7 @@
 import pytest
 
 from fab.tools import (Ar, Category, FortranCompiler, Gcc, Gfortran, Ifort,
-                       Linker, ToolRepository)
+                       ToolRepository)
 
 
 def test_tool_repository_get_singleton_new():
@@ -62,10 +62,6 @@ def test_tool_repository_get_default():
                               openmp=False)
     assert isinstance(gfortran, Gfortran)
 
-    gcc_linker = tr.get_default(Category.LINKER, mpi=False, openmp=False)
-    assert isinstance(gcc_linker, Linker)
-    assert gcc_linker.name == "linker-gcc"
-
     gcc = tr.get_default(Category.C_COMPILER, mpi=False, openmp=False)
     assert isinstance(gcc, Gcc)