Properly handle compiler versions (MetOffice#334)

* Removed unused glob import. * Fixed typo picked up in review. * Fixed failing tests. * Replaced _artefact_store with getter as much as possible. * #3 Introduce Tool, Compiler, and ToolRepository. * #3 Made ToolRepository a singleton. * #3 Added ToolBox. * #3 Added dedicated enum for categories. * #3 Add the category to a tool. * #3 Use tool category when adding a tool to the tool box. * #3 Added preprocessor, and dictionary-like accesses to ToolBox. * #3 Added tool_box as mandatory parameter for a BuildConfig. * #3 Start to use new compiler objects for Fortran compilation. * #3 Added tests for Compiler class. * #3 Add specific C- and Fortran-compiler classes. Updated tests. * #3 Fixed typing. * #3 Moved function to remove into Flags object, added test file for Flags. * # Fixed tests to work with new Fortran compiler handling. * #3 Use for compiler variables if defined. * #3 Add is_available flag to tools, added types. * #3 Added cpp as Fortran preprocessor. * #3 Removed unnecessary code. * #3 Properly ignore mypy warnings. * Support ToolBox for preprocessing. * #3 Add a specific 'preprocess' method to Preprocessor. * #3 Changed order in which compiler flags are used. * #3 Use toolbox in compile_c step. * #3 Added get_hash function to compiler. * #3 Added conftest.py file (with a fixture to create a C-compiler). * #3 Introduce fixtures for Fortran compiler and tool_box. * #3 Remove explicit compiler information from MpCommonArgs (since it's already part of the config toolbox). * #3 Added linker as tool. * #3 Added test for linking shared libraries. * #3 Pass compiler flags to the linker if a compiler was specified. * #3 Remove unused function. * #3 Removed more unused code. * #3 Automatically add a linker for each compiler. * #3 Fixed typo. * #3 Support vendor for compiler and linker. * #3 Make linker having a vendor, too. * #3 Add set_default_vendor method to tool repository. * Ignore build directory for git. * Updated test. * # Fix some mypy errors and warnings. * Avoid using get() for singleton, instead use __new__ which makes mypy happier. * Changed the transformation_script parameter of function psyclone to accept a function that can return file-specific transformation scripts * Make mypy happy by using patch.object. * Remove more comments and confusion about mypy :) * Try to make mypy happy on older python versions. * Make flake8 happy. * Sort imported name alphabetically. * Try to fix failing hash test (and add some additional improvements in the test). * Removed fpath= for input transformation_script function to pass mypy test for Python 3.7; Moved transformation_script_hash test to unit test from system test * Fix mypy typing check errors for psyclone unit test * Fix config typing issue with mypy in psyclone unit test * Fix flake8 issues; Revert Config mypy typing fix * Add comment to ignore typing check for fpath parameter of input transformation_script function * Fix assert check after transformation_script function is changed from being called twice to once * Filter out 'no transformation script' warning for psyclone system test * Replace 'ignore' typing of fpath of transformation_script with removing keyword argument * #3 Support proper tests to check if tools are available. * 1. Updated transformation_script description; 2. Modified mock_transformation_script; 3.Removed redundant _analysis_for_prebuilds * Updated lfric/atm.py and lfric/gungho.py examples to pass in transformation_script functions * Added description for the psyclone step to instructions on writing a config * #3 Added git as a tool. * #3 Fix incorrect | usage in typing. * #3 Added unit tests for git. * #3 Renamed git.py to versioning.py, to avoid name clash with the corresponding test_git.py tests. * #3 Converted svn and fcm to tools. * #3 Fixed missing whitespace. * Modified the documentation for writing a config with PSyclone * Add config as a parameter for run_psyclone for the transformation_script to use;Updated the related functions and tests; Changed the logic of the transformation_script examples * #3 Replaced ar with tool object. * #3 Added tests for ar.py. * #3 Removed debug output. * #3 Converted PSyclone to be a tool. * #3 Removed debug print, fixed python 3.7 typing information. * #3 Updated comments. * Modified the get_optimisation_script function examples and updated the doc formatting * #3 Add Rsync tool. * #3 Removed now unused function. * #3 Added test for rsync. * #3 Fixed all mypy warnings about functions not checked. * #3 Replace all mock-tests to use subprocess so the name of the executable is tested as well. * #3 Remove duplicated flags. * #3 Fixed changed order of linking. * #3 Removed run_command function. * #3 Fixed 3.8 typing error. * #3 Fixed unused imports. * #3 Move flags checksum into Flags, and remove now unused tools.py file. * #3 Renamed newtools to tools. * #3 Made custom function for all git functions called (instead of just calling run). * #3 Updated and fixed comments. * #3 Fixed errors in comments. * Fixed minor errors in documentation. * #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 * 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 * 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 * Remove inheritance from mixins and use protocol * Simplify compiler inheritance Mixins have static methods with unique names, overrides only happen in concrete classes * Simplify usage of compiler-specific parsing mixins. * Test for missing mixin. * Fixed test. * 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 test. --------- Co-authored-by: Joerg Henrichs <[email protected]> Co-authored-by: Junwei Lyu <[email protected]> Co-authored-by: jasonjunweilyu <[email protected]> Co-authored-by: Junwei Lyu <[email protected]> Co-authored-by: Matthew Hambley <[email protected]> Co-authored-by: Junwei Lyu <[email protected]> Co-authored-by: Junwei Lyu <[email protected]> Co-authored-by: Junwei Lyu <[email protected]> Co-authored-by: Joerg Henrichs <[email protected]> Co-authored-by: Luke Hoffmann <[email protected]>
hiker · Aug 19, 2024 · d0a92e8 · d0a92e8
1 parent 546ee9e
commit d0a92e8
Show file tree

Hide file tree

Showing 7 changed files with 590 additions and 227 deletions.
diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py
@@ -121,7 +121,8 @@ def handle_compiler_args(config: BuildConfig, common_flags=None,
     if not isinstance(compiler, FortranCompiler):
         raise RuntimeError(f"Unexpected tool '{compiler.name}' of type "
                            f"'{type(compiler)}' instead of FortranCompiler")
-    logger.info(f'Fortran compiler is {compiler} {compiler.get_version()}')
+    logger.info(
+        f'Fortran compiler is {compiler} {compiler.get_version_string()}')
 
     # Collate the flags from 1) flags env and 2) parameters.
     env_flags = os.getenv('FFLAGS', '').split()

diff --git a/source/fab/tools/__init__.py b/source/fab/tools/__init__.py
@@ -10,7 +10,8 @@
 from fab.tools.ar import Ar
 from fab.tools.category import Category
 from fab.tools.compiler import (CCompiler, Compiler, FortranCompiler, Gcc,
-                                Gfortran, Icc, Ifort)
+                                Gfortran, GnuVersionHandling, Icc, Ifort,
+                                IntelVersionHandling)
 from fab.tools.flags import Flags
 from fab.tools.linker import Linker
 from fab.tools.psyclone import Psyclone
@@ -36,8 +37,10 @@
            "Gcc",
            "Gfortran",
            "Git",
+           "GnuVersionHandling",
            "Icc",
            "Ifort",
+           "IntelVersionHandling",
            "Linker",
            "Preprocessor",
            "Psyclone",

diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py
@@ -9,8 +9,9 @@
 """
 
 import os
+import re
 from pathlib import Path
-from typing import List, Optional, Union
+from typing import List, Optional, Tuple, Union
 import zlib
 
 from fab.tools.category import Category
@@ -45,7 +46,7 @@ def __init__(self, name: str,
                  output_flag: Optional[str] = None,
                  omp_flag: Optional[str] = None):
         super().__init__(name, exec_name, suite, category)
-        self._version = None
+        self._version: Union[Tuple[int, ...], None] = None
         self._compile_flag = compile_flag if compile_flag else "-c"
         self._output_flag = output_flag if output_flag else "-o"
         self._omp_flag = omp_flag
@@ -55,7 +56,7 @@ def get_hash(self) -> int:
         ''':returns: a hash based on the compiler name and version.
         '''
         return (zlib.crc32(self.name.encode()) +
-                zlib.crc32(str(self.get_version()).encode()))
+                zlib.crc32(self.get_version_string().encode()))
 
     def compile_file(self, input_file: Path, output_file: Path,
                      add_flags: Union[None, List[str]] = None):
@@ -92,68 +93,95 @@ def check_available(self) -> bool:
             this by requesting the compiler version.
         '''
         try:
-            version = self.get_version()
-        except RuntimeError:
-            # Compiler does not exist:
+            self.get_version()
+            # A valid version means the compiler is available.
+            return True
+        except RuntimeError as err:
+            # Compiler does not exist, or version could not be handled:
+            self.logger.error(f'Error getting compiler version: {err}')
             return False
 
-        # An empty string is returned if some other error occurred when trying
-        # to get the compiler version.
-        return version != ""
-
-    def get_version(self):
+    def get_version(self) -> Tuple[int, ...]:
         """
         Try to get the version of the given compiler.
-        # TODO: why return "" when an error happened?
-        # TODO: we need to properly create integers for compiler versions
-        #       to (later) allow less and greater than comparisons.
 
         Expects a version in a certain part of the --version output,
         which must adhere to the n.n.n format, with at least 2 parts.
 
-        :Returns: a version string, e.g '6.10.1', or empty string if
-            a different error happened when trying to get the compiler version.
+        :returns: a tuple of at least 2 integers, representing the version
+            e.g. (6, 10, 1) for version '6.10.1'.
 
-        :raises RuntimeError: if the compiler was not found.
+        :raises RuntimeError: if the compiler was not found, or if it returned
+            an unrecognised output from the version command.
         """
-        if self._version:
+        if self._version is not None:
             return self._version
 
-        try:
-            res = self.run("--version", capture_output=True)
-        except FileNotFoundError as err:
-            raise RuntimeError(f'Compiler not found: {self.name}') from err
-        except RuntimeError as err:
-            self.logger.warning(f"Error asking for version of compiler "
-                                f"'{self.name}': {err}")
-            return ''
+        # Run the compiler to get the version and parse the output
+        # The implementations depend on vendor
+        output = self.run_version_command()
+        version_string = self.parse_version_output(self.category, output)
 
-        # Pull the version string from the command output.
-        # All the versions of gfortran and ifort we've tried follow the
-        # same pattern, it's after a ")".
+        # Expect the version to be dot-separated integers.
+        # todo: Not all will be integers? but perhaps major and minor?
         try:
-            version = res.split(')')[1].split()[0]
-        except IndexError:
-            self.logger.warning(f"Unexpected version response from "
-                                f"compiler '{self.name}': {res}")
-            return ''
-
-        # expect major.minor[.patch, ...]
-        # validate - this may be overkill
-        split = version.split('.')
-        if len(split) < 2:
-            self.logger.warning(f"unhandled compiler version format for "
-                                f"compiler '{self.name}' is not "
-                                f"<n.n[.n, ...]>: {version}")
-            return ''
-
-        # todo: do we care if the parts are integers? Not all will be,
-        # but perhaps major and minor?
-
-        self.logger.info(f'Found compiler version for {self.name} = {version}')
+            version = tuple(int(x) for x in version_string.split('.'))
+        except ValueError as err:
+            raise RuntimeError(f"Unexpected version output format for "
+                               f"compiler '{self.name}'. Should be numeric "
+                               f"<n.n[.n, ...]>: {version_string}") from err
+
+        # Expect at least 2 integer components, i.e. major.minor[.patch, ...]
+        if len(version) < 2:
+            raise RuntimeError(f"Unexpected version output format for "
+                               f"compiler '{self.name}'. Should have at least "
+                               f"two parts, <n.n[.n, ...]>: {version_string}")
+
+        self.logger.info(
+            f'Found compiler version for {self.name} = {version_string}')
         self._version = version
         return version
 
+    def run_version_command(
+            self, version_command: Optional[str] = '--version') -> str:
+        '''
+        Run the compiler's command to get its version.
+
+        :param version_command: The compiler argument used to get version info.
+
+        :returns: The output from the version command.
+
+        :raises RuntimeError: if the compiler was not found, or raised an
+            error.
+        '''
+        try:
+            return self.run(version_command, capture_output=True)
+        except RuntimeError as err:
+            raise RuntimeError(f"Error asking for version of compiler "
+                               f"'{self.name}'") from err
+
+    def parse_version_output(self, category: Category,
+                             version_output: str) -> str:
+        '''
+        Extract the numerical part from the version output.
+        Implemented in specific compilers.
+        '''
+        raise NotImplementedError("The method `parse_version_output` must be "
+                                  "provided using a mixin.")
+
+    def get_version_string(self) -> str:
+        """
+        Get a string representing the version of the given compiler.
+
+        :returns: a string of at least 2 numeric version components,
+            i.e. major.minor[.patch, ...]
+
+        :raises RuntimeError: if the compiler was not found, or if it returned
+            an unrecognised output from the version command.
+        """
+        version = self.get_version()
+        return '.'.join(str(x) for x in version)
+
 
 # ============================================================================
 class CCompiler(Compiler):
@@ -163,7 +191,6 @@ class CCompiler(Compiler):
     :param name: name of the compiler.
     :param exec_name: name of the executable to start.
     :param suite: name of the compiler suite.
-    :param category: the Category (C_COMPILER or FORTRAN_COMPILER).
     :param compile_flag: the compilation flag to use when only requesting
         compilation (not linking).
     :param output_flag: the compilation flag to use to indicate the name
@@ -251,7 +278,45 @@ def compile_file(self, input_file: Path, output_file: Path,
 
 
 # ============================================================================
-class Gcc(CCompiler):
+class GnuVersionHandling():
+    '''Mixin to handle version information from GNU compilers'''
+
+    def parse_version_output(self, category: Category,
+                             version_output: str) -> str:
+        '''
+        Extract the numerical part from a GNU compiler's version output
+
+        :param name: the compiler's name
+        :param category: the compiler's Category
+        :param version_output: the full version output from the compiler
+        :returns: the actual version as a string
+
+        :raises RuntimeError: if the output is not in an expected format.
+        '''
+
+        # Expect the version to appear after some in parentheses, e.g.
+        # "GNU Fortran (...) n.n[.n, ...]" or # "gcc (...) n.n[.n, ...]"
+        if category is Category.FORTRAN_COMPILER:
+            name = "GNU Fortran"
+        else:
+            name = "gcc"
+        # A version number is a digit, followed by a sequence of digits and
+        # '.'', ending with a digit. It must then be followed by either the
+        # end of the string, or a space (e.g. "... 5.6 123456"). We can't use
+        # \b to determine the end, since then "1.2." would be matched
+        # excluding the dot (so it would become a valid 1.2)
+        exp = name + r" \(.*?\) (\d[\d\.]+\d)(?:$| )"
+        # Multiline is required in case that the version number is the
+        # end of the string, otherwise the $ would not match the end of line
+        matches = re.search(exp, version_output, re.MULTILINE)
+        if not matches:
+            raise RuntimeError(f"Unexpected version output format for "
+                               f"compiler '{name}': {version_output}")
+        return matches.groups()[0]
+
+
+# ============================================================================
+class Gcc(GnuVersionHandling, CCompiler):
     '''Class for GNU's gcc compiler.
 
     :param name: name of this compiler.
@@ -264,7 +329,7 @@ def __init__(self,
 
 
 # ============================================================================
-class Gfortran(FortranCompiler):
+class Gfortran(GnuVersionHandling, FortranCompiler):
     '''Class for GNU's gfortran compiler.
 
     :param name: name of this compiler.
@@ -280,7 +345,41 @@ def __init__(self,
 
 
 # ============================================================================
-class Icc(CCompiler):
+class IntelVersionHandling():
+    '''Mixin to handle version information from Intel compilers'''
+
+    def parse_version_output(self, category: Category,
+                             version_output: str) -> str:
+        '''
+        Extract the numerical part from an Intel compiler's version output
+
+        :param name: the compiler's name
+        :param version_output: the full version output from the compiler
+        :returns: the actual version as a string
+
+        :raises RuntimeError: if the output is not in an expected format.
+        '''
+
+        # Expect the version to appear after some in parentheses, e.g.
+        # "icc (...) n.n[.n, ...]" or "ifort (...) n.n[.n, ...]"
+        if category == Category.C_COMPILER:
+            name = "icc"
+        else:
+            name = "ifort"
+
+        # A version number is a digit, followed by a sequence of digits and
+        # '.'', ending with a digit. It must then be followed by a space.
+        exp = name + r" \(.*?\) (\d[\d\.]+\d) "
+        matches = re.search(exp, version_output)
+
+        if not matches:
+            raise RuntimeError(f"Unexpected version output format for "
+                               f"compiler '{name}': {version_output}")
+        return matches.groups()[0]
+
+
+# ============================================================================
+class Icc(IntelVersionHandling, CCompiler):
     '''Class for the Intel's icc compiler.
 
     :param name: name of this compiler.
@@ -294,7 +393,7 @@ def __init__(self,
 
 
 # ============================================================================
-class Ifort(FortranCompiler):
+class Ifort(IntelVersionHandling, FortranCompiler):
     '''Class for Intel's ifort compiler.
 
     :param name: name of this compiler.

diff --git a/tests/conftest.py b/tests/conftest.py
@@ -20,7 +20,7 @@ def fixture_mock_c_compiler():
     '''Provides a mock C-compiler.'''
     mock_compiler = CCompiler("mock_c_compiler", "mock_exec", "suite")
     mock_compiler.run = mock.Mock()
-    mock_compiler._version = "1.2.3"
+    mock_compiler._version = (1, 2, 3)
     mock_compiler._name = "mock_c_compiler"
     mock_compiler._exec_name = "mock_c_compiler.exe"
     return mock_compiler
@@ -36,7 +36,7 @@ def fixture_mock_fortran_compiler():
     mock_compiler.run = mock.Mock()
     mock_compiler._name = "mock_fortran_compiler"
     mock_compiler._exec_name = "mock_fortran_compiler.exe"
-    mock_compiler._version = "1.2.3"
+    mock_compiler._version = (1, 2, 3)
     return mock_compiler
 
 
@@ -46,7 +46,7 @@ def fixture_mock_linker():
     mock_linker = Linker("mock_linker", "mock_linker.exe",
                          Category.FORTRAN_COMPILER)
     mock_linker.run = mock.Mock()
-    mock_linker._version = "1.2.3"
+    mock_linker._version = (1, 2, 3)
     return mock_linker
 
 

diff --git a/tests/unit_tests/steps/test_compile_c.py b/tests/unit_tests/steps/test_compile_c.py
@@ -156,6 +156,6 @@ def test_change_compiler_version(self, content, flags):
         changes the hash.'''
         config, analysed_file, expect_hash = content
         compiler = config.tool_box[Category.C_COMPILER]
-        compiler._version = "9.8.7"
+        compiler._version = (9, 8, 7)
         result = _get_obj_combo_hash(compiler, analysed_file, flags)
         assert result != expect_hash
diff --git a/tests/unit_tests/steps/test_compile_fortran.py b/tests/unit_tests/steps/test_compile_fortran.py
@@ -376,7 +376,7 @@ def test_compiler_version_hash(self, content):
         # changing the compiler version must change the combo hash for the mods and obj
         mp_common_args, flags, analysed_file, orig_obj_hash, orig_mods_hash = content
         compiler = mp_common_args.config.tool_box[Category.FORTRAN_COMPILER]
-        compiler._version = "9.8.7"
+        compiler._version = (9, 8, 7)
 
         obj_combo_hash = '1a87f4e07'
         mods_combo_hash = '131edbafd'