Make compiler raise an error for any invalid version string

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

source/fab/tools/compiler.py

@@ -93,30 +93,25 @@ def check_available(self) -> bool:
             this by requesting the compiler version.
         '''
         try:
-            version = self.get_version()
+            self.get_version()
+            # A valid version means the compiler is available.
+            return True
         except RuntimeError:
-            # Compiler does not exist:
+            # Compiler does not exist, or version could not be handled:
             return False
 
-        # An empty tuple is returned if some other error occurred when trying
-        # to get the compiler version.
-        return version != ()
-
     def get_version(self):
         """
         Try to get the version of the given compiler.
 
-        # TODO: an empty tuple is returned for an invalid version, so that the
-        #       compiler can still be hashed. Is that necessary?
-
         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 tuple of integers representing the version string,
-            e.g (6, 10, 1) for version '6.10.1', or an empty tuple if a
-            different error happened when trying to get the compiler 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 invalid version string.
         """
         if self._version is not None:
             return self._version
@@ -126,37 +121,32 @@ def get_version(self):
         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 ()
+            raise RuntimeError(f"Error asking for version of compiler "
+                               f"'{self.name}': {err}")
 
         # 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 ")".
         try:
             version_string = 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, ...]
-        split = version_string.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_string}")
-            return ()
+            raise RuntimeError(f"Unexpected version response from compiler "
+                               f"'{self.name}': {res}")
 
         # expect the parts to be integers
         # todo: Not all will be integers? but perhaps major and minor?
         try:
-            version = tuple(int(x) for x in split)
+            version = tuple(int(x) for x in version_string.split('.'))
         except ValueError:
-            self.logger.warning(f"Unhandled compiler version for compiler "
-                                f"'{self.name}' should be numeric "
-                                f"<n.n[.n, ...]>: {version_string}")
-            return ()
+            raise RuntimeError(f"Unhandled compiler version format for "
+                               f"compiler '{self.name}'. Should be numeric "
+                               f"<n.n[.n, ...]>: {version_string}")
+
+        # expect at least 2 components, i.e. major.minor[.patch, ...]
+        if len(version) < 2:
+            raise RuntimeError(f"Unhandled compiler version format for "
+                               f"compiler '{self.name}'. Should have format "
+                               f"<n.n[.n, ...]>: {version_string}")
 
         self.logger.info(f'Found compiler version for {self.name} = {version_string}')
         self._version = version

tests/unit_tests/tools/test_compiler.py

@@ -46,23 +46,13 @@ def test_available():
 
 
 def test_available_after_error():
-    ''' Check the compiler is not available when get_version raises an
-    error.
+    ''' Check the compiler is not available when get_version raises an error.
     '''
     cc = CCompiler("gcc", "gcc", "gnu")
     with mock.patch.object(cc, "get_version", side_effect=RuntimeError("")):
         assert not cc.check_available()
 
 
-def test_unavailable_when_version_missing():
-    ''' Check the compiler is not available when get_version returns an
-    empty version.
-    '''
-    cc = CCompiler("gcc", "gcc", "gnu")
-    with mock.patch.object(cc, "_version", tuple()):
-        assert not cc.check_available()
-
-
 def test_compiler_hash():
     '''Test the hash functionality.'''
     cc = CCompiler("gcc", "gcc", "gnu")
@@ -81,14 +71,24 @@ def test_compiler_hash():
     assert hash3 not in (hash1, hash2)
 
 
-# TODO: Do we need to support this, or can it raise an error?
-def test_compiler_hash_missing_version():
+def test_compiler_hash_compiler_error():
+    '''Test the hash functionality when version info is missing.'''
+    cc = CCompiler("gcc", "gcc", "gnu")
+
+    # raise an error when trying to get compiler version
+    with mock.patch.object(cc, 'run', side_effect=RuntimeError()):
+        with pytest.raises(RuntimeError):
+            cc.get_hash()
+
+
+def test_compiler_hash_invalid_version():
     '''Test the hash functionality when version info is missing.'''
     cc = CCompiler("gcc", "gcc", "gnu")
-    # Return an empty tuple from get_version()
-    with mock.patch.object(cc, "_version", tuple()):
-        hash1 = cc.get_hash()
-        assert hash1 == 682757169
+
+    # returns an invalid compiler version string
+    with mock.patch.object(cc, "run", mock.Mock(return_value='foo v1')):
+        with pytest.raises(RuntimeError):
+            cc.get_hash()
 
 
 def test_compiler_with_env_fflags():
@@ -154,12 +154,22 @@ def test_compiler_with_add_args():
 class TestGetCompilerVersion:
     '''Test `get_version`.'''
 
+    def _check_error(self, full_version_string: str, expected_error: str):
+        '''Checks if the correct error is raised from the given invalid
+        full_version_string.
+        '''
+        c = Compiler("gfortran", "gfortran", "gnu", Category.FORTRAN_COMPILER)
+        with mock.patch.object(c, "run",
+                               mock.Mock(return_value=full_version_string)):
+            with pytest.raises(RuntimeError) as err:
+                c.get_version()
+            assert expected_error in str(err.value)
+
     def _check(self, full_version_string: str, expected: str):
-        '''Checks if the correct version is extracted from the
-        given full_version_string.
+        '''Checks if the correct version is extracted from the given
+        full_version_string.
         '''
-        c = Compiler("gfortran", "gfortran", "gnu",
-                     Category.FORTRAN_COMPILER)
+        c = Compiler("gfortran", "gfortran", "gnu", Category.FORTRAN_COMPILER)
         with mock.patch.object(c, "run",
                                mock.Mock(return_value=full_version_string)):
             assert c.get_version() == expected
@@ -170,12 +180,12 @@ def _check(self, full_version_string: str, expected: str):
             assert c.get_version() == expected
 
     def test_command_failure(self):
-        '''If the version command fails, we must return an empty tuple, not
-        None, so it can still be hashed.'''
+        '''If the version command fails, we must raise an error.'''
         c = Compiler("gfortran", "gfortran", "gnu",
                      Category.FORTRAN_COMPILER)
         with mock.patch.object(c, 'run', side_effect=RuntimeError()):
-            assert c.get_version() == (), 'expected empty tuple'
+            with pytest.raises(RuntimeError):
+                c.get_version()
 
     def test_file_not_found(self):
         '''If the compiler is not found, we must raise an error.'''
@@ -188,35 +198,51 @@ def test_file_not_found(self):
 
     def test_unknown_command_response(self):
         '''If the full version output is in an unknown format,
-        we must return an empty tuple.'''
-        self._check(full_version_string='foo fortran 1.2.3', expected=())
+        we must raise an error.'''
+        full_version_string = 'foo fortran 1.2.3'
+        expected_error = "Unexpected version response from compiler 'gfortran'"
+        self._check_error(
+            full_version_string=full_version_string,
+            expected_error=expected_error
+        )
 
     def test_unknown_version_format(self):
-        '''If the version is in an unknown format, we must return an
-        empty tuple.'''
+        '''If the version is in an unknown format, we must raise an error.'''
+
         full_version_string = dedent("""
             Foo Fortran (Foo) 5 123456 (Foo Hat 4.8.5-44)
             Copyright (C) 2022 Foo Software Foundation, Inc.
         """)
-        self._check(full_version_string=full_version_string, expected=())
+        expected_error = "Unhandled compiler version format for compiler 'gfortran'"
+        self._check_error(
+            full_version_string=full_version_string,
+            expected_error=expected_error
+        )
 
     def test_non_int_version_format(self):
-        '''If the version contains non-number characters, we must return an
-        empty tuple.'''
+        '''If the version contains non-number characters, we must raise an error.'''
         full_version_string = dedent("""
             Foo Fortran (Foo) 5.1f.2g (Foo Hat 4.8.5)
             Copyright (C) 2022 Foo Software Foundation, Inc.
         """)
-        self._check(full_version_string=full_version_string, expected=())
+        expected_error = "Unhandled compiler version format for compiler 'gfortran'"
+        self._check_error(
+            full_version_string=full_version_string,
+            expected_error=expected_error
+        )
 
     def test_1_part_version(self):
         '''If the version is just one integer, that is invalid and we must
-        return an empty tuple. '''
+        raise an error. '''
         full_version_string = dedent("""
             Foo Fortran (Foo) 77
             Copyright (C) 2022 Foo Software Foundation, Inc.
         """)
-        self._check(full_version_string=full_version_string, expected=())
+        expected_error = "Unhandled compiler version format for compiler 'gfortran'"
+        self._check_error(
+            full_version_string=full_version_string,
+            expected_error=expected_error
+        )
 
     def test_2_part_version(self):
         '''Test major.minor format. '''