From 10678fafe528b74a90a9a4bc240b11da258954f8 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 4 Sep 2025 15:20:42 +1000 Subject: [PATCH 01/30] #313 First set of changes, introducing new Flag types. --- source/fab/build_config.py | 11 +- source/fab/steps/compile_c.py | 11 +- source/fab/steps/compile_fortran.py | 12 +- source/fab/tools/compiler.py | 46 +- source/fab/tools/compiler_wrapper.py | 25 +- source/fab/tools/flags.py | 411 ++++++++++++++++-- source/fab/tools/linker.py | 15 +- source/fab/tools/tool.py | 24 +- source/fab/util.py | 2 +- tests/unit_tests/steps/test_compile_c.py | 8 +- tests/unit_tests/test_build_config.py | 13 + .../unit_tests/tools/test_compiler_wrapper.py | 67 ++- tests/unit_tests/tools/test_flags.py | 210 +++++++-- tests/unit_tests/tools/test_linker.py | 20 +- tests/unit_tests/tools/test_tool.py | 12 +- .../unit_tests/tools/test_tool_repository.py | 4 + 16 files changed, 718 insertions(+), 173 deletions(-) diff --git a/source/fab/build_config.py b/source/fab/build_config.py index 0cc85915c..38ccea65d 100644 --- a/source/fab/build_config.py +++ b/source/fab/build_config.py @@ -91,9 +91,9 @@ def __init__(self, project_label: str, self._openmp = openmp if profile is None: # An empty string is used for non-profiled flags - self._profile = "" + self.set_profile("") else: - self._profile = profile + self.set_profile(profile) self.two_stage = two_stage self.verbose = verbose compiler = tool_box.get_tool(Category.FORTRAN_COMPILER, mpi=mpi, @@ -223,6 +223,13 @@ def profile(self) -> str: ''':returns: the name of the compiler profile to use.''' return self._profile + def set_profile(self, profile: str) -> None: + """Sets the compilation profile. + + :param profile: the name of the profile. + """ + self._profile = profile.lower() + def add_current_prebuilds(self, artefacts: Iterable[Path]): """ Mark the given file paths as being current prebuilds, not to be diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index a059b4345..fc08255c0 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -18,7 +18,8 @@ from fab.metrics import send_metric from fab.parse.c import AnalysedC from fab.steps import check_for_errors, run_mp, step -from fab.tools import Category, Compiler, Flags +from fab.tools.flags import Flags, ProfileFlags +from fab.tools import Category, Compiler from fab.util import CompiledFile, log_or_dot, Timer, by_type logger = logging.getLogger(__name__) @@ -129,8 +130,10 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): # 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, - config=config)) + f_f_p = mp_payload.flags.flags_for_path(path=analysed_file.fpath, + config=config) + flags = ProfileFlags() + flags.add_flags(f_f_p, config.profile) obj_combo_hash = _get_obj_combo_hash(config, compiler, analysed_file, flags) @@ -148,7 +151,7 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): try: compiler.compile_file(analysed_file.fpath, obj_file_prebuild, config=config, - add_flags=flags) + add_flags=f_f_p) except RuntimeError as err: return FabException(f"error compiling " f"{analysed_file.fpath}:\n{err}") diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index a3dc97ea3..072862676 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -22,6 +22,7 @@ from fab.parse.fortran import AnalysedFortran from fab.steps import check_for_errors, run_mp, step from fab.tools import Category, Compiler, Flags +from fab.tools.flags import ProfileFlags from fab.util import (CompiledFile, log_or_dot_finish, log_or_dot, Timer, by_type, file_checksum) @@ -270,8 +271,9 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ # 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)) + f_f_p = mp_common_args.flags.flags_for_path(path=analysed_file.fpath, + config=config) + flags = ProfileFlags(f_f_p, config.profile) mod_combo_hash = _get_mod_combo_hash(config, analysed_file, compiler=compiler) @@ -296,7 +298,7 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ # compile try: logger.debug(f'CompileFortran compiling {analysed_file.fpath}') - compile_file(analysed_file.fpath, flags, + compile_file(analysed_file.fpath, f_f_p, output_fpath=obj_file_prebuild, mp_common_args=mp_common_args) except Exception as err: @@ -355,7 +357,7 @@ def _get_obj_combo_hash(config: BuildConfig, analysed_file.file_hash, flags.checksum(), sum(mod_deps_hashes.values()), - compiler.get_hash(config.profile), + compiler.get_hash(config), ]) except TypeError as err: raise ValueError("Could not generate combo hash " @@ -368,7 +370,7 @@ def _get_mod_combo_hash(config, analysed_file, compiler: Compiler): try: mod_combo_hash = sum([ analysed_file.file_hash, - compiler.get_hash(config.profile), + compiler.get_hash(config), ]) except TypeError as err: raise ValueError("Could not generate combo " diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 1a3d1c4fe..86e60199f 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -15,7 +15,7 @@ import zlib from fab.tools.category import Category -from fab.tools.flags import Flags +from fab.tools.flags import AlwaysFlags from fab.tools.tool import CompilerSuiteTool if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -104,20 +104,14 @@ def output_flag(self) -> str: """ return self._output_flag - def get_hash(self, profile: Optional[str] = None) -> int: + def get_hash(self, config: Optional["BuildConfig"] = None) -> int: """ :returns: hash of compiler name and version. """ return (zlib.crc32(self.name.encode()) + - zlib.crc32(str(self.get_flags(profile)).encode()) + + zlib.crc32(str(self.get_flags(config)).encode()) + zlib.crc32(self.get_version_string().encode())) - def get_flags(self, profile: Optional[str] = None) -> List[str]: - '''Determines the flags to be used. - - :returns: the flags to be used with this tool.''' - return self._flags[profile] - def get_all_commandline_options( self, config: "BuildConfig", @@ -143,6 +137,11 @@ def get_all_commandline_options( if config.openmp: params.append(self.openmp_flag) + + # Explicitly add all compilation flags here, where the input + # path can be provided to properly resolve path-specific flags. + params += self.get_resolved_flags(config, input_file) + if add_flags: if self.openmp_flag in add_flags: warnings.warn( @@ -154,6 +153,22 @@ def get_all_commandline_options( params.extend([input_file.name, self._output_flag, str(output_file)]) return params + def get_flags(self, config: Optional["BuildConfig"] = None) -> List[str]: + '''Since compiler need path-specific information (and the + `run` method in tool does not provide the path), a compiler + will return an empty list as flags. All compiler flags + are added explicitly by `get_all_commandline_options`, + where the input path can be provided. + + :returns: the flags to be added automatically by tool, which + is just empty, since all compiler flags are added explicitly. + ''' + return [] + + def get_resolved_flags(self, config: "BuildConfig", + file_path: Path) -> List[str]: + return self.flags.get_flags(config, file_path) + def compile_file(self, input_file: Path, output_file: Path, config: "BuildConfig", @@ -174,7 +189,7 @@ def compile_file(self, input_file: Path, params = self.get_all_commandline_options(config, input_file, output_file, add_flags) - return self.run(profile=config.profile, cwd=input_file.parent, + return self.run(config=config, cwd=input_file.parent, additional_parameters=params) def check_available(self) -> bool: @@ -397,13 +412,14 @@ def get_all_commandline_options( :returns: all command line options for Fortran compilation. ''' if add_flags: - add_flags = Flags(add_flags) + # Create an AlwaysFlags to use its remove_flag method + af = AlwaysFlags(add_flags) if self._module_folder_flag: # Remove any module flag the user has specified, since # this will interfere with Fab's module handling. - add_flags.remove_flag(self._module_folder_flag, - has_parameter=True) - add_flags.remove_flag(self._compile_flag, has_parameter=False) + af.remove_flag(self._module_folder_flag, has_parameter=True) + af.remove_flag(self._compile_flag, has_parameter=False) + add_flags = af.get_flags(config, input_file) # Get the flags from the base class params = super().get_all_commandline_options(config, input_file, @@ -449,7 +465,7 @@ def compile_file(self, input_file: Path, output_file, add_flags, syntax_only) - self.run(profile=config.profile, cwd=input_file.parent, + self.run(config=config, cwd=input_file.parent, additional_parameters=params) diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index 38b2364f5..27f67c4cf 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -13,7 +13,7 @@ from fab.tools.category import Category from fab.tools.compiler import Compiler, FortranCompiler -from fab.tools.flags import Flags +from fab.tools.flags import ProfileFlags if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -71,14 +71,18 @@ def has_syntax_only(self) -> bool: raise RuntimeError(f"Compiler '{self._compiler.name}' has " f"no has_syntax_only.") - def get_flags(self, profile: Optional[str] = None) -> List[str]: + def get_flags(self, config: Optional["BuildConfig"] = None) -> List[str]: ''':returns: the ProfileFlags for the given profile, combined from the wrapped compiler and this wrapper. :param profile: the profile to use. ''' - return (self._compiler.get_flags(profile) + - super().get_flags(profile)) + return [] + + def get_resolved_flags(self, config: "BuildConfig", + file_path: Path) -> List[str]: + return (self._compiler.get_resolved_flags(config, file_path) + + super().get_resolved_flags(config, file_path)) def set_module_output_path(self, path: Path): '''Sets the output path for modules. @@ -100,7 +104,7 @@ def get_all_commandline_options( input_file: Path, output_file: Path, add_flags: Union[None, List[str]] = None, - syntax_only: Optional[bool] = False) -> List[str]: + syntax_only: Optional[bool] = None) -> List[str]: '''This function returns all command line options for a compiler wrapper. The syntax_only flag is only accepted, if the wrapped compiler is a Fortran compiler. Otherwise, @@ -121,7 +125,8 @@ def get_all_commandline_options( ''' # We need to distinguish between Fortran and non-Fortran compiler, # since only a Fortran compiler supports the syntax-only flag. - new_flags = Flags(add_flags) + new_flags = ProfileFlags(self.flags.get_flags(config, input_file)) + new_flags.add_flags(add_flags) if self._compiler.category is Category.FORTRAN_COMPILER: # Mypy complains that self._compiler does not take the syntax @@ -135,16 +140,18 @@ def get_all_commandline_options( # with Fab's module handling. new_flags.remove_flag(self._compiler._module_folder_flag, has_parameter=True) + resolved_flags = new_flags.get_flags(file_path=input_file) flags = self._compiler.get_all_commandline_options( - config, input_file, output_file, add_flags=add_flags, + config, input_file, output_file, add_flags=resolved_flags, syntax_only=syntax_only) else: # It's not valid to specify syntax_only for a non-Fortran compiler if syntax_only is not None: raise RuntimeError(f"Syntax-only cannot be used with compiler " f"'{self.name}'.") + resolved_flags = new_flags.get_flags(file_path=input_file) flags = self._compiler.get_all_commandline_options( - config, input_file, output_file, add_flags=add_flags) + config, input_file, output_file, add_flags=resolved_flags) return flags @@ -169,7 +176,7 @@ def compile_file(self, input_file: Path, config, input_file, output_file, add_flags=add_flags, syntax_only=syntax_only) - self.run(profile=config.profile, cwd=input_file.parent, + self.run(config=config, cwd=input_file.parent, additional_parameters=flags) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index 23b04b3ca..3d3b136c1 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -4,51 +4,173 @@ # which you should have received as part of this distribution ############################################################################## -'''This file contains a simple Flag class to manage tool flags. -It will need to be combined with build_config.FlagsConfig in a follow up -PR. ''' +This file contains the flag classes used to manage command line flags +for tools, especially path-specific flags for compiler. + +ProfileFlags +AbstractFlags: + The base class for a set of flags. + +AlwaysFlags(AbstractFlags): + Flags that will always apply, independent of the path of the + file to be compiled. It provides Python Template functionality: + - `$source` for */source* + - `$output` for */build_output* + - `$relative` for ** + +MatchFlags(AlwaysFlags) + Flags that are only applied if a wildcard search matches the + source file. + +ContainFlags(AlwaysFlags) + Flags that are only applied if the file path contains the specified + string. + +Flags: + Manages a list of flags, each of which is an instance of an AbstractFlag. + #TODO: Rename to FlagsList + +ProfileFlags: + Manages a set of flags for specific profiles, including inheritance. + +Each tool uses a ProfileFlags instance. At runtime, the compilation steps +will use the selected profile to get the Flags instance to use. +The function `get_flags` will resolve the list of AbstractFlags by +converting them from left to right into a list of strings. For example, +[AlwaysFlag("-g"), ContainFlags("-O3", pattern="special_file")] +will be convert to `["-g", "-O3"]` if the file contains the string +`special_file`, and otherwise it will be `["-g"]`. +''' + +from abc import ABC, abstractmethod +from fnmatch import fnmatch import logging -from typing import Dict, List, Optional, Union +from pathlib import Path +from string import Template +from typing import Dict, List, Optional, TYPE_CHECKING, Union import warnings from fab.util import string_checksum +if TYPE_CHECKING: + from fab.build_config import BuildConfig -class Flags(list): - '''This class represents a list of parameters for a tool. It is a - list with some additional functionality. +logger = logging.getLogger(__name__) - TODO #22: This class and build_config.FlagsConfig should be combined. - :param list_of_flags: List of parameters to initialise this object with. +class AbstractFlags(ABC): + ''' + An abstract class to act as base class for all flag classes. ''' - def __init__(self, list_of_flags: Optional[List[str]] = None): - self._logger = logging.getLogger(__name__) - super().__init__() - if list_of_flags: - self.extend(list_of_flags) + @abstractmethod + def __init__(self) -> None: + """ + Dummy constructor. + """ - def checksum(self) -> str: + @abstractmethod + def get_flags(self, + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: """ - :returns: a checksum of the flags. + This function returns the list of flags to be used for the given + filename. + + :param config: the config object (used for paths in templates) + :param file_path: the file path of the file, not used in this + class. + :returns: the list of flags to use. """ - return string_checksum(str(self)) - def add_flags(self, new_flags: Union[str, List[str]]): - '''Adds the specified flags to the list of flags. + @abstractmethod + def remove_flag(self, remove_flag: str, has_parameter: bool = False): + '''Removes all occurrences of `remove_flag` in flags. + If has_parameter is defined, the next entry in flags will also be + removed, and if this object contains this flag+parameter without space + (e.g. `-J/tmp`), it will be correctly removed. Note that only the + flag itself must be specified, you cannot remove a flag only if a + specific parameter is given (i.e. `remove_flag="-J/tmp"` will not + work if this object contains `[...,"-J", "/tmp"]`). - :param new_flags: A single string or list of strings which are the - flags to be added. + :param remove_flag: the flag to remove + :param has_parameter: if the flag to remove takes a parameter ''' - if isinstance(new_flags, str): - self.append(new_flags) + +class AlwaysFlags(AbstractFlags): + """ + This class represents a list of flags that is always to be used, + independent of the path of the source file. It also provides + template functionality. This class also acts as convenient base + class for all flags that add flags. + + :param flags: a string or list of strings with command line flags. + """ + def __init__(self, flags: Optional[Union[str, List[str]]] = None) -> None: + + if isinstance(flags, str): + self._flags = [flags] + elif flags: + # Make a copy in case the user modifies the list later + self._flags = flags[:] else: - self.extend(new_flags) + self._flags = [] + + def __eq__(self, other: "AlwaysFlags") -> bool: + """ + :returns: if two AlwaysFlags are identical + """ + print("ALWAYS=", self, other, type(self), type(other)) + return (type(self) == type(other) and + self._flags == other._flags) + + def replace_template(self, + string_list: List[str], + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: + """This function replaces all `$relative`, `$source`, and `$output` + in the string or list of string with the values taken from + the config object and the file path. + + :param string_list: list of strings, which will get all template + arguments replaced. + :param config: the build configuration object from which paths + are taken. + :param file_path: the file path of an argument, used for `$source`. + + :returns: a new list with strings where all the template parameters + have been removed. + """ + params = {} + if config: + params['source'] = config.source_root + params['output'] = config.build_output + if file_path: + params['relative'] = file_path.parent + + # Use templating to render any relative paths in our flags + return [Template(i).substitute(params) for i in string_list] + + def get_flags(self, + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: + """ + This function returns the list of flags to be used for the given + filename. This class will not take the file path into account, + but it does support templated expressions `$relative`, `$source`, + and `$output`. + + :param config: the config object (used for paths in templates) + :param file_path: the file path of the file, not used in this + class. + + :returns: the list of flags to use. + """ + return self.replace_template(self._flags, config, file_path) def remove_flag(self, remove_flag: str, has_parameter: bool = False): '''Removes all occurrences of `remove_flag` in flags. @@ -67,20 +189,21 @@ def remove_flag(self, remove_flag: str, has_parameter: bool = False): # behaviour here (since ordering of flags can be important) i = 0 flag_len = len(remove_flag) - while i < len(self): - flag = self[i] + while i < len(self._flags): + flag = self._flags[i] # First check for the flag stand-alone, i.e. if it has a parameter, # it will be the next entry: [... "-J", "/tmp"]: if flag == remove_flag: - if has_parameter and i + 1 == len(self): + if has_parameter and i + 1 == len(self._flags): # We have a flag which takes a parameter, but there is no # parameter. Issue a warning: - self._logger.warning(f"Flags '{' '. join(self)}' contain " - f"'{remove_flag}' but no parameter.") - del self[i] + logger.warning(f"Flags '{' '. join(self._flags)}'" + f" contain '{remove_flag}' but no " + f"parameter.") + del self._flags[i] else: # Delete the argument and if required its parameter - del self[i:i+(2 if has_parameter else 1)] + del self._flags[i:i+(2 if has_parameter else 1)] warnings.warn(f"Removing managed flag '{remove_flag}'.") continue # Now check if it has flag and parameter as one argument (-J/tmp) @@ -88,19 +211,170 @@ def remove_flag(self, remove_flag: str, has_parameter: bool = False): if has_parameter and flag[:flag_len] == remove_flag: # No space between flag and parameter, remove this one flag warnings.warn(f"Removing managed flag '{remove_flag}'.") - del self[i] + del self._flags[i] continue i += 1 +class MatchFlags(AlwaysFlags): + """ + This class implements path-specific flags using a file system + wild card expressions (e.g. supporting `*` and `?`). It does support + templated expressions `$relative`, `$source`, and `$output` in the + pattern as well. + + :param flags: a string or list of strings with command line flags. + :param pattern: the wildcard pattern which is used when matching. + """ + def __init__(self, flags: Union[str, List[str]], pattern: str) -> None: + super().__init__(flags) + self._pattern = pattern + + def get_flags(self, + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: + """ + This function returns the list of flags to be used for the given + filename if the specified file path matches the pattern specified. + + :param config: the config object (used for paths in templates) + :param file_path: the file path of the file that must match the + specified wildcard pattern. + + :returns: the list of flags to use. + """ + # Resolve the pattern template: + pattern = self.replace_template([self._pattern], config, file_path)[0] + # If there is no wildcard match, return an empty list + if not fnmatch(str(file_path), pattern): + return [] + + return super().get_flags(config, file_path) + + +class ContainFlags(AlwaysFlags): + """ + This class implements path-specific flags using a simple + substring search. + + :param flags: a string or list of strings with command line flags. + :param pattern: the substring which is used when matching. + """ + + def __init__(self, flags: List[str], pattern: str) -> None: + super().__init__(flags) + self._pattern = pattern + + def get_flags(self, + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: + """ + This function returns the list of flags to be used for the given + filename if the specified file path contains the pattern as + a substrings (i.e. anywhere in the file path). + + :param config: the config object (used for paths in templates) + :param file_path: the file path of the file that must match the + specified wildcard pattern. + + :returns: the list of flags to use. + """ + + # Resolve the pattern template: + pattern = self.replace_template([self._pattern], config, file_path)[0] + # If pattern is not in the file path, return an empty list + if pattern not in str(file_path): + return [] + + return super().get_flags(config, file_path) + + +class Flags(List[AbstractFlags]): + '''This class represents a list of parameters for a tool. It is a + list with some additional functionality. + + :param list_of_flags: List of parameters to initialise this object with. + ''' + + def __init__( + self, + list_of_flags: Optional[Union[AbstractFlags, str, + List[str]]] = None) -> None: + self._logger = logging.getLogger(__name__) + super().__init__() + if isinstance(list_of_flags, (str, list)): + self.append(AlwaysFlags(list_of_flags)) + elif list_of_flags: + self.append(list_of_flags) + + def get_flags(self, + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: + """ + :returns: the flags to be used for the compilation profile and + file path specified. + """ + + if not file_path: + # If no path, provide a dummy path + file_path = Path() + + all_flags_resolved: List[str] = [] + + for flags in self: + all_flags_resolved.extend(flags.get_flags(config, file_path)) + + return all_flags_resolved + + def add_flags(self, + new_flags: Union[AbstractFlags, str, List[str]]) -> None: + '''Adds the specified flags to the list of flags. + + :param new_flags: A single string or list of strings which are the + flags to be added. + ''' + + if isinstance(new_flags, AbstractFlags): + self.append(new_flags) + else: + self.append(AlwaysFlags(new_flags)) + + def remove_flag(self, remove_flag: str, has_parameter: bool = False): + '''Removes all occurrences of `remove_flag` in flags. + If `has_parameter` is defined, the next entry in flags will also be + removed, and if this object contains this flag+parameter without space + (e.g. `-J/tmp`), it will be correctly removed. Note that only the + flag itself must be specified, you cannot remove a flag only if a + specific parameter is given (i.e. `remove_flag="-J/tmp"` will not + work if this object contains `[...,"-J", "/tmp"]`). + + This function will just call all individual flag objects to remove + the flag. Note that a flag and its parameter cannot be split into + two Flag instances (i.e. [AlwaysFlag("-J"), AlwaysFlag("/tmp")] + will not work). + + :param remove_flag: the flag to remove + :param has_parameter: if the flag to remove takes a parameter + ''' + + for flags in self: + flags.remove_flag(remove_flag, has_parameter) + + class ProfileFlags: '''A list of flags that support a 'profile' to be used. If no profile is specified, it will use "" (empty string) as 'profile'. All functions take an optional profile parameter, so this class can also be used for tools that do not need a profile. + + :param flags: optional flags to be added to this profile. + :param profile: optional profile to use if flags are specified, + defaults to "". ''' - def __init__(self: "ProfileFlags"): + def __init__(self: "ProfileFlags", + flags: Optional[Union[AbstractFlags, str, List[str]]] = None, + profile: str = "") -> None: # Stores the flags for each profile mode. The key is the (lower case) # name of the profile mode, and it contains a list of flags self._profiles: Dict[str, Flags] = {"": Flags()} @@ -109,11 +383,49 @@ def __init__(self: "ProfileFlags"): # 'inherits' the flags from a different mode (recursively) self._inherit_from: Dict[str, str] = {} - def __getitem__(self, profile: Optional[str] = None) -> List[str]: + if flags: + if profile != "": + self.define_profile(profile) + self.add_flags(flags, profile) + + def get_flags(self, + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: + ''' + This method returns the flags used for the specified file, + i.e. it will support path-specific flags. The BuildConfig + is added as parameter to get the profile, but also to + allow flags to use templated expressions `$relative` and + `$output` (the values are taken from the config object). + + :param config: the build config object. It stores the selected + compilation profile, and paths that can be used in templated + expressions. + :param file_path: path to the source file to compile. + ''' + if not file_path: + # If no path, provide a dummy path + file_path = Path() + if config: + profile = config.profile + else: + profile = "" + + all_flags = self[profile] + + resolved_flags = [] + for flags in all_flags: + resolved_flags.extend(flags.get_flags(config, file_path)) + + return resolved_flags + + def __getitem__(self, + profile: Optional[str] = None) -> List[AbstractFlags]: '''Returns the flags for the requested profile. If profile is not specified, the empty profile ("") will be used. It will also take inheritance into account, so add flags (recursively) from inherited - profiles. + profiles. But this function will not resolve the flags, i.e. replace + the AbstractFlags instances with a list of strings. :param profile: the optional profile to use. @@ -165,8 +477,8 @@ def define_profile(self, self._inherit_from[name.lower()] = inherit_from.lower() def add_flags(self, - new_flags: Union[str, List[str]], - profile: Optional[str] = None): + new_flags: Union[AbstractFlags, str, List[str]], + profile: Optional[str] = None) -> None: '''Adds the specified flags to the list of flags. :param new_flags: A single string or list of strings which are the @@ -211,17 +523,28 @@ def remove_flag(self, self._profiles[profile].remove_flag(remove_flag, has_parameter) - def checksum(self, profile: Optional[str] = None) -> str: + def checksum(self, + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> int: """ - :returns: a checksum of the flags. + :param config: the config object (used for templating) + :param file_path: the file path of the source file, used for + path-specific flags. + :returns: a checksum of the flags. """ - if not profile: - profile = "" + + if not file_path: + # If no path, provide a dummy path + file_path = Path() + if config: + profile = config.profile else: - profile = profile.lower() + profile = "" if profile not in self._profiles: - raise KeyError(f"checksum: Profile '{profile}' is not defined.") + raise KeyError(f"checksum: Profile '{profile}' is " + f"not defined.") - return self._profiles[profile].checksum() + resolve_flags: List[str] = self.get_flags(config, file_path) + return string_checksum(str(resolve_flags)) diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index a1307729e..733756ff4 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -109,7 +109,7 @@ def define_profile(self, self._pre_lib_flags.define_profile(name, inherit_from) self._post_lib_flags.define_profile(name, inherit_from) - def get_profile_flags(self, profile: str) -> List[str]: + def get_profile_flags(self, config: "BuildConfig") -> List[str]: ''' :returns: the ProfileFlags for the given profile, combined from the wrapped compiler and this wrapper. @@ -117,10 +117,13 @@ def get_profile_flags(self, profile: str) -> List[str]: :param profile: the profile to use. ''' if self._linker: - flags = self._linker.get_profile_flags(profile)[:] + flags = self._linker.get_profile_flags(config)[:] else: flags = [] - return flags + self._compiler.get_flags(profile) + # A compiler can have path specific flags. Since we are only + # interested in linking here, path-specific flags are not + # needed, so we provide a dummy file-path. + return flags + self._compiler.get_resolved_flags(config, Path("")) def get_lib_flags(self, lib: str) -> List[str]: '''Gets the standard flags for a standard library @@ -186,7 +189,7 @@ def get_pre_link_flags(self, config: "BuildConfig") -> List[str]: ''' params: List[str] = [] if self._pre_lib_flags: - params.extend(self._pre_lib_flags[config.profile]) + params.extend(self._pre_lib_flags.get_flags(config)) if self._linker: # If we are wrapping a linker, get the wrapped linker's # pre-link flags and append them to the end (so the linker @@ -211,7 +214,7 @@ def get_post_link_flags(self, config: "BuildConfig") -> List[str]: # wrapped linker). params.extend(self._linker.get_post_link_flags(config)) if self._post_lib_flags: - params.extend(self._post_lib_flags[config.profile]) + params.extend(self._post_lib_flags.get_flags(config)) return params def link(self, input_files: List[Path], output_file: Path, @@ -232,7 +235,7 @@ def link(self, input_files: List[Path], output_file: Path, params: List[Union[str, Path]] = [] - params.extend(self._compiler.get_flags(config.profile)) + params.extend(self._compiler.get_resolved_flags(config, Path())) if config.openmp: params.append(self._compiler.openmp_flag) diff --git a/source/fab/tools/tool.py b/source/fab/tools/tool.py index a12773cc4..d26738c78 100644 --- a/source/fab/tools/tool.py +++ b/source/fab/tools/tool.py @@ -16,10 +16,13 @@ import logging from pathlib import Path import subprocess -from typing import Dict, List, Optional, Sequence, Union +from typing import Dict, List, Optional, Sequence, TYPE_CHECKING, Union from fab.tools.category import Category -from fab.tools.flags import ProfileFlags +from fab.tools.flags import AbstractFlags, ProfileFlags + +if TYPE_CHECKING: + from fab.build_config import BuildConfig class Tool: @@ -121,11 +124,16 @@ def category(self) -> Category: ''':returns: the category of this tool.''' return self._category - def get_flags(self, profile: Optional[str] = None): + @property + def flags(self) -> ProfileFlags: + ''':returns: the profile flags for this tool.''' + return self._flags + + def get_flags(self, config: Optional["BuildConfig"] = None) -> List[str]: ''':returns: the flags to be used with this tool.''' - return self._flags[profile] + return self.flags.get_flags(config) - def add_flags(self, new_flags: Union[str, List[str]], + def add_flags(self, new_flags: Union[AbstractFlags, str, List[str]], profile: Optional[str] = None): '''Adds the specified flags to the list of flags. @@ -159,7 +167,7 @@ def __str__(self): def run(self, additional_parameters: Optional[ Union[str, Sequence[Union[Path, str]]]] = None, - profile: Optional[str] = None, + config: Optional["BuildConfig"] = None, env: Optional[Dict[str, str]] = None, cwd: Optional[Union[Path, str]] = None, capture_output=True) -> str: @@ -170,6 +178,8 @@ def run(self, List of strings or paths to be sent to :func:`subprocess.run` as additional parameters for the command. Any path will be converted to a normal string. + :param config: the config object, used for accessing compilation mode + and paths (if templating is required) :param env: Optional env for the command. By default it will use the current session's environment. @@ -180,7 +190,7 @@ def run(self, :raises RuntimeError: if the code is not available. :raises RuntimeError: if the return code of the executable is not 0. """ - command = [str(self.exec_path)] + self.get_flags(profile) + command = [str(self.exec_path)] + self.get_flags(config) if additional_parameters: if isinstance(additional_parameters, str): command.append(additional_parameters) diff --git a/source/fab/util.py b/source/fab/util.py index fb9f60adb..4cedef1e8 100644 --- a/source/fab/util.py +++ b/source/fab/util.py @@ -63,7 +63,7 @@ def file_checksum(fpath): return HashedFile(fpath, zlib.crc32(infile.read())) -def string_checksum(s: str): +def string_checksum(s: str) -> int: """ Return a checksum of the given string. diff --git a/tests/unit_tests/steps/test_compile_c.py b/tests/unit_tests/steps/test_compile_c.py index 7b4399637..e10329a22 100644 --- a/tests/unit_tests/steps/test_compile_c.py +++ b/tests/unit_tests/steps/test_compile_c.py @@ -17,7 +17,7 @@ from fab.parse.c import AnalysedC from fab.steps.compile_c import _get_obj_combo_hash, _compile_file, compile_c from fab.tools.category import Category -from fab.tools.flags import Flags +from fab.tools.flags import ProfileFlags from fab.tools.tool_box import ToolBox @@ -117,7 +117,9 @@ class TestGetObjComboHash: @fixture(scope='function') def flags(self): '''Returns the flag for these tests.''' - return Flags(['-Denv_flag', '-I', 'foo/include', '-Dhello']) + pf = ProfileFlags() + pf.add_flags(['-Denv_flag', '-I', 'foo/include', '-Dhello']) + return pf def test_vanilla(self, content, flags, fake_process: FakeProcess) -> None: """ @@ -158,7 +160,7 @@ def test_change_flags(self, content, flags, fake_process.register(['scc', '--version'], stdout='1.2.3') compiler = config.tool_box[Category.C_COMPILER] - flags = Flags(['-Dfoo'] + flags) + flags = ProfileFlags(['-Dfoo'] + flags.get_flags(), config.profile) result = _get_obj_combo_hash(config, compiler, analysed_file, flags) assert result != 5066163117 diff --git a/tests/unit_tests/test_build_config.py b/tests/unit_tests/test_build_config.py index 2b21e81ab..da0605d94 100644 --- a/tests/unit_tests/test_build_config.py +++ b/tests/unit_tests/test_build_config.py @@ -81,3 +81,16 @@ def test_fab_workspace_with_env(self): some_dir = Path('/some_dir') config = BuildConfig('proj', ToolBox(), fab_workspace=some_dir) assert config.project_workspace == some_dir / 'proj' + + +def test_set_profile(stub_configuration): + """ + Tests that setting compilation profiles work as expected. + """ + + stub_configuration.set_profile("fast-debug") + assert stub_configuration.profile == "fast-debug" + + # All profiles names are in lower case + stub_configuration.set_profile("fast-DEBUG") + assert stub_configuration.profile == "fast-debug" diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index 0109d82ce..d48916215 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -263,23 +263,46 @@ def test_flags_independent(stub_c_compiler: CCompiler, assert stub_c_compiler.get_flags() == [] assert wrapper.get_flags() == [] - stub_c_compiler.add_flags(["-a", "-b"]) - assert stub_c_compiler.get_flags() == ["-a", "-b"] - assert wrapper.get_flags() == ['-a', '-b'] + stub_c_compiler.add_flags(['-a', '-b']) + # Compiler flags are handled differently in a compiler (since a compiler + # needs path-specific flags, but the generic get_flags call from Tools + # does not provide a path). So the compiler should report no flags: + assert stub_c_compiler.get_flags() == [] + resolved_flags = stub_c_compiler.get_all_commandline_options( + stub_configuration, Path('/in'), Path("/out")) + assert resolved_flags == ['-c', '-a', '-b', 'in', '-o', '/out'] + + assert wrapper.get_flags() == [] + + # We need to test `get_all_commandline_options` to check the correct + # behaviour of flags, which can resolve path-specific flags. + resolved_flags = wrapper.get_all_commandline_options(stub_configuration, + Path('/in'), + Path('/out')) + + assert resolved_flags == ['-c', '-a', '-b', 'in', '-o', '/out'] assert wrapper.openmp_flag == stub_c_compiler.openmp_flag # Adding flags to the wrapper should not affect the wrapped compiler: - wrapper.add_flags(["-d", "-e"]) - assert stub_c_compiler.get_flags() == ['-a', '-b'] + wrapper.add_flags(['-d', '-e']) + resolved_flags = stub_c_compiler.get_all_commandline_options( + stub_configuration, Path('/in'), Path("/out")) + assert resolved_flags == ['-c', '-a', '-b', 'in', '-o', '/out'] + # And the compiler wrapper should report the wrapped compiler's flag # followed by the wrapper flag (i.e. the wrapper flag can therefore # overwrite the wrapped compiler's flags) - assert wrapper.get_flags() == ['-a', '-b', "-d", "-e"] + assert wrapper.get_flags() == [] + resolved_flags = wrapper.get_all_commandline_options(stub_configuration, + Path("/in"), + Path("/out")) + assert resolved_flags == ['-c', '-a', '-b', '-d', '-e', + 'in', '-o', '/out'] wrapper.compile_file(Path("a.f90"), Path('a.o'), add_flags=['-f'], config=stub_configuration) assert subproc_record.invocations() == [ - ['mpicc', "-a", "-b", "-d", "-e", "-c", "-f", "a.f90", "-o", 'a.o'] + ['mpicc', '-c', '-a', '-b', '-d', '-e', '-f', 'a.f90', '-o', 'a.o'] ] assert subproc_record.extras()[0]['cwd'] == '.' @@ -290,26 +313,26 @@ def test_compiler_wrapper_flags_with_add_arg(stub_c_compiler: CCompiler, '''Tests that flags set in the base compiler will be accessed in the wrapper if also additional flags are specified.''' mpicc = Mpicc(stub_c_compiler) - stub_c_compiler.define_profile("default", inherit_from="") - mpicc.define_profile("default", inherit_from="") + stub_c_compiler.define_profile('default', inherit_from="") + mpicc.define_profile('default', inherit_from="") # Due to inheritance, this will give "-a -b" for gcc - stub_c_compiler.add_flags(["-a"]) - stub_c_compiler.add_flags(["-b"], "default") - # Due to inheritance, this will give "-d -e" for mpicc - mpicc.add_flags(["-d"]) - mpicc.add_flags(["-e"], "default") + stub_c_compiler.add_flags(['-a']) + stub_c_compiler.add_flags(['-b'], 'default') + # Due to inheritance, this will give '-d -e' for mpicc + mpicc.add_flags(['-d']) + mpicc.add_flags(['-e'], 'default') # Check that the flags are assembled in the right order in the # actual compiler call: first the wrapped compiler flag, then # the wrapper flag, then additional flags stub_configuration._openmp = True - stub_configuration._profile = "default" + stub_configuration._profile = 'default' - mpicc.compile_file(Path("a.f90"), Path("a.o"), add_flags=["-f"], + mpicc.compile_file(Path('a.f90'), Path('a.o'), add_flags=['-f'], config=stub_configuration) assert subproc_record.invocations() == [ - ['mpicc', "-a", "-b", "-d", "-e", "-c", "-omp", "-f", - "a.f90", "-o", 'a.o'] + ['mpicc', '-c', '-omp', '-a', '-b', '-d', '-e', '-f', + 'a.f90', '-o', 'a.o'] ] @@ -322,12 +345,12 @@ def test_args_without_add_arg(stub_c_compiler: CCompiler, """ wrapper = CompilerWrapper('wrapper', 'wrp', compiler=stub_c_compiler) - stub_c_compiler.add_flags(["-a", "-b"]) - wrapper.add_flags(["-d", "-e"]) + stub_c_compiler.add_flags(['-a', '-b']) + wrapper.add_flags(['-d', '-e']) - wrapper.compile_file(Path("a.f90"), Path('a.o'), config=stub_configuration) + wrapper.compile_file(Path('a.f90'), Path('a.o'), config=stub_configuration) assert subproc_record.invocations() == [ - ['wrp', "-a", "-b", "-d", "-e", "-c", "a.f90", "-o", 'a.o'] + ['wrp', '-c', '-a', '-b', '-d', '-e', 'a.f90', '-o', 'a.o'] ] assert subproc_record.extras()[0]['cwd'] == '.' diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index 008d262f9..6b957d3f7 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -7,12 +7,101 @@ '''Tests the compiler implementation. ''' +from pathlib import Path import pytest -from fab.tools import Flags, ProfileFlags +from fab.tools.flags import (AlwaysFlags, ContainFlags, Flags, MatchFlags, + ProfileFlags) from fab.util import string_checksum +def test_always_flags(stub_configuration): + """ + Tests the various AbstractFlags constructors. + """ + + # Constructor: + af = AlwaysFlags() + assert af.get_flags() == [] + af = AlwaysFlags("-g") + assert af.get_flags() == ["-g"] + af = AlwaysFlags(["-g", "-O2"]) + assert af.get_flags() == ["-g", "-O2"] + + # Comparison: First different derived type: + cf_copy = ContainFlags(["-g", "-O2"], pattern="XX") + assert af != cf_copy + af_copy = AlwaysFlags(["-g", "-O2"]) + assert af_copy == af + af_copy = AlwaysFlags(["-O2", "-g"]) + assert af_copy != af + + # Templating + af = AlwaysFlags(["$source", "$output"]) + assert (af.get_flags(stub_configuration) == + [str(stub_configuration.source_root), + str(stub_configuration.build_output)]) + af = AlwaysFlags(["$source", "$output", "$relative"]) + file_path = Path("/my/file") + assert (af.get_flags(stub_configuration, file_path) == + [str(stub_configuration.source_root), + str(stub_configuration.build_output), + "/my"]) + + +def test_always_flags_remove_flags(): + '''Test remove_flags functionality.''' + flags = AlwaysFlags() + flags.remove_flag("-c", False) + # pylint: disable-next=use-implicit-booleaness-not-comparison + assert flags.get_flags() == [] + + all_flags = ['a.f90', '-c', '-o', 'a.o', '-fsyntax-only', "-J", "/tmp"] + flags = AlwaysFlags(all_flags) + assert flags.get_flags() == all_flags + with pytest.warns(UserWarning, match="Removing managed flag"): + flags.remove_flag("-c") + del all_flags[1] + assert flags.get_flags() == all_flags + with pytest.warns(UserWarning, match="Removing managed flag"): + flags.remove_flag("-J", has_parameter=True) + del all_flags[-2:] + assert flags.get_flags() == all_flags + + for flags_in, expected in [(["-J", "b"], []), + (["-Jb"], []), + (["a", "-J", "c"], ["a"]), + (["a", "-Jc"], ["a"]), + (["a", "-J"], ["a"]), + ]: + flags = AlwaysFlags(flags_in) + with pytest.warns(UserWarning, match="Removing managed flag"): + flags.remove_flag("-J", has_parameter=True) + assert flags.get_flags() == expected + + +def test_match_flags() -> None: + """ + Tests matching using wildcards. + """ + mf = MatchFlags("-g", pattern="/*") + assert mf.get_flags(file_path=Path(".")) == [] + mf = MatchFlags(["-g", "$relative"], pattern="/*") + file_path = Path("/my/dir") + assert mf.get_flags(file_path=file_path) == ["-g", "/my"] + + +def test_contain_flags() -> None: + """ + Tests matching using substrings. + """ + cf = ContainFlags("-g", pattern="yes") + assert cf.get_flags(file_path=Path(".")) == [] + cf = ContainFlags(["-g", "$relative"], pattern="/") + file_path = Path("/my/dir") + assert cf.get_flags(file_path=file_path) == ["-g", "/my"] + + def test_flags_constructor(): '''Tests the constructor of Flags.''' f1 = Flags() @@ -22,22 +111,46 @@ def test_flags_constructor(): assert f1 == [] f2 = Flags(["a"]) assert isinstance(f2, list) - assert f2 == ["a"] + assert f2.get_flags() == ["a"] + + af = AlwaysFlags() + assert af.get_flags() == [] + af = AlwaysFlags("-g") + assert af.get_flags() == ["-g"] + af = AlwaysFlags(["-g", "-O2"]) + assert af.get_flags() == ["-g", "-O2"] def test_flags_adding(): '''Tests adding flags.''' f1 = Flags() # pylint: disable-next=use-implicit-booleaness-not-comparison - assert f1 == [] + assert f1.get_flags() == [] f1.add_flags("-a") - assert f1 == ["-a"] + assert f1.get_flags() == ["-a"] f1.add_flags(["-b", "-c"]) - assert f1 == ["-a", "-b", "-c"] + assert len(f1) == 2 + assert f1.get_flags() == ["-a", "-b", "-c"] + assert f1[0] == AlwaysFlags("-a") + assert f1[1] == AlwaysFlags(["-b", "-c"]) + + # Check functionality when adding a flag object: + af1 = AlwaysFlags("-g") + f1 = Flags(af1) + assert f1 == [af1] + assert f1.get_flags() == ["-g"] + + af2 = AlwaysFlags(["-O2", "-warn"]) + f1.add_flags(af2) + assert f1 == [af1, af2] + assert f1.get_flags() == ["-g", "-O2", "-warn"] def test_remove_flags(): - '''Test remove_flags functionality.''' + '''Test remove_flags functionality. This is a subset of the remove + tests for AlwaysFlags, just to ensure that the calls are getting + forwarded from Flags to the AlwaysFlags implementation. + ''' flags = Flags() flags.remove_flag("-c", False) # pylint: disable-next=use-implicit-booleaness-not-comparison @@ -45,32 +158,22 @@ def test_remove_flags(): all_flags = ['a.f90', '-c', '-o', 'a.o', '-fsyntax-only', "-J", "/tmp"] flags = Flags(all_flags) - assert flags == all_flags + assert flags.get_flags() == all_flags with pytest.warns(UserWarning, match="Removing managed flag"): flags.remove_flag("-c") del all_flags[1] - assert flags == all_flags + assert flags.get_flags() == all_flags with pytest.warns(UserWarning, match="Removing managed flag"): flags.remove_flag("-J", has_parameter=True) del all_flags[-2:] - assert flags == all_flags - - for flags_in, expected in [(["-J", "b"], []), - (["-Jb"], []), - (["a", "-J", "c"], ["a"]), - (["a", "-Jc"], ["a"]), - (["a", "-J"], ["a"]), - ]: - flags = Flags(flags_in) - with pytest.warns(UserWarning, match="Removing managed flag"): - flags.remove_flag("-J", has_parameter=True) - assert flags == expected + assert flags.get_flags() == all_flags def test_flags_checksum(): '''Tests computation of the checksum.''' list_of_flags = ['one', 'two', 'three', 'four'] - flags = Flags(list_of_flags) + flags = ProfileFlags() + flags.add_flags(list_of_flags) assert flags.checksum() == string_checksum(str(list_of_flags)) @@ -80,9 +183,10 @@ def test_profile_flags_with_profile(): pf.define_profile("base") assert pf["base"] == [] pf.add_flags("-base", "base") - assert pf["base"] == ["-base"] + assert pf["base"] == [AlwaysFlags("-base")] pf.add_flags(["-base2", "-base3"], "base") - assert pf["base"] == ["-base", "-base2", "-base3"] + assert pf["base"] == [AlwaysFlags("-base"), + AlwaysFlags(["-base2", "-base3"])] # Check that we get an exception if we specify a profile # that does not exist @@ -91,14 +195,24 @@ def test_profile_flags_with_profile(): assert "Profile 'does_not_exist' is not defined" in str(err.value) +def test_profile_flags_constructor_args(): + '''Tests various constructor argument combinations.''' + pf = ProfileFlags("-g") + assert pf[""] == [AlwaysFlags("-g")] + pf = ProfileFlags("-g", profile="prof") + assert pf[""] == [] + assert pf["prof"] == [AlwaysFlags("-g")] + + def test_profile_flags_without_profile(): '''Tests adding flags.''' pf = ProfileFlags() assert pf[""] == [] + assert pf[None] == [] pf.add_flags("-base") - assert pf[""] == ["-base"] + assert pf[""] == [AlwaysFlags("-base")] pf.add_flags(["-base2", "-base3"]) - assert pf[""] == ["-base", "-base2", "-base3"] + assert pf[""] == [AlwaysFlags("-base"), AlwaysFlags(["-base2", "-base3"])] # Check that we get an exception if we specify a profile with pytest.raises(KeyError) as err: @@ -117,7 +231,7 @@ def test_profile_flags_without_profile(): assert pf._inherit_from["from_default"] == "" -def test_profile_flags_inheriting(): +def test_profile_flags_inheriting(stub_configuration): '''Tests adding flags.''' pf = ProfileFlags() pf.define_profile("base") @@ -126,21 +240,25 @@ def test_profile_flags_inheriting(): assert "base" not in pf._inherit_from pf.add_flags("-base", "base") - assert pf["base"] == ["-base"] + stub_configuration.set_profile("base") + assert pf.get_flags(stub_configuration) == ["-base"] pf.define_profile("derived", "base") - assert pf["derived"] == ["-base"] + stub_configuration.set_profile("derived") + assert pf.get_flags(stub_configuration) == ["-base"] assert pf._inherit_from["derived"] == "base" pf.add_flags("-derived", "derived") - assert pf["derived"] == ["-base", "-derived"] + assert pf.get_flags(stub_configuration) == ["-base", "-derived"] pf.define_profile("derived2", "derived") - assert pf["derived2"] == ["-base", "-derived"] + stub_configuration.set_profile("derived2") + assert pf.get_flags(stub_configuration) == ["-base", "-derived"] pf.add_flags("-derived2", "derived2") - assert pf["derived2"] == ["-base", "-derived", "-derived2"] + assert pf.get_flags(stub_configuration) == ["-base", "-derived", + "-derived2"] -def test_profile_flags_removing(): +def test_profile_flags_removing(stub_configuration): '''Tests adding flags.''' pf = ProfileFlags() pf.define_profile("base") @@ -149,34 +267,41 @@ def test_profile_flags_removing(): warn_message = "Removing managed flag '-base1'." with pytest.warns(UserWarning, match=warn_message): pf.remove_flag("-base1", "base") - assert pf["base"] == ["-base2"] + stub_configuration.set_profile("base") + assert pf.get_flags(stub_configuration, Path()) == ["-base2"] # Try removing a flag that's not there. This should not # cause any issues. pf.remove_flag("-does-not-exist") - assert pf["base"] == ["-base2"] + assert pf.get_flags(stub_configuration, Path()) == ["-base2"] pf.add_flags(["-base1", "-base2"]) warn_message = "Removing managed flag '-base1'." with pytest.warns(UserWarning, match=warn_message): pf.remove_flag("-base1") - assert pf[""] == ["-base2"] + stub_configuration.set_profile("") + assert pf.get_flags(stub_configuration) == ["-base2"] -def test_profile_flags_checksum(): +def test_profile_flags_checksum(stub_configuration): '''Tests computation of the checksum.''' pf = ProfileFlags() pf.define_profile("base") list_of_flags = ['one', 'two', 'three', 'four'] pf.add_flags(list_of_flags, "base") - assert pf.checksum("base") == string_checksum(str(list_of_flags)) + stub_configuration._profile = "base" + assert (pf.checksum(stub_configuration, Path()) == + string_checksum(str(list_of_flags))) - list_of_flags_new = ['one', 'two', 'three', 'four', "five"] + # These flags get added to the "" profile, NOT base: + list_of_flags_new = ["five", "six"] pf.add_flags(list_of_flags_new) - assert pf.checksum() == string_checksum(str(list_of_flags_new)) + stub_configuration.set_profile("") + assert (pf.checksum(stub_configuration, Path()) == + string_checksum(str(list_of_flags_new))) -def test_profile_flags_errors_invalid_profile_name(): +def test_profile_flags_errors_invalid_profile_name(stub_configuration): '''Tests that given undefined profile names will raise KeyError in call functions. ''' @@ -196,7 +321,8 @@ def test_profile_flags_errors_invalid_profile_name(): assert ("remove_flag: Profile 'does not exist' is not defined." in str(err.value)) + stub_configuration._profile = "does_not_exist" with pytest.raises(KeyError) as err: - pf.checksum("does not exist") - assert ("checksum: Profile 'does not exist' is not defined." + pf.checksum(stub_configuration, Path("/some/path")) + assert ("checksum: Profile 'does_not_exist' is not defined." in str(err.value)) diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index 998c6d8a4..d075d9c11 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -219,9 +219,10 @@ def test_c_with_libraries(stub_c_compiler: CCompiler, ] -def test_c_with_libraries_and_post_flags(stub_c_compiler: CCompiler, - stub_configuration: BuildConfig, - subproc_record: ExtendedRecorder) -> None: +def test_c_with_libraries_and_post_flags( + stub_c_compiler: CCompiler, + stub_configuration: BuildConfig, + subproc_record: ExtendedRecorder) -> None: """ Tests link command line when a library and additional flags are specified. """ @@ -236,9 +237,10 @@ def test_c_with_libraries_and_post_flags(stub_c_compiler: CCompiler, ] -def test_c_with_libraries_and_pre_flags(stub_c_compiler: CCompiler, - stub_configuration: BuildConfig, - subproc_record: ExtendedRecorder) -> None: +def test_c_with_libraries_and_pre_flags( + stub_c_compiler: CCompiler, + stub_configuration: BuildConfig, + subproc_record: ExtendedRecorder) -> None: """ Tests link command line when a library and additional flags are specified. """ @@ -367,7 +369,8 @@ def test_linker_inheriting() -> None: assert str(err.value) == "Unknown library name: 'does_not_exist'" -def test_linker_profile_flags_inheriting(stub_c_compiler): +def test_linker_profile_flags_inheriting(stub_c_compiler, + stub_configuration): """ Tests nested compiler and nested linker with inherited profiling flags. """ @@ -386,7 +389,8 @@ def test_linker_profile_flags_inheriting(stub_c_compiler): count += 2 # One set f0-f3 from the compiler wrapper, one from the wrapped linker - assert (linker_wrapper.get_profile_flags("derived") == + stub_configuration.set_profile("derived") + assert (linker_wrapper.get_profile_flags(stub_configuration) == ["-f0", "-f1", "-f2", "-f3", "-f0", "-f1", "-f2", "-f3"]) diff --git a/tests/unit_tests/tools/test_tool.py b/tests/unit_tests/tools/test_tool.py index bbfa97380..7defe84b9 100644 --- a/tests/unit_tests/tools/test_tool.py +++ b/tests/unit_tests/tools/test_tool.py @@ -139,7 +139,7 @@ def test_tool_flags_no_profile() -> None: assert tool.get_flags() == ["-a", "-b", "-c"] -def test_tool_profiles() -> None: +def test_tool_profiles(stub_configuration) -> None: '''Test that profiles work as expected. These tests use internal implementation details of ProfileFlags, but we need to test that the exposed flag-related API works as expected @@ -151,16 +151,18 @@ def test_tool_profiles() -> None: assert tool.get_flags() == [] # Define a profile with no inheritance + stub_configuration.set_profile("mode1") tool.define_profile("mode1") - assert tool.get_flags("mode1") == [] + assert tool.get_flags(stub_configuration) == [] tool.add_flags("-flag1", "mode1") - assert tool.get_flags("mode1") == ["-flag1"] + assert tool.get_flags(stub_configuration) == ["-flag1"] # Define a profile with inheritance tool.define_profile("mode2", "mode1") - assert tool.get_flags("mode2") == ["-flag1"] + stub_configuration.set_profile("mode2") + assert tool.get_flags(stub_configuration) == ["-flag1"] tool.add_flags("-flag2", "mode2") - assert tool.get_flags("mode2") == ["-flag1", "-flag2"] + assert tool.get_flags(stub_configuration) == ["-flag1", "-flag2"] class TestToolRun: diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index 3b75c69ed..958db6230 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -276,8 +276,12 @@ def test_tool_repository_full_path(fake_process: FakeProcess) -> None: in which case the right tool should be returned with an updated exec name that uses the path. ''' + fake_process.register(['/usr/bin/gfortran', '--version'], + stdout='GNU Fortran (foo) 1.2.3') tr = ToolRepository() gfortran = tr.get_tool(Category.FORTRAN_COMPILER, "/usr/bin/gfortran") + gfortran._is_available = True + gfortran._version = (1, 2) assert isinstance(gfortran, Gfortran) assert gfortran.name == "gfortran" assert gfortran.exec_name == "gfortran" From ce0e630812f175ccf4754f34be09c0cfa78fb536 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 8 Oct 2025 13:15:53 +1100 Subject: [PATCH 02/30] #313 Minor code cleanup. --- source/fab/tools/flags.py | 28 +++++++++++++++------------- source/fab/tools/versioning.py | 2 +- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index 3d3b136c1..2db4bb3f5 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -8,7 +8,6 @@ This file contains the flag classes used to manage command line flags for tools, especially path-specific flags for compiler. -ProfileFlags AbstractFlags: The base class for a set of flags. @@ -21,11 +20,13 @@ MatchFlags(AlwaysFlags) Flags that are only applied if a wildcard search matches the - source file. + source file. This will likely require to make sure that the full + path is specified (or the pattern starts with `*`). ContainFlags(AlwaysFlags) Flags that are only applied if the file path contains the specified - string. + string. The difference to MatchFlags is that ContainFlags do not + need the full path to be specified. Flags: Manages a list of flags, each of which is an instance of an AbstractFlag. @@ -79,9 +80,10 @@ def get_flags(self, This function returns the list of flags to be used for the given filename. - :param config: the config object (used for paths in templates) - :param file_path: the file path of the file, not used in this - class. + :param config: the config object (required for paths in templated + strings) + :param file_path: the file path of the file. This might not be used + in all implementations. :returns: the list of flags to use. """ @@ -106,7 +108,7 @@ class AlwaysFlags(AbstractFlags): This class represents a list of flags that is always to be used, independent of the path of the source file. It also provides template functionality. This class also acts as convenient base - class for all flags that add flags. + class for all applications that add path-independent flags. :param flags: a string or list of strings with command line flags. """ @@ -124,17 +126,19 @@ def __eq__(self, other: "AlwaysFlags") -> bool: """ :returns: if two AlwaysFlags are identical """ - print("ALWAYS=", self, other, type(self), type(other)) return (type(self) == type(other) and self._flags == other._flags) - def replace_template(self, - string_list: List[str], + @staticmethod + def replace_template(string_list: List[str], config: Optional["BuildConfig"] = None, file_path: Optional[Path] = None) -> List[str]: """This function replaces all `$relative`, `$source`, and `$output` in the string or list of string with the values taken from the config object and the file path. + It is implemented as an abstract method (instead of acting on + self._flags) since templating is also supported in patterns, to + the same code here can be re-used). :param string_list: list of strings, which will get all template arguments replaced. @@ -170,7 +174,7 @@ def get_flags(self, :returns: the list of flags to use. """ - return self.replace_template(self._flags, config, file_path) + return AlwaysFlags.replace_template(self._flags, config, file_path) def remove_flag(self, remove_flag: str, has_parameter: bool = False): '''Removes all occurrences of `remove_flag` in flags. @@ -185,8 +189,6 @@ def remove_flag(self, remove_flag: str, has_parameter: bool = False): :param has_parameter: if the flag to remove takes a parameter ''' - # TODO #313: Check if we can use an OrderedDict and get O(1) - # behaviour here (since ordering of flags can be important) i = 0 flag_len = len(remove_flag) while i < len(self._flags): diff --git a/source/fab/tools/versioning.py b/source/fab/tools/versioning.py index 410cc250f..7e602f4cd 100644 --- a/source/fab/tools/versioning.py +++ b/source/fab/tools/versioning.py @@ -45,7 +45,7 @@ def __init__(self): def current_commit(self, folder: Optional[Union[Path, str]] = None) -> str: ''':returns: the hash of the current commit. - :param folder: the folder for which to determine the current commitf + :param folder: the folder for which to determine the current commit (defaults to .). ''' folder = folder or '.' From 22bbca76698188a1e809ee170b74da4616b00c7f Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 8 Oct 2025 15:06:17 +1100 Subject: [PATCH 03/30] #313 Renamed Flags into FlagList. --- source/fab/steps/compile_c.py | 10 +++---- source/fab/steps/compile_fortran.py | 8 +++--- source/fab/tools/__init__.py | 4 +-- source/fab/tools/compiler_wrapper.py | 7 +++-- source/fab/tools/flags.py | 28 ++++++++++++++++--- tests/unit_tests/tools/test_flags.py | 16 +++++------ .../unit_tests/tools/test_tool_repository.py | 1 + 7 files changed, 48 insertions(+), 26 deletions(-) diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index fc08255c0..695c374d7 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -18,7 +18,7 @@ from fab.metrics import send_metric from fab.parse.c import AnalysedC from fab.steps import check_for_errors, run_mp, step -from fab.tools.flags import Flags, ProfileFlags +from fab.tools.flags import FlagList from fab.tools import Category, Compiler from fab.util import CompiledFile, log_or_dot, Timer, by_type @@ -132,8 +132,8 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): with Timer() as timer: f_f_p = mp_payload.flags.flags_for_path(path=analysed_file.fpath, config=config) - flags = ProfileFlags() - flags.add_flags(f_f_p, config.profile) + flags = FlagList() + flags.add_flags(f_f_p) obj_combo_hash = _get_obj_combo_hash(config, compiler, analysed_file, flags) @@ -165,13 +165,13 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): def _get_obj_combo_hash(config: BuildConfig, - compiler: Compiler, analysed_file, flags: Flags): + compiler: Compiler, analysed_file, flags: FlagList): # get a combo hash of things which matter to the object file we define try: obj_combo_hash = sum([ analysed_file.file_hash, flags.checksum(), - compiler.get_hash(config.profile), + compiler.get_hash(config), ]) except TypeError as err: raise ValueError("could not generate combo hash for " diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index 072862676..b668db66f 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -21,8 +21,8 @@ from fab.metrics import send_metric from fab.parse.fortran import AnalysedFortran from fab.steps import check_for_errors, run_mp, step -from fab.tools import Category, Compiler, Flags -from fab.tools.flags import ProfileFlags +from fab.tools import Category, Compiler +from fab.tools.flags import FlagList from fab.util import (CompiledFile, log_or_dot_finish, log_or_dot, Timer, by_type, file_checksum) @@ -273,7 +273,7 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ compiler = cast(Compiler, compiler) f_f_p = mp_common_args.flags.flags_for_path(path=analysed_file.fpath, config=config) - flags = ProfileFlags(f_f_p, config.profile) + flags = FlagList(f_f_p) mod_combo_hash = _get_mod_combo_hash(config, analysed_file, compiler=compiler) @@ -346,7 +346,7 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ def _get_obj_combo_hash(config: BuildConfig, analysed_file, mp_common_args: MpCommonArgs, - compiler: Compiler, flags: Flags): + compiler: Compiler, flags: FlagList): # get a combo hash of things which matter to the object file we define # todo: don't just silently use 0 for a missing dep hash mod_deps_hashes = { diff --git a/source/fab/tools/__init__.py b/source/fab/tools/__init__.py index 7b6630467..93e19df18 100644 --- a/source/fab/tools/__init__.py +++ b/source/fab/tools/__init__.py @@ -14,7 +14,7 @@ Icx, Ifort, Ifx, Nvc, Nvfortran) from fab.tools.compiler_wrapper import (CompilerWrapper, CrayCcWrapper, CrayFtnWrapper, Mpicc, Mpif90) -from fab.tools.flags import Flags, ProfileFlags +from fab.tools.flags import FlagList, ProfileFlags from fab.tools.linker import Linker from fab.tools.psyclone import Psyclone from fab.tools.rsync import Rsync @@ -39,7 +39,7 @@ "Crayftn", "CrayFtnWrapper", "Fcm", - "Flags", + "FlagList", "FortranCompiler", "Fpp", "Gcc", diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index 27f67c4cf..cd46dda23 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -13,7 +13,7 @@ from fab.tools.category import Category from fab.tools.compiler import Compiler, FortranCompiler -from fab.tools.flags import ProfileFlags +from fab.tools.flags import FlagList if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -125,8 +125,9 @@ def get_all_commandline_options( ''' # We need to distinguish between Fortran and non-Fortran compiler, # since only a Fortran compiler supports the syntax-only flag. - new_flags = ProfileFlags(self.flags.get_flags(config, input_file)) - new_flags.add_flags(add_flags) + new_flags = FlagList(self.flags.get_flags(config, input_file)) + if add_flags: + new_flags.add_flags(add_flags) if self._compiler.category is Category.FORTRAN_COMPILER: # Mypy complains that self._compiler does not take the syntax diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index 2db4bb3f5..5ec402044 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -263,7 +263,9 @@ class ContainFlags(AlwaysFlags): :param pattern: the substring which is used when matching. """ - def __init__(self, flags: List[str], pattern: str) -> None: + def __init__(self, + flags: Union[str, List[str]], + pattern: str) -> None: super().__init__(flags) self._pattern = pattern @@ -291,7 +293,7 @@ def get_flags(self, return super().get_flags(config, file_path) -class Flags(List[AbstractFlags]): +class FlagList(List[AbstractFlags]): '''This class represents a list of parameters for a tool. It is a list with some additional functionality. @@ -328,6 +330,24 @@ def get_flags(self, return all_flags_resolved + def checksum(self, + config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> int: + """ + :param config: the config object (used for templating) + :param file_path: the file path of the source file, used for + path-specific flags. + + :returns: a checksum of the flags. + """ + + if not file_path: + # If no path, provide a dummy path + file_path = Path() + + resolve_flags: List[str] = self.get_flags(config, file_path) + return string_checksum(str(resolve_flags)) + def add_flags(self, new_flags: Union[AbstractFlags, str, List[str]]) -> None: '''Adds the specified flags to the list of flags. @@ -379,7 +399,7 @@ def __init__(self: "ProfileFlags", profile: str = "") -> None: # Stores the flags for each profile mode. The key is the (lower case) # name of the profile mode, and it contains a list of flags - self._profiles: Dict[str, Flags] = {"": Flags()} + self._profiles: Dict[str, FlagList] = {"": FlagList()} # This dictionary stores an optional inheritance, where one mode # 'inherits' the flags from a different mode (recursively) @@ -470,7 +490,7 @@ def define_profile(self, ''' if name in self._profiles: raise KeyError(f"Profile '{name}' is already defined.") - self._profiles[name.lower()] = Flags() + self._profiles[name.lower()] = FlagList() if inherit_from is not None: if inherit_from not in self._profiles: diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index 6b957d3f7..402c07d1e 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -10,7 +10,7 @@ from pathlib import Path import pytest -from fab.tools.flags import (AlwaysFlags, ContainFlags, Flags, MatchFlags, +from fab.tools.flags import (AlwaysFlags, ContainFlags, FlagList, MatchFlags, ProfileFlags) from fab.util import string_checksum @@ -102,14 +102,14 @@ def test_contain_flags() -> None: assert cf.get_flags(file_path=file_path) == ["-g", "/my"] -def test_flags_constructor(): +def test_flag_list_constructor(): '''Tests the constructor of Flags.''' - f1 = Flags() + f1 = FlagList() assert isinstance(f1, list) # pylint: disable-next=use-implicit-booleaness-not-comparison assert f1 == [] - f2 = Flags(["a"]) + f2 = FlagList(["a"]) assert isinstance(f2, list) assert f2.get_flags() == ["a"] @@ -123,7 +123,7 @@ def test_flags_constructor(): def test_flags_adding(): '''Tests adding flags.''' - f1 = Flags() + f1 = FlagList() # pylint: disable-next=use-implicit-booleaness-not-comparison assert f1.get_flags() == [] f1.add_flags("-a") @@ -136,7 +136,7 @@ def test_flags_adding(): # Check functionality when adding a flag object: af1 = AlwaysFlags("-g") - f1 = Flags(af1) + f1 = FlagList(af1) assert f1 == [af1] assert f1.get_flags() == ["-g"] @@ -151,13 +151,13 @@ def test_remove_flags(): tests for AlwaysFlags, just to ensure that the calls are getting forwarded from Flags to the AlwaysFlags implementation. ''' - flags = Flags() + flags = FlagList() flags.remove_flag("-c", False) # pylint: disable-next=use-implicit-booleaness-not-comparison assert flags == [] all_flags = ['a.f90', '-c', '-o', 'a.o', '-fsyntax-only', "-J", "/tmp"] - flags = Flags(all_flags) + flags = FlagList(all_flags) assert flags.get_flags() == all_flags with pytest.warns(UserWarning, match="Removing managed flag"): flags.remove_flag("-c") diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index 169e585f9..d026a3234 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -288,6 +288,7 @@ def test_tool_repository_full_path(fake_process: FakeProcess) -> None: stdout='GNU Fortran (gcc) 1.2.3') tr = ToolRepository() gfortran = tr.get_tool(Category.FORTRAN_COMPILER, "/usr/bin/gfortran") + gfortran = cast(Compiler, gfortran) gfortran._is_available = True gfortran._version = (1, 2) assert isinstance(gfortran, Gfortran) From 0b3b568941edd6f32d884659c732f48c73f79a6c Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 8 Oct 2025 15:48:03 +1100 Subject: [PATCH 04/30] #313 Removed duplicated test. --- tests/unit_tests/tools/test_flags.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index 402c07d1e..1dad94eca 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -113,13 +113,6 @@ def test_flag_list_constructor(): assert isinstance(f2, list) assert f2.get_flags() == ["a"] - af = AlwaysFlags() - assert af.get_flags() == [] - af = AlwaysFlags("-g") - assert af.get_flags() == ["-g"] - af = AlwaysFlags(["-g", "-O2"]) - assert af.get_flags() == ["-g", "-O2"] - def test_flags_adding(): '''Tests adding flags.''' From f1722265360b1923342bacaf1288aa30116ebee6 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 8 Oct 2025 22:40:13 +1100 Subject: [PATCH 05/30] #313 Replaced FlagsConfig with FlagList, and use FlagList internally to handle flags and path-flags. --- source/fab/build_config.py | 53 +++-------------- source/fab/steps/compile_c.py | 26 ++++---- source/fab/steps/compile_fortran.py | 59 +++++++++++-------- source/fab/steps/preprocess.py | 21 +++---- source/fab/tools/flags.py | 21 ++++++- source/fab/tools/preprocessor.py | 6 +- .../unit_tests/steps/test_compile_fortran.py | 26 ++++---- tests/unit_tests/tools/test_flags.py | 13 ++++ 8 files changed, 113 insertions(+), 112 deletions(-) diff --git a/source/fab/build_config.py b/source/fab/build_config.py index 38ccea65d..9f250e799 100644 --- a/source/fab/build_config.py +++ b/source/fab/build_config.py @@ -295,10 +295,16 @@ def _finalise_metrics(self, start_time, steps_timer): # todo: better name? perhaps PathFlags? +# todo: This is going to be replaced with tools.flags.MatchFlags + class AddFlags(): """ Add command-line flags when our path filter matches. - Generally used inside a :class:`~fab.build_config.FlagsConfig`. + This class is deprecated, use the new + :class:`~fab.tools.flags.FlagList` and + :class:`~fab.tools.flags.MatchFlag` instead. + The compile and preprocess steps will convert AddFlags into + MatchFlags """ def __init__(self, match: str, flags: List[str]): @@ -354,48 +360,3 @@ def run(self, fpath: Path, input_flags: List[str], config): # add our flags input_flags += add_flags - - -class FlagsConfig(): - """ - Return command-line flags for a given path. - - Simply allows appending flags but may evolve to also replace and - remove flags. - - """ - def __init__(self, common_flags: Optional[List[str]] = None, - path_flags: Optional[List[AddFlags]] = None): - """ - :param common_flags: - List of flags to apply to all files. E.g `['-O2']`. - :param path_flags: - List of :class:`~fab.build_config.AddFlags` objects which apply - flags to selected paths. - - """ - self.common_flags = common_flags or [] - self.path_flags = path_flags or [] - - # todo: there's templating both in this method and the run method it calls. - # make sure it's all properly documented and rationalised. - def flags_for_path(self, path: Path, config): - """ - Get all the flags for a given file, in a reproducible order. - - :param path: - The file path for which we want command-line flags. - :param config: - The config contains the source root and project workspace. - - """ - # We COULD make the user pass these template params to the constructor - # but we have a design requirement to minimise the config burden on - # the user, so we take care of it for them here instead. - params = {'source': config.source_root, 'output': config.build_output} - flags = [Template(i).substitute(params) for i in self.common_flags] - - for flags_modifier in self.path_flags: - flags_modifier.run(path, flags, config=config) - - return flags diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index 695c374d7..3e94104e3 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -14,7 +14,7 @@ from fab import FabException from fab.artefacts import (ArtefactsGetter, ArtefactSet, ArtefactStore, FilterBuildTrees) -from fab.build_config import BuildConfig, FlagsConfig +from fab.build_config import AddFlags, BuildConfig from fab.metrics import send_metric from fab.parse.c import AnalysedC from fab.steps import check_for_errors, run_mp, step @@ -32,12 +32,13 @@ class MpCommonArgs: '''A simple class to pass arguments to subprocesses.''' config: BuildConfig - flags: FlagsConfig + flag_list: FlagList @step -def compile_c(config, common_flags: Optional[List[str]] = None, - path_flags: Optional[List] = None, +def compile_c(config: BuildConfig, + common_flags: Optional[List[str]] = None, + path_flags: Optional[List[AddFlags]] = None, source: Optional[ArtefactsGetter] = None): """ Compiles all C files in all build trees, creating or extending a set of @@ -68,7 +69,8 @@ def compile_c(config, common_flags: Optional[List[str]] = None, common_flags = common_flags or [] - flags = FlagsConfig(common_flags=common_flags, path_flags=path_flags) + flag_list = FlagList(common_flags, add_flags=path_flags) + source_getter = source or DEFAULT_SOURCE_GETTER # gather all the source to compile, for all build trees, into one big lump @@ -84,7 +86,7 @@ def compile_c(config, common_flags: Optional[List[str]] = None, openmp=config.openmp) logger.info(f'C compiler is {compiler}') - mp_payload = MpCommonArgs(config=config, flags=flags) + mp_payload = MpCommonArgs(config=config, flag_list=flag_list) mp_items = [(fpath, mp_payload) for fpath in to_compile] # compile everything in one go @@ -130,12 +132,9 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): # to cast it to be a Compiler. compiler = cast(Compiler, compiler) with Timer() as timer: - f_f_p = mp_payload.flags.flags_for_path(path=analysed_file.fpath, - config=config) - flags = FlagList() - flags.add_flags(f_f_p) + flag_list = mp_payload.flag_list obj_combo_hash = _get_obj_combo_hash(config, compiler, - analysed_file, flags) + analysed_file, flag_list) obj_file_prebuild = (config.prebuild_folder / f'{analysed_file.fpath.stem}.' @@ -148,10 +147,11 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): else: obj_file_prebuild.parent.mkdir(parents=True, exist_ok=True) log_or_dot(logger, f'CompileC compiling {analysed_file.fpath}') + flags = flag_list.get_flags(config, analysed_file.fpath) try: compiler.compile_file(analysed_file.fpath, obj_file_prebuild, config=config, - add_flags=f_f_p) + add_flags=flags) except RuntimeError as err: return FabException(f"error compiling " f"{analysed_file.fpath}:\n{err}") @@ -170,7 +170,7 @@ def _get_obj_combo_hash(config: BuildConfig, try: obj_combo_hash = sum([ analysed_file.file_hash, - flags.checksum(), + flags.checksum(config, analysed_file.fpath), compiler.get_hash(config), ]) except TypeError as err: diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index b668db66f..d7744056d 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -17,11 +17,12 @@ from fab.artefacts import (ArtefactsGetter, ArtefactSet, ArtefactStore, FilterBuildTrees) -from fab.build_config import BuildConfig, FlagsConfig +from fab.build_config import BuildConfig from fab.metrics import send_metric from fab.parse.fortran import AnalysedFortran from fab.steps import check_for_errors, run_mp, step -from fab.tools import Category, Compiler +from fab.tools.category import Category +from fab.tools.compiler import Compiler, FortranCompiler from fab.tools.flags import FlagList from fab.util import (CompiledFile, log_or_dot_finish, log_or_dot, Timer, by_type, file_checksum) @@ -36,7 +37,7 @@ class MpCommonArgs: """Arguments to be passed into the multiprocessing function, alongside the filenames.""" config: BuildConfig - flags: FlagsConfig + flag_list: FlagList mod_hashes: Dict[str, int] syntax_only: bool @@ -86,15 +87,15 @@ def compile_fortran(config: BuildConfig, if len(uncompiled) == 0: return - compiler, flags_config = handle_compiler_args(config, common_flags, - path_flags) + compiler, flag_list = handle_compiler_args(config, common_flags, + path_flags) # Set module output folder: compiler.set_module_output_path(config.build_output) syntax_only = compiler.has_syntax_only and config.two_stage # build the arguments passed to the multiprocessing function mp_common_args = MpCommonArgs( - config=config, flags=flags_config, + config=config, flag_list=flag_list, mod_hashes=mod_hashes, syntax_only=syntax_only) if syntax_only: @@ -128,8 +129,9 @@ def compile_fortran(config: BuildConfig, store_artefacts(compiled, build_lists, config.artefact_store) -def handle_compiler_args(config: BuildConfig, common_flags=None, - path_flags=None): +def handle_compiler_args(config: BuildConfig, + common_flags=None, + path_flags=None) -> Tuple[FortranCompiler, FlagList]: # Command line tools are sometimes specified with flags attached. compiler = config.tool_box[Category.FORTRAN_COMPILER] @@ -138,16 +140,15 @@ def handle_compiler_args(config: BuildConfig, common_flags=None, 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) + compiler = cast(FortranCompiler, compiler) logger.info( f'Fortran compiler is {compiler} {compiler.get_version_string()}') # Collate the flags from 1) flags env and 2) parameters. common_flags = common_flags or [] - flags_config = FlagsConfig(common_flags=common_flags, - path_flags=path_flags) - return compiler, flags_config + flag_list = FlagList(common_flags, add_flags=path_flags) + return compiler, flag_list def compile_pass(config, compiled: Dict[Path, CompiledFile], @@ -269,17 +270,16 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ 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) - f_f_p = mp_common_args.flags.flags_for_path(path=analysed_file.fpath, - config=config) - flags = FlagList(f_f_p) + # this is a FortranCompiler + compiler = cast(FortranCompiler, compiler) + flag_list = mp_common_args.flag_list mod_combo_hash = _get_mod_combo_hash(config, analysed_file, compiler=compiler) obj_combo_hash = _get_obj_combo_hash(config, analysed_file, mp_common_args=mp_common_args, - compiler=compiler, flags=flags) + compiler=compiler, + flag_list=flag_list) # calculate the incremental/prebuild artefact filenames obj_file_prebuild = ( @@ -296,9 +296,10 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ [obj_file_prebuild] + mod_file_prebuilds)) if not all(prebuilds_exist): # compile + logger.debug(f'CompileFortran compiling {analysed_file.fpath}') + flags = flag_list.get_flags(config, analysed_file.fpath) try: - logger.debug(f'CompileFortran compiling {analysed_file.fpath}') - compile_file(analysed_file.fpath, f_f_p, + compile_file(analysed_file.fpath, flags, output_fpath=obj_file_prebuild, mp_common_args=mp_common_args) except Exception as err: @@ -345,8 +346,10 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ def _get_obj_combo_hash(config: BuildConfig, - analysed_file, mp_common_args: MpCommonArgs, - compiler: Compiler, flags: FlagList): + analysed_file: AnalysedFortran, + mp_common_args: MpCommonArgs, + compiler: Compiler, + flag_list: FlagList): # get a combo hash of things which matter to the object file we define # todo: don't just silently use 0 for a missing dep hash mod_deps_hashes = { @@ -355,7 +358,7 @@ def _get_obj_combo_hash(config: BuildConfig, try: obj_combo_hash = sum([ analysed_file.file_hash, - flags.checksum(), + flag_list.checksum(config, analysed_file.fpath), sum(mod_deps_hashes.values()), compiler.get_hash(config), ]) @@ -378,7 +381,10 @@ def _get_mod_combo_hash(config, analysed_file, compiler: Compiler): return mod_combo_hash -def compile_file(analysed_file, flags, output_fpath, mp_common_args): +def compile_file(input_fpath: Path, + flags: List[str], + output_fpath: Path, + mp_common_args: MpCommonArgs) -> None: """ Call the compiler. @@ -390,11 +396,12 @@ def compile_file(analysed_file, flags, output_fpath, mp_common_args): """ output_fpath.parent.mkdir(parents=True, exist_ok=True) - # tool config = mp_common_args.config compiler = config.tool_box[Category.FORTRAN_COMPILER] + compiler = cast(FortranCompiler, compiler) - compiler.compile_file(input_file=analysed_file, output_file=output_fpath, + compiler.compile_file(input_file=input_fpath, + output_file=output_fpath, config=config, add_flags=flags, syntax_only=mp_common_args.syntax_only) diff --git a/source/fab/steps/preprocess.py b/source/fab/steps/preprocess.py index 8698a1eff..52eb0bd1e 100644 --- a/source/fab/steps/preprocess.py +++ b/source/fab/steps/preprocess.py @@ -15,10 +15,11 @@ from fab.artefacts import (ArtefactSet, ArtefactsGetter, SuffixFilter, CollectionGetter) -from fab.build_config import BuildConfig, FlagsConfig +from fab.build_config import BuildConfig from fab.metrics import send_metric from fab.steps import check_for_errors, run_mp, step from fab.tools import Category, Cpp, CppFortran, Preprocessor +from fab.tools.flags import FlagList from fab.util import (log_or_dot_finish, input_to_output_fpath, log_or_dot, suffix_filter, Timer, by_type) @@ -31,7 +32,7 @@ class MpCommonArgs(): config: BuildConfig output_suffix: str preprocessor: Preprocessor - flags: FlagsConfig + flag_list: FlagList name: str @@ -59,15 +60,15 @@ def pre_processor(config: BuildConfig, preprocessor: Preprocessor, :param output_suffix: Suffix for output files. :param common_flags: - Used to construct a :class:`~fab.config.FlagsConfig` object. + Path-independent flags for the preprocessor to use. :param path_flags: - Used to construct a :class:`~fab.build_config.FlagsConfig` object. + Path-dependent flags for the preprocessor to use. :param name: Human friendly name for logger output, with sensible default. """ common_flags = common_flags or [] - flags = FlagsConfig(common_flags=common_flags, path_flags=path_flags) + flag_list = FlagList(common_flags, add_flags=path_flags) logger.info(f"preprocessor is '{preprocessor.name}'.") @@ -78,7 +79,7 @@ def pre_processor(config: BuildConfig, preprocessor: Preprocessor, config=config, output_suffix=output_suffix, preprocessor=preprocessor, - flags=flags, + flag_list=flag_list, name=name, ) @@ -112,12 +113,12 @@ def process_artefact(arg: Tuple[Path, MpCommonArgs]): else: output_fpath.parent.mkdir(parents=True, exist_ok=True) - params = args.flags.flags_for_path(path=input_fpath, config=args.config) - + flags = args.flag_list.get_flags(args.config, input_fpath) log_or_dot(logger, f"PreProcessor running with parameters: " - f"'{' '.join(params)}'.'") + f"'{' '.join(flags)}'.'") try: - args.preprocessor.preprocess(input_fpath, output_fpath, params) + args.preprocessor.preprocess(input_fpath, output_fpath, + flags) except Exception as err: raise Exception(f"error preprocessing {input_fpath}:\n" f"{err}") from err diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index 5ec402044..d140d7311 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -56,7 +56,7 @@ from fab.util import string_checksum if TYPE_CHECKING: - from fab.build_config import BuildConfig + from fab.build_config import AddFlags, BuildConfig logger = logging.getLogger(__name__) @@ -122,10 +122,12 @@ def __init__(self, flags: Optional[Union[str, List[str]]] = None) -> None: else: self._flags = [] - def __eq__(self, other: "AlwaysFlags") -> bool: + def __eq__(self, other: object) -> bool: """ :returns: if two AlwaysFlags are identical """ + if not isinstance(other, AlwaysFlags): + return NotImplemented return (type(self) == type(other) and self._flags == other._flags) @@ -298,18 +300,31 @@ class FlagList(List[AbstractFlags]): list with some additional functionality. :param list_of_flags: List of parameters to initialise this object with. + :param path_flags: List of old-style PathFlags, which will be converted + to the new MatchFlags ''' def __init__( self, list_of_flags: Optional[Union[AbstractFlags, str, - List[str]]] = None) -> None: + List[str]]] = None, + add_flags: Optional[Union["AddFlags", + List["AddFlags"]]] = None) -> None: self._logger = logging.getLogger(__name__) super().__init__() if isinstance(list_of_flags, (str, list)): self.append(AlwaysFlags(list_of_flags)) elif list_of_flags: self.append(list_of_flags) + if add_flags: + # TODO: circular import otherwise + from fab.build_config import AddFlags + if isinstance(add_flags, AddFlags): + add_flags = [add_flags] + # Convert old-style AddFlags to the new MatchFlags: + for add_flag in add_flags: + self.add_flags(MatchFlags(add_flag.flags, + add_flag.match)) def get_flags(self, config: Optional["BuildConfig"] = None, diff --git a/source/fab/tools/preprocessor.py b/source/fab/tools/preprocessor.py index d93c26883..c5b934455 100644 --- a/source/fab/tools/preprocessor.py +++ b/source/fab/tools/preprocessor.py @@ -10,7 +10,7 @@ """ from pathlib import Path -from typing import List, Optional, Union +from typing import List, Optional, Sequence, Union from fab.tools.category import Category from fab.tools.tool import Tool @@ -32,7 +32,7 @@ def __init__(self, name: str, exec_name: Union[str, Path], self._version = None def preprocess(self, input_file: Path, output_file: Path, - add_flags: Union[None, List[Union[Path, str]]] = None): + add_flags: Optional[Sequence[Union[Path, str]]] = None): '''Calls the preprocessor to process the specified input file, creating the requested output file. @@ -43,7 +43,7 @@ def preprocess(self, input_file: Path, output_file: Path, params: List[Union[str, Path]] = [] if add_flags: # Make a copy to avoid modifying the caller's list - params = add_flags[:] + params = list(add_flags) # Input and output files come as the last two parameters params.extend([input_file, output_file]) diff --git a/tests/unit_tests/steps/test_compile_fortran.py b/tests/unit_tests/steps/test_compile_fortran.py index a991b97dd..ba016c6ed 100644 --- a/tests/unit_tests/steps/test_compile_fortran.py +++ b/tests/unit_tests/steps/test_compile_fortran.py @@ -7,7 +7,7 @@ from pytest_subprocess.fake_process import FakeProcess from fab.artefacts import ArtefactSet, ArtefactStore -from fab.build_config import BuildConfig, FlagsConfig +from fab.build_config import BuildConfig from fab.parse.fortran import AnalysedFortran from fab.steps.compile_fortran import ( compile_pass, get_compile_next, @@ -15,6 +15,7 @@ store_artefacts ) from fab.tools.category import Category +from fab.tools.flags import FlagList from fab.tools.tool_box import ToolBox from fab.util import CompiledFile @@ -97,7 +98,7 @@ def test_vanilla(self, analysed_files, stub_tool_box: ToolBox, config = BuildConfig('proj', stub_tool_box, fab_workspace=tmp_path) mp_common_args = MpCommonArgs(config, - FlagsConfig(), + FlagList(), {}, syntax_only=True) uncompiled_result = compile_pass(config=config, @@ -172,8 +173,7 @@ def test_vanilla(self): @fixture(scope='function') def content(stub_tool_box, fs: FakeFilesystem): flags = ['flag1', 'flag2'] - flags_config = Mock() - flags_config.flags_for_path.return_value = flags + flag_list = FlagList(flags) analysed_file = AnalysedFortran(fpath=Path('foofile'), file_hash=34567) analysed_file.add_module_dep('mod_dep_1') @@ -183,7 +183,7 @@ def content(stub_tool_box, fs: FakeFilesystem): mp_common_args = MpCommonArgs( config=BuildConfig('proj', stub_tool_box, fab_workspace=Path('/fab')), - flags=flags_config, + flag_list=flag_list, mod_hashes={'mod_dep_1': 12345, 'mod_dep_2': 23456}, syntax_only=False, ) @@ -349,15 +349,19 @@ def test_file_hash(self, content, fs: FakeFilesystem, fake_process: FakeProcess) '/fab/proj/build_output/_prebuild/mod_def_2.188dd00a9.mod' ).read_text() == "Second module" - def test_flags_hash(self, content, fs: FakeFilesystem, fake_process: FakeProcess) -> None: + def test_flags_hash(self, + content, + fs: FakeFilesystem, + fake_process: FakeProcess) -> None: """ Tests changing compiler arguments changes generated object and module hashes. Not source modules. """ - mp_common_args, flags, analysed_file = content + mp_common_args, _, analysed_file = content + # Change the flags in mp_common_args flags = ['flag1', 'flag3'] - mp_common_args.flags.flags_for_path.return_value = flags + mp_common_args.flag_list[0]._flags = flags fake_process.register(['sfc', '--version'], stdout='1.2.3') record = fake_process.register(['sfc', fake_process.any()]) @@ -412,7 +416,7 @@ def test_deps_hash(self, content, fs: FakeFilesystem, fake_process: FakeProcess) The generated object hash should change but the generated module hashes should not. """ - mp_common_args, flags, analysed_file = content + mp_common_args, _, analysed_file = content mp_common_args.mod_hashes['mod_dep_1'] += 1 @@ -467,7 +471,7 @@ def test_mod_missing(self, content, fs: FakeFilesystem, fake_process: FakeProces """ Tests compilation on missing module. """ - mp_common_args, flags, analysed_file = content + mp_common_args, _, analysed_file = content fake_process.register(['sfc', '--version'], stdout='1.2.3') record = fake_process.register(['sfc', fake_process.any()]) @@ -526,7 +530,7 @@ def test_obj_missing(self, content, version, mod_hash, obj_hash, Tests compilation of missing object. Also tests that different compiler version numbers lead to different hashes. """ - mp_common_args, flags, analysed_file = content + mp_common_args, _, analysed_file = content fake_process.register(['sfc', '--version'], stdout=version) record = fake_process.register(['sfc', fake_process.any()]) diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index 1dad94eca..ceab1f91a 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -10,6 +10,7 @@ from pathlib import Path import pytest +from fab.build_config import AddFlags from fab.tools.flags import (AlwaysFlags, ContainFlags, FlagList, MatchFlags, ProfileFlags) from fab.util import string_checksum @@ -319,3 +320,15 @@ def test_profile_flags_errors_invalid_profile_name(stub_configuration): pf.checksum(stub_configuration, Path("/some/path")) assert ("checksum: Profile 'does_not_exist' is not defined." in str(err.value)) + + +def test_old_addflags(): + """ + Tests that old-style AddFlags are converted to MatchFlags. + """ + add_flags = AddFlags(match="/some/pattern", flags=["-g", "-O0"]) + flag_list = FlagList(add_flags=[add_flags]) + match_flag = flag_list[0] + assert isinstance(match_flag, MatchFlags) + assert match_flag._pattern == "/some/pattern" + assert match_flag._flags == ["-g", "-O0"] From a49236be8bfcd1d08d18acee9d36c9c24b223c5e Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 9 Oct 2025 18:10:12 +1100 Subject: [PATCH 06/30] #313 Take the file path into account when computing the hash of compiler flags to support path-specific flags. Compute the hast as crc of the combined string (and not by adding three crc for three strings). --- source/fab/steps/compile_c.py | 2 +- source/fab/steps/compile_fortran.py | 4 +- source/fab/tools/compiler.py | 27 ++++- tests/unit_tests/steps/test_compile_c.py | 10 +- .../unit_tests/steps/test_compile_fortran.py | 105 ++++++++++-------- tests/unit_tests/tools/test_compiler.py | 51 +++++++-- .../unit_tests/tools/test_compiler_wrapper.py | 17 +-- 7 files changed, 139 insertions(+), 77 deletions(-) diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index 3e94104e3..fb5b4517f 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -171,7 +171,7 @@ def _get_obj_combo_hash(config: BuildConfig, obj_combo_hash = sum([ analysed_file.file_hash, flags.checksum(config, analysed_file.fpath), - compiler.get_hash(config), + compiler.get_hash(config, analysed_file.fpath), ]) except TypeError as err: raise ValueError("could not generate combo hash for " diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index d7744056d..3d8d647eb 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -360,7 +360,7 @@ def _get_obj_combo_hash(config: BuildConfig, analysed_file.file_hash, flag_list.checksum(config, analysed_file.fpath), sum(mod_deps_hashes.values()), - compiler.get_hash(config), + compiler.get_hash(config, analysed_file.fpath), ]) except TypeError as err: raise ValueError("Could not generate combo hash " @@ -373,7 +373,7 @@ def _get_mod_combo_hash(config, analysed_file, compiler: Compiler): try: mod_combo_hash = sum([ analysed_file.file_hash, - compiler.get_hash(config), + compiler.get_hash(config, analysed_file.fpath), ]) except TypeError as err: raise ValueError("Could not generate combo " diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 86e60199f..43c7dcafa 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -12,11 +12,11 @@ from pathlib import Path import warnings from typing import cast, List, Optional, Tuple, TYPE_CHECKING, Union -import zlib from fab.tools.category import Category from fab.tools.flags import AlwaysFlags from fab.tools.tool import CompilerSuiteTool +from fab.util import string_checksum if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -104,13 +104,23 @@ def output_flag(self) -> str: """ return self._output_flag - def get_hash(self, config: Optional["BuildConfig"] = None) -> int: + def get_hash(self, + config: "BuildConfig", + file_path: Path + ) -> int: """ + Computes a hash code using the name and version of the compiler, + and the compilation flag used by the compiler for the specified + file. + + :param config: The build configuration to use. + :param file_path: Path of the file to compile. :returns: hash of compiler name and version. """ - return (zlib.crc32(self.name.encode()) + - zlib.crc32(str(self.get_flags(config)).encode()) + - zlib.crc32(self.get_version_string().encode())) + all_params = (self.name + + self.get_version_string() + + str(self.get_resolved_flags(config, file_path))) + return string_checksum(all_params) def get_all_commandline_options( self, @@ -167,6 +177,13 @@ def get_flags(self, config: Optional["BuildConfig"] = None) -> List[str]: def get_resolved_flags(self, config: "BuildConfig", file_path: Path) -> List[str]: + """ + :param config: The build configuration to use. + :param file_path: the path to the file to be compiled. + + :returns: the flags actually used when building the specified + path. + """ return self.flags.get_flags(config, file_path) def compile_file(self, input_file: Path, diff --git a/tests/unit_tests/steps/test_compile_c.py b/tests/unit_tests/steps/test_compile_c.py index e10329a22..e5829b39d 100644 --- a/tests/unit_tests/steps/test_compile_c.py +++ b/tests/unit_tests/steps/test_compile_c.py @@ -81,7 +81,7 @@ def test_vanilla(self, content, fake_process.register([ 'scc', '-c', '-I', 'foo/include', '-Dhello', 'foo.c', - '-o', str(config.prebuild_folder / 'foo.18f203cab.o') + '-o', str(config.prebuild_folder / 'foo.17ecdc5e7.o') ]) with warns(UserWarning, match="_metric_send_conn not set, " "cannot send metrics"): @@ -92,7 +92,7 @@ def test_vanilla(self, content, # ensure it created the correct artefact collection assert config.artefact_store[ArtefactSet.OBJECT_FILES] == { - None: {config.prebuild_folder / 'foo.18f203cab.o', } + None: {config.prebuild_folder / 'foo.17ecdc5e7.o', } } def test_exception_handling(self, content, @@ -105,7 +105,7 @@ def test_exception_handling(self, content, fake_process.register(['scc', '--version'], stdout='1.2.3') fake_process.register([ 'scc', '-c', 'foo.c', - '-o', str(config.build_output / '_prebuild/foo.101865856.o') + '-o', str(config.build_output / '_prebuild/foo.f133e192.o') ], returncode=1) with raises(RuntimeError): compile_c(config=config) @@ -133,7 +133,7 @@ def test_vanilla(self, content, flags, fake_process: FakeProcess) -> None: # ToDo: Messing with "private" members. # result = _get_obj_combo_hash(config, compiler, analysed_file, flags) - assert result == 5289295574 + assert result == 5015455762 def test_change_file(self, content, flags, fake_process: FakeProcess) -> None: @@ -149,7 +149,7 @@ def test_change_file(self, content, flags, # analysed_file._file_hash += 1 result = _get_obj_combo_hash(config, compiler, analysed_file, flags) - assert result == 5289295575 + assert result == 5015455763 def test_change_flags(self, content, flags, fake_process: FakeProcess) -> None: diff --git a/tests/unit_tests/steps/test_compile_fortran.py b/tests/unit_tests/steps/test_compile_fortran.py index ba016c6ed..5ad5c142a 100644 --- a/tests/unit_tests/steps/test_compile_fortran.py +++ b/tests/unit_tests/steps/test_compile_fortran.py @@ -212,13 +212,13 @@ def test_without_prebuild(self, content, res, artefacts = process_file((analysed_file, mp_common_args)) expect_object_fpath = Path( - '/fab/proj/build_output/_prebuild/foofile.1ff6e93b2.o' + '/fab/proj/build_output/_prebuild/foofile.106dc4755.o' ) assert res == CompiledFile(input_fpath=analysed_file.fpath, output_fpath=expect_object_fpath) assert [call.args for call in record.calls] == [ ['sfc', '-c', 'flag1', 'flag2', 'foofile', - '-o', '/fab/proj/build_output/_prebuild/foofile.1ff6e93b2.o'] + '-o', '/fab/proj/build_output/_prebuild/foofile.106dc4755.o'] ] # check the correct artefacts were generated. @@ -228,16 +228,16 @@ def test_without_prebuild(self, content, pb = mp_common_args.config.prebuild_folder assert artefacts is not None assert set(artefacts) == { - pb / 'foofile.1ff6e93b2.o', - pb / 'mod_def_2.188dd00a8.mod', - pb / 'mod_def_1.188dd00a8.mod' + pb / 'foofile.106dc4755.o', + pb / 'mod_def_2.904ab44b.mod', + pb / 'mod_def_1.904ab44b.mod' } assert Path( - '/fab/proj/build_output/_prebuild/mod_def_1.188dd00a8.mod' + '/fab/proj/build_output/_prebuild/mod_def_1.904ab44b.mod' ).read_text() == "First module" assert Path( - '/fab/proj/build_output/_prebuild/mod_def_2.188dd00a8.mod' + '/fab/proj/build_output/_prebuild/mod_def_2.904ab44b.mod' ).read_text() == "Second module" def test_with_prebuild(self, content, @@ -251,17 +251,20 @@ def test_with_prebuild(self, content, fake_process.register(['sfc', '--version'], stdout='1.2.3') record = fake_process.register(['sfc', fake_process.any()]) + hash_foofile = "106dc4755" + hash_mod = "904ab44b" + Path('/fab/proj/build_output/_prebuild').mkdir(parents=True) Path('/fab/proj/build_output/mod_def_1.mod').write_text("First module") Path('/fab/proj/build_output/mod_def_2.mod').write_text("Second module") Path( - '/fab/proj/build_output/_prebuild/mod_def_1.188dd00a8.mod' + f'/fab/proj/build_output/_prebuild/mod_def_1.{hash_mod}.mod' ).write_text("First module") Path( - '/fab/proj/build_output/_prebuild/mod_def_2.188dd00a8.mod' + f'/fab/proj/build_output/_prebuild/mod_def_2.{hash_mod}.mod' ).write_text("Second module") Path( - '/fab/proj/build_output/_prebuild/foofile.1ff6e93b2.o' + f'/fab/proj/build_output/_prebuild/foofile.{hash_foofile}.o' ).write_text("Object file") with warns(UserWarning, @@ -269,7 +272,7 @@ def test_with_prebuild(self, content, res, artefacts = process_file((analysed_file, mp_common_args)) expect_object_fpath = Path( - '/fab/proj/build_output/_prebuild/foofile.1ff6e93b2.o' + f'/fab/proj/build_output/_prebuild/foofile.{hash_foofile}.o' ) # check the correct artefacts were generated. @@ -279,9 +282,9 @@ def test_with_prebuild(self, content, pb = mp_common_args.config.prebuild_folder assert artefacts is not None assert set(artefacts) == { - pb / 'foofile.1ff6e93b2.o', - pb / 'mod_def_2.188dd00a8.mod', - pb / 'mod_def_1.188dd00a8.mod' + pb / f'foofile.{hash_foofile}.o', + pb / f'mod_def_2.{hash_mod}.mod', + pb / f'mod_def_1.{hash_mod}.mod' } assert [call.args for call in record.calls] == [] @@ -289,10 +292,10 @@ def test_with_prebuild(self, content, output_fpath=expect_object_fpath) assert Path( - '/fab/proj/build_output/_prebuild/mod_def_1.188dd00a8.mod' + f'/fab/proj/build_output/_prebuild/mod_def_1.{hash_mod}.mod' ).read_text() == "First module" assert Path( - '/fab/proj/build_output/_prebuild/mod_def_2.188dd00a8.mod' + f'/fab/proj/build_output/_prebuild/mod_def_2.{hash_mod}.mod' ).read_text() == "Second module" def test_file_hash(self, content, fs: FakeFilesystem, fake_process: FakeProcess) -> None: @@ -320,14 +323,16 @@ def test_file_hash(self, content, fs: FakeFilesystem, fake_process: FakeProcess) match="_metric_send_conn not set, cannot send metrics"): res, artefacts = process_file((analysed_file, mp_common_args)) + hash_foofile = "106dc4756" + hash_mod = "904ab44c" expect_object_fpath = Path( - '/fab/proj/build_output/_prebuild/foofile.1ff6e93b3.o' + f'/fab/proj/build_output/_prebuild/foofile.{hash_foofile}.o' ) assert res == CompiledFile(input_fpath=analysed_file.fpath, output_fpath=expect_object_fpath) assert [call.args for call in record.calls] == [ - ['sfc', '-c', 'flag1', 'flag2', 'foofile', - '-o', '/fab/proj/build_output/_prebuild/foofile.1ff6e93b3.o'] + ['sfc', '-c', 'flag1', 'flag2', 'foofile', '-o', + f'/fab/proj/build_output/_prebuild/foofile.{hash_foofile}.o'] ] # check the correct artefacts were generated. @@ -337,16 +342,16 @@ def test_file_hash(self, content, fs: FakeFilesystem, fake_process: FakeProcess) pb = mp_common_args.config.prebuild_folder assert artefacts is not None assert set(artefacts) == { - pb / 'foofile.1ff6e93b3.o', - pb / 'mod_def_2.188dd00a9.mod', - pb / 'mod_def_1.188dd00a9.mod' + pb / f'foofile.{hash_foofile}.o', + pb / f'mod_def_2.{hash_mod}.mod', + pb / f'mod_def_1.{hash_mod}.mod' } assert Path( - '/fab/proj/build_output/_prebuild/mod_def_1.188dd00a9.mod' + f'/fab/proj/build_output/_prebuild/mod_def_1.{hash_mod}.mod' ).read_text() == "First module" assert Path( - '/fab/proj/build_output/_prebuild/mod_def_2.188dd00a9.mod' + f'/fab/proj/build_output/_prebuild/mod_def_2.{hash_mod}.mod' ).read_text() == "Second module" def test_flags_hash(self, @@ -380,14 +385,16 @@ def test_flags_hash(self, match="_metric_send_conn not set, cannot send metrics"): res, artefacts = process_file((analysed_file, mp_common_args)) + hash_foofile = "1079ead2a" + hash_mod = "904ab44b" expect_object_fpath = Path( - '/fab/proj/build_output/_prebuild/foofile.20030f987.o' + f'/fab/proj/build_output/_prebuild/foofile.{hash_foofile}.o' ) assert res == CompiledFile(input_fpath=analysed_file.fpath, output_fpath=expect_object_fpath) assert [call.args for call in record.calls] == [ - ['sfc', '-c', 'flag1', 'flag3', 'foofile', - '-o', '/fab/proj/build_output/_prebuild/foofile.20030f987.o'] + ['sfc', '-c', 'flag1', 'flag3', 'foofile', '-o', + f'/fab/proj/build_output/_prebuild/foofile.{hash_foofile}.o'] ] # check the correct artefacts were generated. @@ -398,9 +405,9 @@ def test_flags_hash(self, pb = mp_common_args.config.prebuild_folder assert artefacts is not None assert set(artefacts) == { - pb / 'foofile.20030f987.o', - pb / 'mod_def_2.188dd00a8.mod', - pb / 'mod_def_1.188dd00a8.mod' + pb / f'foofile.{hash_foofile}.o', + pb / f'mod_def_2.{hash_mod}.mod', + pb / f'mod_def_1.{hash_mod}.mod' } assert Path( @@ -438,13 +445,17 @@ def test_deps_hash(self, content, fs: FakeFilesystem, fake_process: FakeProcess) res, artefacts = process_file((analysed_file, mp_common_args)) expect_object_fpath = Path( - '/fab/proj/build_output/_prebuild/foofile.1ff6e93b3.o' + '/fab/proj/build_output/_prebuild/foofile.106dc4756.o' ) + print("XX", res) + print("YY", analysed_file.fpath, expect_object_fpath) + # XX CompiledFile(foofile, /fab/proj/build_output/_prebuild/foofile.106dc4756.o) + # YY foofile /fab/proj/build_output/_prebuild/foofile.1ff6e93b3.o assert res == CompiledFile(input_fpath=analysed_file.fpath, output_fpath=expect_object_fpath) assert [call.args for call in record.calls] == [ ['sfc', '-c', 'flag1', 'flag2', 'foofile', - '-o', '/fab/proj/build_output/_prebuild/foofile.1ff6e93b3.o'] + '-o', '/fab/proj/build_output/_prebuild/foofile.106dc4756.o'] ] # check the correct artefacts were created. @@ -455,16 +466,16 @@ def test_deps_hash(self, content, fs: FakeFilesystem, fake_process: FakeProcess) pb = mp_common_args.config.prebuild_folder assert artefacts is not None assert set(artefacts) == { - pb / 'foofile.1ff6e93b3.o', - pb / 'mod_def_2.188dd00a8.mod', - pb / 'mod_def_1.188dd00a8.mod' + pb / 'foofile.106dc4756.o', + pb / 'mod_def_2.904ab44b.mod', + pb / 'mod_def_1.904ab44b.mod' } assert Path( - '/fab/proj/build_output/_prebuild/mod_def_1.188dd00a8.mod' + '/fab/proj/build_output/_prebuild/mod_def_1.904ab44b.mod' ).read_text() == "First module" assert Path( - '/fab/proj/build_output/_prebuild/mod_def_2.188dd00a8.mod' + '/fab/proj/build_output/_prebuild/mod_def_2.904ab44b.mod' ).read_text() == "Second module" def test_mod_missing(self, content, fs: FakeFilesystem, fake_process: FakeProcess) -> None: @@ -483,7 +494,7 @@ def test_mod_missing(self, content, fs: FakeFilesystem, fake_process: FakeProces '/fab/proj/build_output/_prebuild/mod_def_2.188dd00a8.mod' ).write_text("Second module") Path( - '/fab/proj/build_output/_prebuild/foofile.1ff6e93b2.o' + '/fab/proj/build_output/_prebuild/foofile.106dc4755.o' ).write_text("Object file") with warns(UserWarning, @@ -491,13 +502,13 @@ def test_mod_missing(self, content, fs: FakeFilesystem, fake_process: FakeProces res, artefacts = process_file((analysed_file, mp_common_args)) expect_object_fpath = Path( - '/fab/proj/build_output/_prebuild/foofile.1ff6e93b2.o' + '/fab/proj/build_output/_prebuild/foofile.106dc4755.o' ) assert res == CompiledFile(input_fpath=analysed_file.fpath, output_fpath=expect_object_fpath) assert [call.args for call in record.calls] == [ ['sfc', '-c', 'flag1', 'flag2', 'foofile', - '-o', '/fab/proj/build_output/_prebuild/foofile.1ff6e93b2.o'] + '-o', '/fab/proj/build_output/_prebuild/foofile.106dc4755.o'] ] # check the correct artefacts were created. @@ -508,21 +519,21 @@ def test_mod_missing(self, content, fs: FakeFilesystem, fake_process: FakeProces pb = mp_common_args.config.prebuild_folder assert artefacts is not None assert set(artefacts) == { - pb / 'foofile.1ff6e93b2.o', - pb / 'mod_def_2.188dd00a8.mod', - pb / 'mod_def_1.188dd00a8.mod' + pb / 'foofile.106dc4755.o', + pb / 'mod_def_2.904ab44b.mod', + pb / 'mod_def_1.904ab44b.mod' } assert Path( - '/fab/proj/build_output/_prebuild/mod_def_1.188dd00a8.mod' + '/fab/proj/build_output/_prebuild/mod_def_1.904ab44b.mod' ).read_text() == "First module" assert Path( - '/fab/proj/build_output/_prebuild/mod_def_2.188dd00a8.mod' + '/fab/proj/build_output/_prebuild/mod_def_2.904ab44b.mod' ).read_text() == "Second module" @mark.parametrize(['version', 'mod_hash', 'obj_hash'], [ - ('1.2.3', '188dd00a8', '1ff6e93b2'), - ('9.8.7', '1b26306b2', '228f499bc') + ('1.2.3', '904ab44b', '106dc4755'), + ('9.8.7', 'ee1d5a65', '164aeed6f') ]) def test_obj_missing(self, content, version, mod_hash, obj_hash, fs: FakeFilesystem, fake_process: FakeProcess) -> None: diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index 13b3827d5..3ae2678d7 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -9,6 +9,7 @@ from pathlib import Path from textwrap import dedent from unittest import mock +from zlib import crc32 from pytest import mark, raises, warns from pytest_subprocess.fake_process import FakeProcess @@ -21,6 +22,7 @@ Icc, Ifort, Icx, Ifx, Nvc, Nvfortran) +from fab.tools.flags import ContainFlags from tests.conftest import arg_list, call_list @@ -108,43 +110,74 @@ def test_compiler_check_available_runtime_error(): assert not cc.check_available() -def test_compiler_hash(): +def test_compiler_hash(stub_configuration): '''Test the hash functionality.''' cc = Gcc() with mock.patch.object(cc, "_version", (5, 6, 7)): - hash1 = cc.get_hash() - assert hash1 == 2991650113 + hash1 = cc.get_hash(stub_configuration, Path('.')) + assert hash1 == 804998173 # A change in the version number must change the hash: with mock.patch.object(cc, "_version", (8, 9)): - hash2 = cc.get_hash() + hash2 = cc.get_hash(stub_configuration, Path('.')) assert hash2 != hash1 # A change in the name must change the hash, again: cc._name = "new_name" - hash3 = cc.get_hash() + hash3 = cc.get_hash(stub_configuration, Path('.')) assert hash3 not in (hash1, hash2) -def test_compiler_hash_compiler_error(): +def test_compiler_path_specific_flags(stub_configuration, + stub_fortran_compiler): + """ + Tests that path-specific flags are used as expected. + """ + fc = stub_fortran_compiler + # Make sure we can get a version number for the stub compiler: + fc._version = (1, 2) + print(fc.name, fc.get_version()) + + contain_flag = ContainFlags(["-myflag"], pattern="myfile") + fc.add_flags("-always-flag") + fc.add_flags(contain_flag) + + flags = fc.get_resolved_flags(stub_configuration, Path(".")) + assert flags == ["-always-flag"] + flags = fc.get_resolved_flags(stub_configuration, + Path("/somewhere/myfile.F90")) + assert flags == ["-always-flag", "-myflag"] + + compiler_info = "some Fortran compiler1.2['-always-flag']" + hash_without = fc.get_hash(stub_configuration, Path('.')) + assert hash_without == crc32(compiler_info.encode()) + + compiler_info = "some Fortran compiler1.2['-always-flag', '-myflag']" + hash_with = fc.get_hash(stub_configuration, Path('/somewhere/myfile.F90')) + assert hash_with == crc32(compiler_info.encode()) + # Just to be certain they are indeed different + assert hash_with != hash_without + + +def test_compiler_hash_compiler_error(stub_configuration): '''Test the hash functionality when version info is missing.''' cc = Gcc() # raise an error when trying to get compiler version with mock.patch.object(cc, 'run', side_effect=RuntimeError()): with raises(RuntimeError) as err: - cc.get_hash() + cc.get_hash(stub_configuration, Path('.')) assert "Error asking for version of compiler" in str(err.value) -def test_compiler_hash_invalid_version(): +def test_compiler_hash_invalid_version(stub_configuration): '''Test the hash functionality when version info is missing.''' cc = Gcc() # returns an invalid compiler version string with mock.patch.object(cc, "run", mock.Mock(return_value='foo v1')): with raises(RuntimeError) as err: - cc.get_hash() + cc.get_hash(stub_configuration, Path('.')) assert ("Unexpected version output format for compiler 'gcc'" in str(err.value)) diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index d48916215..804dc879c 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -11,8 +11,6 @@ from pytest import raises, warns from pytest_subprocess.fake_process import FakeProcess -from tests.conftest import ExtendedRecorder, call_list, not_found_callback - from fab.build_config import BuildConfig from fab.tools.category import Category from fab.tools.compiler import CCompiler, FortranCompiler @@ -20,6 +18,8 @@ CrayCcWrapper, CrayFtnWrapper, Mpicc, Mpif90) +from tests.conftest import ExtendedRecorder, call_list, not_found_callback + def test_compiler_getter(stub_c_compiler: CCompiler) -> None: """ @@ -85,7 +85,8 @@ def test_compiler_is_available_no_version(stub_c_compiler: CCompiler, assert not mpicc.is_available -def test_compiler_hash(fake_process: FakeProcess) -> None: +def test_compiler_hash(stub_configuration, + fake_process: FakeProcess) -> None: """ Test the hash functionality. """ @@ -94,8 +95,8 @@ def test_compiler_hash(fake_process: FakeProcess) -> None: cc1 = CCompiler('test C compiler', 'tcc', 'test', version_regex=r'([\d.]+)') mpicc1 = Mpicc(cc1) - hash1 = mpicc1.get_hash() - assert hash1 == 5953380633 + hash1 = mpicc1.get_hash(stub_configuration, Path('.')) + assert hash1 == 2609406574 # A change in the version number must change the hash: fake_process.register(['tcc', '--version'], stdout='8.9') @@ -103,7 +104,7 @@ def test_compiler_hash(fake_process: FakeProcess) -> None: cc2 = CCompiler('test C compiler', 'tcc', 'test', version_regex=r'([\d.]+)') mpicc2 = Mpicc(cc2) - hash2 = mpicc2.get_hash() + hash2 = mpicc2.get_hash(stub_configuration, Path('.')) assert hash2 != hash1 # A change in the name with the original version number @@ -113,7 +114,7 @@ def test_compiler_hash(fake_process: FakeProcess) -> None: cc3 = CCompiler('New test C compiler', 'tcc', 'test', version_regex=r'([\d.]+)') mpicc3 = Mpicc(cc3) - hash3 = mpicc3.get_hash() + hash3 = mpicc3.get_hash(stub_configuration, Path('.')) assert hash3 not in (hash1, hash2) # A change in the name with the modified version number @@ -123,7 +124,7 @@ def test_compiler_hash(fake_process: FakeProcess) -> None: cc4 = CCompiler('New test C compiler', 'tcc', 'test', version_regex=r'([\d.]+)') mpicc4 = Mpicc(cc4) - hash4 = mpicc4.get_hash() + hash4 = mpicc4.get_hash(stub_configuration, Path('.')) assert hash4 not in (hash1, hash2, hash3) From 4535905f48a34b30a5aa9e24837cdb19f28bf2cd Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 9 Oct 2025 18:10:41 +1100 Subject: [PATCH 07/30] #313 Make flake8 happy. --- source/fab/tools/flags.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index d140d7311..a8a2f26af 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -128,7 +128,10 @@ def __eq__(self, other: object) -> bool: """ if not isinstance(other, AlwaysFlags): return NotImplemented - return (type(self) == type(other) and + # Flake insists to use isinstance. But in this case + # we explicitly do not want to allow subclasses, e.g. + # AlwaysFlag should never be equal to a MatchFlag + return (type(self) == type(other) and # noqa self._flags == other._flags) @staticmethod From f1090bd95d8433dc5287ac5fc02c899c52c549ed Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 9 Oct 2025 19:25:57 +1100 Subject: [PATCH 08/30] #313 Updated testing to cover all of flags. --- source/fab/tools/flags.py | 6 +++++- tests/unit_tests/tools/test_flags.py | 26 ++++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index a8a2f26af..f568b3a67 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -127,10 +127,13 @@ def __eq__(self, other: object) -> bool: :returns: if two AlwaysFlags are identical """ if not isinstance(other, AlwaysFlags): - return NotImplemented + raise NotImplementedError( + f"Cannot compare '{type(self).__name__}' with object " + f"of type '{type(other).__name__}'.") # Flake insists to use isinstance. But in this case # we explicitly do not want to allow subclasses, e.g. # AlwaysFlag should never be equal to a MatchFlag + # pylint: disable=unidiomatic-typecheck return (type(self) == type(other) and # noqa self._flags == other._flags) @@ -320,6 +323,7 @@ def __init__( elif list_of_flags: self.append(list_of_flags) if add_flags: + # pylint: disable=import-outside-toplevel # TODO: circular import otherwise from fab.build_config import AddFlags if isinstance(add_flags, AddFlags): diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index ceab1f91a..72e80c698 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -49,6 +49,13 @@ def test_always_flags(stub_configuration): str(stub_configuration.build_output), "/my"]) + # Test comparison of different objects + with pytest.raises(NotImplementedError) as err: + # pylint: disable=pointless-statement + af == 1 + assert ("Cannot compare 'AlwaysFlags' with object of type 'int'." + in str(err.value)) + def test_always_flags_remove_flags(): '''Test remove_flags functionality.''' @@ -166,8 +173,7 @@ def test_remove_flags(): def test_flags_checksum(): '''Tests computation of the checksum.''' list_of_flags = ['one', 'two', 'three', 'four'] - flags = ProfileFlags() - flags.add_flags(list_of_flags) + flags = FlagList(list_of_flags) assert flags.checksum() == string_checksum(str(list_of_flags)) @@ -294,6 +300,14 @@ def test_profile_flags_checksum(stub_configuration): assert (pf.checksum(stub_configuration, Path()) == string_checksum(str(list_of_flags_new))) + # Test handling when no config is provided: + assert (pf.checksum(file_path=Path()) == + string_checksum(str(list_of_flags_new))) + + # Test handling when no file_path is provided: + assert (pf.checksum(stub_configuration) == + string_checksum(str(list_of_flags_new))) + def test_profile_flags_errors_invalid_profile_name(stub_configuration): '''Tests that given undefined profile names will raise @@ -332,3 +346,11 @@ def test_old_addflags(): assert isinstance(match_flag, MatchFlags) assert match_flag._pattern == "/some/pattern" assert match_flag._flags == ["-g", "-O0"] + + # Provide a single AddFlags instead of a list: + flag_list = FlagList(["-x"], + add_flags=AddFlags("pattern", ["-y"])) + match_flag = flag_list[1] + assert isinstance(match_flag, MatchFlags) + assert match_flag._pattern == "pattern" + assert match_flag._flags == ["-y"] From d2f156e0ca5e533b69de860e6a54904f0b6ae72f Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 9 Oct 2025 19:34:48 +1100 Subject: [PATCH 09/30] #313 Made pattern the first argument of the constructor. --- source/fab/tools/flags.py | 12 +++++++----- tests/unit_tests/tools/test_compiler.py | 3 ++- tests/unit_tests/tools/test_flags.py | 10 +++++----- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index f568b3a67..fb20c5cd7 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -236,7 +236,9 @@ class MatchFlags(AlwaysFlags): :param flags: a string or list of strings with command line flags. :param pattern: the wildcard pattern which is used when matching. """ - def __init__(self, flags: Union[str, List[str]], pattern: str) -> None: + def __init__(self, + pattern: str, + flags: Union[str, List[str]]) -> None: super().__init__(flags) self._pattern = pattern @@ -272,8 +274,8 @@ class ContainFlags(AlwaysFlags): """ def __init__(self, - flags: Union[str, List[str]], - pattern: str) -> None: + pattern: str, + flags: Union[str, List[str]]) -> None: super().__init__(flags) self._pattern = pattern @@ -330,8 +332,8 @@ def __init__( add_flags = [add_flags] # Convert old-style AddFlags to the new MatchFlags: for add_flag in add_flags: - self.add_flags(MatchFlags(add_flag.flags, - add_flag.match)) + self.add_flags(MatchFlags(add_flag.match, + add_flag.flags)) def get_flags(self, config: Optional["BuildConfig"] = None, diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index 3ae2678d7..52aef644a 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -138,7 +138,8 @@ def test_compiler_path_specific_flags(stub_configuration, fc._version = (1, 2) print(fc.name, fc.get_version()) - contain_flag = ContainFlags(["-myflag"], pattern="myfile") + contain_flag = ContainFlags(pattern="myfile", + flags=["-myflag"]) fc.add_flags("-always-flag") fc.add_flags(contain_flag) diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index 72e80c698..5fa0ccb1a 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -30,7 +30,7 @@ def test_always_flags(stub_configuration): assert af.get_flags() == ["-g", "-O2"] # Comparison: First different derived type: - cf_copy = ContainFlags(["-g", "-O2"], pattern="XX") + cf_copy = ContainFlags("XX", ["-g", "-O2"]) assert af != cf_copy af_copy = AlwaysFlags(["-g", "-O2"]) assert af_copy == af @@ -92,9 +92,9 @@ def test_match_flags() -> None: """ Tests matching using wildcards. """ - mf = MatchFlags("-g", pattern="/*") + mf = MatchFlags("/*", "-g") assert mf.get_flags(file_path=Path(".")) == [] - mf = MatchFlags(["-g", "$relative"], pattern="/*") + mf = MatchFlags("/*", ["-g", "$relative"]) file_path = Path("/my/dir") assert mf.get_flags(file_path=file_path) == ["-g", "/my"] @@ -103,9 +103,9 @@ def test_contain_flags() -> None: """ Tests matching using substrings. """ - cf = ContainFlags("-g", pattern="yes") + cf = ContainFlags(pattern="yes", flags="-g") assert cf.get_flags(file_path=Path(".")) == [] - cf = ContainFlags(["-g", "$relative"], pattern="/") + cf = ContainFlags("/", ["-g", "$relative"]) file_path = Path("/my/dir") assert cf.get_flags(file_path=file_path) == ["-g", "/my"] From 80d1905e8a8928dbd06bff887d29aba617ebe940 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Fri, 10 Oct 2025 13:06:37 +1100 Subject: [PATCH 10/30] #502 Support ignoring DEPDENDS-ON dependencies, and avoid crash. --- source/fab/mo.py | 21 ++++++++++++++++----- source/fab/steps/analyse.py | 2 +- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/source/fab/mo.py b/source/fab/mo.py index a2208d02f..1fd47a36f 100644 --- a/source/fab/mo.py +++ b/source/fab/mo.py @@ -4,22 +4,26 @@ # which you should have received as part of this distribution # ############################################################################## """ -A temporary place for some Met Office specific logic which, for now, needs to be integrated into Fab's internals. +A temporary place for some Met Office specific logic which, for now, needs to +be integrated into Fab's internals. """ from pathlib import Path -from typing import Dict, List +from typing import Dict, Iterable, List, Optional from fab.dep_tree import AnalysedDependent, filter_source_tree, logger from fab.parse.c import AnalysedC from fab.parse.fortran import AnalysedFortran -def add_mo_commented_file_deps(source_tree: Dict[Path, AnalysedDependent]): +def add_mo_commented_file_deps( + source_tree: Dict[Path, AnalysedDependent], + ignore_dependencies: Optional[Iterable[str]] = None): """ - Handle dependencies from Met Office "DEPENDS ON:" code comments which refer to a c file. - These are the comments which refer to a .o file and not those which just refer to symbols. + Handle dependencies from Met Office "DEPENDS ON:" code comments which + refer to a c file. These are the comments which refer to a .o file and + not those which just refer to symbols. :param source_tree: The source tree of analysed files. @@ -34,5 +38,12 @@ def add_mo_commented_file_deps(source_tree: Dict[Path, AnalysedDependent]): for f in analysed_fortran: num_found += len(f.mo_commented_file_deps) for dep in f.mo_commented_file_deps: + if dep in ignore_set: + continue + if dep not in lookup: + logger.error(f"DEPENDS ON dependency '{dep}' not found for " + f"file '{f.fpath}' - ignored for now, but " + f"the build might fail because of this.") + continue f.file_deps.add(lookup[dep].fpath) logger.info(f"processed {num_found} DEPENDS ON file dependencies") diff --git a/source/fab/steps/analyse.py b/source/fab/steps/analyse.py index 49833fa58..8db67b5ce 100644 --- a/source/fab/steps/analyse.py +++ b/source/fab/steps/analyse.py @@ -172,7 +172,7 @@ def analyse( # add the file dependencies for MO FCM's "DEPENDS ON:" commented file deps (being removed soon) with TimerLogger("adding MO FCM 'DEPENDS ON:' file dependency comments"): - add_mo_commented_file_deps(project_source_tree) + add_mo_commented_file_deps(project_source_tree, ignore_dependencies) logger.info(f"source tree size {len(project_source_tree)}") From bd612f183076effaecaf0b10fc150beac33180e5 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Fri, 10 Oct 2025 13:30:05 +1100 Subject: [PATCH 11/30] #502 Fixed #todo. --- source/fab/mo.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/source/fab/mo.py b/source/fab/mo.py index 1fd47a36f..76646feb2 100644 --- a/source/fab/mo.py +++ b/source/fab/mo.py @@ -10,16 +10,16 @@ """ from pathlib import Path -from typing import Dict, Iterable, List, Optional +from typing import Dict, Iterable, Optional -from fab.dep_tree import AnalysedDependent, filter_source_tree, logger +from fab.dep_tree import AnalysedDependent, logger from fab.parse.c import AnalysedC from fab.parse.fortran import AnalysedFortran def add_mo_commented_file_deps( source_tree: Dict[Path, AnalysedDependent], - ignore_dependencies: Optional[Iterable[str]] = None): + ignore_dependencies: Optional[Iterable[str]] = None) -> None: """ Handle dependencies from Met Office "DEPENDS ON:" code comments which refer to a c file. These are the comments which refer to a .o file and @@ -29,9 +29,11 @@ def add_mo_commented_file_deps( The source tree of analysed files. """ - # todo: this would be better if filtered by type, i,e, AnalysedFortran & AnalysedC - analysed_fortran: List[AnalysedFortran] = filter_source_tree(source_tree, '.f90') # type: ignore - analysed_c: List[AnalysedC] = filter_source_tree(source_tree, '.c') # type: ignore + ignore_set = set(ignore_dependencies) if ignore_dependencies else set() + + analysed_fortran = [i for i in source_tree.values() + if isinstance(i, AnalysedFortran)] + analysed_c = [i for i in source_tree.values() if isinstance(i, AnalysedC)] lookup = {c.fpath.name: c for c in analysed_c} num_found = 0 From 38a049ce76e96fddcb54f8b96b0706582ec1567c Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Fri, 10 Oct 2025 14:59:12 +1100 Subject: [PATCH 12/30] #502 Added standalone unit test for mo. --- tests/unit_tests/test_mo.py | 67 +++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 tests/unit_tests/test_mo.py diff --git a/tests/unit_tests/test_mo.py b/tests/unit_tests/test_mo.py new file mode 100644 index 000000000..1cf20579a --- /dev/null +++ b/tests/unit_tests/test_mo.py @@ -0,0 +1,67 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## + +""" +Test the mo.py file, which handles 'DEPENDS ON' dependency comments. +""" + +import logging +from pathlib import Path + +from fab.mo import add_mo_commented_file_deps +from fab.parse.c import AnalysedC +from fab.parse.fortran import AnalysedFortran + + +def test_mo(): + """Test that a commented dependency is added. + """ + src_tree = { + Path('foo.f90'): AnalysedFortran(fpath=Path('foo.f90'), + file_hash=0, + mo_commented_file_deps=["root.c"]), + Path('root.c'): AnalysedC(fpath=Path('root.c'), + file_hash=0), + } + an_for = src_tree[Path('foo.f90')] + assert an_for.file_deps == set() + add_mo_commented_file_deps(src_tree) + assert an_for.file_deps == set([Path('root.c')]) + + +def test_mo_missing(): + """Test handling of a missing dependency + """ + src_tree = { + Path('foo.f90'): AnalysedFortran(fpath=Path('foo.f90'), + file_hash=0, + mo_commented_file_deps=["root.c"]), + } + an_for = src_tree[Path('foo.f90')] + assert an_for.file_deps == set() + add_mo_commented_file_deps(src_tree, + ignore_dependencies=["root.c"]) + assert an_for.file_deps == set() + + +def test_mo_missing_warning(caplog): + """Test handling of a missing dependency if no ignore is defined + for it. + """ + src_tree = { + Path('foo.f90'): AnalysedFortran(fpath=Path('foo.f90'), + file_hash=0, + mo_commented_file_deps=["root.c"]), + } + an_for = src_tree[Path('foo.f90')] + assert an_for.file_deps == set() + with caplog.at_level(logging.ERROR): + add_mo_commented_file_deps(src_tree) + # There should be one error logged: + assert len(caplog.records) == 1 + assert caplog.records[0].levelname == "ERROR" + + assert an_for.file_deps == set() From 0c92c5cf8714f127e86aa919b686a2e024771965 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Sat, 11 Oct 2025 11:31:20 +1100 Subject: [PATCH 13/30] #502 For DEPENDS ON comments store the original .o name, so it can be used for ignore lists when converting DEPENDS dependencies to file dependencies. --- source/fab/mo.py | 9 ++++++++ source/fab/parse/fortran.py | 5 ++-- .../parse/fortran/test_fortran_analyser.py | 2 +- tests/unit_tests/test_mo.py | 23 ++++++++++++------- 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/source/fab/mo.py b/source/fab/mo.py index 76646feb2..eee195f95 100644 --- a/source/fab/mo.py +++ b/source/fab/mo.py @@ -42,6 +42,15 @@ def add_mo_commented_file_deps( for dep in f.mo_commented_file_deps: if dep in ignore_set: continue + # If the DEPENDS ON specified a .o file, rename it + # to the expected c file. + dep = dep.replace(".o", ".c") + + # Just in case, also allow that a .c file is specified in the + # ignore list: + if dep in ignore_set: + continue + if dep not in lookup: logger.error(f"DEPENDS ON dependency '{dep}' not found for " f"file '{f.fpath}' - ignored for now, but " diff --git a/source/fab/parse/fortran.py b/source/fab/parse/fortran.py index e7d38c1bf..0eff4c62c 100644 --- a/source/fab/parse/fortran.py +++ b/source/fab/parse/fortran.py @@ -378,10 +378,9 @@ def _process_comment(self, analysed_file, obj): logger.debug(f"ignoring use of {dep}") # with .o means a c file elif dep.endswith(".o"): - analysed_file.mo_commented_file_deps.add( - dep.replace(".o", ".c")) - # without .o means a fortran symbol + analysed_file.mo_commented_file_deps.add(dep) else: + # Without .o means a Fortran symbol analysed_file.add_symbol_dep(dep) def _process_subroutine_or_function(self, analysed_file, fpath, obj): diff --git a/tests/unit_tests/parse/fortran/test_fortran_analyser.py b/tests/unit_tests/parse/fortran/test_fortran_analyser.py index 1902ca7b5..b3f8e81db 100644 --- a/tests/unit_tests/parse/fortran/test_fortran_analyser.py +++ b/tests/unit_tests/parse/fortran/test_fortran_analyser.py @@ -45,7 +45,7 @@ def module_expected(module_fpath: Path) -> AnalysedFortran: module_deps={'bar_mod', 'compute_chunk_size_mod'}, symbol_deps={'monty_func', 'bar_mod', 'compute_chunk_size_mod'}, file_deps=set(), - mo_commented_file_deps={'some_file.c'}, + mo_commented_file_deps={'some_file.o'}, ) diff --git a/tests/unit_tests/test_mo.py b/tests/unit_tests/test_mo.py index 1cf20579a..50f177045 100644 --- a/tests/unit_tests/test_mo.py +++ b/tests/unit_tests/test_mo.py @@ -17,31 +17,38 @@ def test_mo(): - """Test that a commented dependency is added. + """Test that a commented dependency is added. This especially + tests that a dependency that is specified as '.o', is added + as a dependency as the corresponding '.c' file. """ src_tree = { Path('foo.f90'): AnalysedFortran(fpath=Path('foo.f90'), file_hash=0, - mo_commented_file_deps=["root.c"]), - Path('root.c'): AnalysedC(fpath=Path('root.c'), + mo_commented_file_deps=["root.o"]), + Path('root.c'): AnalysedC(fpath=Path('/some/path/root.c'), file_hash=0), } an_for = src_tree[Path('foo.f90')] assert an_for.file_deps == set() add_mo_commented_file_deps(src_tree) - assert an_for.file_deps == set([Path('root.c')]) + assert an_for.file_deps == set([Path('/some/path/root.c')]) -def test_mo_missing(): - """Test handling of a missing dependency +def test_mo_missing_ignored(): + """Test handling of a missing dependency if it is supposed to be ignored. """ src_tree = { Path('foo.f90'): AnalysedFortran(fpath=Path('foo.f90'), file_hash=0, - mo_commented_file_deps=["root.c"]), + mo_commented_file_deps=["root.o"]), } an_for = src_tree[Path('foo.f90')] assert an_for.file_deps == set() + add_mo_commented_file_deps(src_tree, + ignore_dependencies=["root.o"]) + assert an_for.file_deps == set() + + # Now also check that the ignore list can use the .c name: add_mo_commented_file_deps(src_tree, ignore_dependencies=["root.c"]) assert an_for.file_deps == set() @@ -54,7 +61,7 @@ def test_mo_missing_warning(caplog): src_tree = { Path('foo.f90'): AnalysedFortran(fpath=Path('foo.f90'), file_hash=0, - mo_commented_file_deps=["root.c"]), + mo_commented_file_deps=["root.o"]), } an_for = src_tree[Path('foo.f90')] assert an_for.file_deps == set() From 41c5956da9358613b01c1f0ae2114047d8120740 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Mon, 13 Oct 2025 10:48:01 +1100 Subject: [PATCH 14/30] #502 Removed incorrect parameter documentation. --- source/fab/steps/analyse.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/fab/steps/analyse.py b/source/fab/steps/analyse.py index 8db67b5ce..a87d6e6c8 100644 --- a/source/fab/steps/analyse.py +++ b/source/fab/steps/analyse.py @@ -112,8 +112,6 @@ def analyse( :param ignore_dependencies: Third party Fortran module names in USE statements, 'DEPENDS ON' files and modules to be ignored. - :param name: - Human friendly name for logger output, with sensible default. """ From 46d1a8c9d4a34b090b94e1ddc7e3050da89a3f95 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Mon, 13 Oct 2025 13:11:33 +1100 Subject: [PATCH 15/30] #502 Add support to ignore dependencies to PSyclone step. --- source/fab/steps/psyclone.py | 37 ++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/source/fab/steps/psyclone.py b/source/fab/steps/psyclone.py index 33ec0208b..6be959386 100644 --- a/source/fab/steps/psyclone.py +++ b/source/fab/steps/psyclone.py @@ -15,7 +15,7 @@ import warnings from itertools import chain from pathlib import Path -from typing import Callable, Dict, List, Optional, Set, Tuple, Union +from typing import Callable, Dict, Iterable, List, Optional, Set, Tuple, Union from fab.build_config import BuildConfig @@ -80,14 +80,17 @@ class MpCommonArgs: @step -def psyclone(config, kernel_roots: Optional[List[Path]] = None, +def psyclone(config: BuildConfig, + kernel_roots: Optional[List[Path]] = None, transformation_script: Optional[Callable[[Path, BuildConfig], Path]] = None, cli_args: Optional[List[str]] = None, source_getter: Optional[ArtefactsGetter] = None, overrides_folder: Optional[Path] = None, - api: Optional[str] = None): + api: Optional[str] = None, + ignore_dependencies: Optional[Iterable[str]] = None, + ): """ - Psyclone runner step. + PSyclone runner step. .. note:: @@ -115,6 +118,9 @@ def psyclone(config, kernel_roots: Optional[List[Path]] = None, Optional folder containing hand-crafted override files. Must be part of the subsequently analysed source code. Any file produced by psyclone will be deleted if there is a corresponding file in this folder. + :param ignore_dependencies: + Third party Fortran module names in USE statements, 'DEPENDS ON' files + and modules to be ignored. """ kernel_roots = kernel_roots or [] @@ -128,7 +134,8 @@ def psyclone(config, kernel_roots: Optional[List[Path]] = None, analysed_x90 = _analyse_x90s(config, x90s) # analyse the kernel files, - all_kernel_hashes = _analyse_kernels(config, kernel_roots) + all_kernel_hashes = _analyse_kernels(config, kernel_roots, + ignore_dependencies=ignore_dependencies) # get the data in a payload object for child processes to calculate prebuild hashes mp_payload = _generate_mp_payload( @@ -183,14 +190,19 @@ def _generate_mp_payload(config, analysed_x90, all_kernel_hashes, overrides_fold ) -def _analyse_x90s(config, x90s: Set[Path]) -> Dict[Path, AnalysedX90]: - # Analyse parsable versions of the x90s, finding kernel dependencies. +def _analyse_x90s(config: BuildConfig, + x90s: Set[Path]) -> Dict[Path, AnalysedX90]: + """ + Analyse parsable versions of the x90s, finding kernel dependencies. + """ # make parsable - todo: fast enough not to require prebuilds? with TimerLogger(f"converting {len(x90s)} x90s into parsable fortran"): parsable_x90s = run_mp(config, items=x90s, func=make_parsable_x90) - # parse + # Parse. Note that there is no need to ignore dependencies: the x90 + # files will be converted to algorithm layer f90 files, and then + # properly analysed later. x90_analyser = X90Analyser(config=config) with TimerLogger(f"analysing {len(parsable_x90s)} parsable x90 files"): x90_results = run_mp(config, items=parsable_x90s, func=x90_analyser.run) @@ -213,7 +225,10 @@ def _analyse_x90s(config, x90s: Set[Path]) -> Dict[Path, AnalysedX90]: return analysed_x90 -def _analyse_kernels(config, kernel_roots) -> Dict[str, int]: +def _analyse_kernels( + config: BuildConfig, + kernel_roots: List[Path], + ignore_dependencies: Optional[Iterable[str]]) -> Dict[str, int]: """ We want to hash the kernel metadata (type defs). @@ -247,7 +262,9 @@ def _analyse_kernels(config, kernel_roots) -> Dict[str, int]: # We use the normal Fortran analyser, which records psyclone kernel metadata. # todo: We'd like to separate that from the general fortran analyser at some point, to reduce coupling. # The Analyse step also uses the same fortran analyser. It stores its results so they won't be analysed twice. - fortran_analyser = FortranAnalyser(config=config) + fortran_analyser = FortranAnalyser(config=config, + ignore_dependencies=ignore_dependencies) + with TimerLogger(f"analysing {len(kernel_files)} potential psyclone kernel files"): fortran_results = run_mp(config, items=kernel_files, func=fortran_analyser.run) log_or_dot_finish(logger) From 93b29f07b15fbbeec209f259060796155b97cc00 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Mon, 13 Oct 2025 15:33:59 +1100 Subject: [PATCH 16/30] #502 Fixed incorrect declaration. --- source/fab/steps/psyclone.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/fab/steps/psyclone.py b/source/fab/steps/psyclone.py index 6be959386..f00372039 100644 --- a/source/fab/steps/psyclone.py +++ b/source/fab/steps/psyclone.py @@ -228,7 +228,7 @@ def _analyse_x90s(config: BuildConfig, def _analyse_kernels( config: BuildConfig, kernel_roots: List[Path], - ignore_dependencies: Optional[Iterable[str]]) -> Dict[str, int]: + ignore_dependencies: Optional[Iterable[str]] = None) -> Dict[str, int]: """ We want to hash the kernel metadata (type defs). From 19d81946639b304d382e728c8434682baa874286 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Tue, 14 Oct 2025 01:01:57 +1100 Subject: [PATCH 17/30] #502 Refactored tool so that Tool has no flags anymore. --- .../site_specific/default/setup_gnu.py | 7 +- source/fab/tools/__init__.py | 3 +- source/fab/tools/compiler.py | 5 +- source/fab/tools/compiler_suite_tool.py | 50 +++++++++ source/fab/tools/compiler_wrapper.py | 3 +- source/fab/tools/linker.py | 6 +- source/fab/tools/preprocessor.py | 20 +++- source/fab/tools/tool.py | 59 +---------- source/fab/tools/tool_with_flags.py | 79 ++++++++++++++ tests/unit_tests/steps/test_preprocess.py | 100 +++++++++--------- tests/unit_tests/tools/test_ar.py | 4 +- .../tools/test_compiler_suite_tool.py | 29 +++++ tests/unit_tests/tools/test_linker.py | 40 +++---- tests/unit_tests/tools/test_preprocessor.py | 11 +- tests/unit_tests/tools/test_psyclone.py | 4 +- tests/unit_tests/tools/test_rsync.py | 1 - tests/unit_tests/tools/test_tool.py | 42 +------- .../unit_tests/tools/test_tool_with_flags.py | 63 +++++++++++ tests/unit_tests/tools/test_versioning.py | 4 - 19 files changed, 333 insertions(+), 197 deletions(-) create mode 100644 source/fab/tools/compiler_suite_tool.py create mode 100644 source/fab/tools/tool_with_flags.py create mode 100644 tests/unit_tests/tools/test_compiler_suite_tool.py create mode 100644 tests/unit_tests/tools/test_tool_with_flags.py diff --git a/source/fab/fab_base/site_specific/default/setup_gnu.py b/source/fab/fab_base/site_specific/default/setup_gnu.py index 1917edadd..f22110ad2 100644 --- a/source/fab/fab_base/site_specific/default/setup_gnu.py +++ b/source/fab/fab_base/site_specific/default/setup_gnu.py @@ -11,7 +11,10 @@ from typing import cast, Dict, List from fab.build_config import AddFlags, BuildConfig -from fab.tools import Category, Linker, ToolRepository +from fab.tools.category import Category +from fab.tools.linker import Linker +from fab.tools.tool_repository import ToolRepository +from fab.tools.compiler import Compiler def setup_gnu(build_config: BuildConfig, @@ -33,6 +36,8 @@ def setup_gnu(build_config: BuildConfig, if not gfortran.is_available: return {} + gfortran = cast(Compiler, gfortran) + # The base flags # ============== gfortran.add_flags(['-ffree-line-length-none', '-Wall', '-g'], diff --git a/source/fab/tools/__init__.py b/source/fab/tools/__init__.py index 93e19df18..a957ac0c9 100644 --- a/source/fab/tools/__init__.py +++ b/source/fab/tools/__init__.py @@ -20,7 +20,7 @@ from fab.tools.rsync import Rsync from fab.tools.preprocessor import Cpp, CppFortran, Fpp, Preprocessor from fab.tools.shell import Shell -from fab.tools.tool import Tool, CompilerSuiteTool +from fab.tools.tool import Tool # Order here is important to avoid a circular import from fab.tools.tool_repository import ToolRepository from fab.tools.tool_box import ToolBox @@ -30,7 +30,6 @@ "Category", "CCompiler", "Compiler", - "CompilerSuiteTool", "CompilerWrapper", "Cpp", "CppFortran", diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 43c7dcafa..0d9acc6c5 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -15,7 +15,7 @@ from fab.tools.category import Category from fab.tools.flags import AlwaysFlags -from fab.tools.tool import CompilerSuiteTool +from fab.tools.compiler_suite_tool import CompilerSuiteTool from fab.util import string_checksum if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -163,7 +163,8 @@ def get_all_commandline_options( params.extend([input_file.name, self._output_flag, str(output_file)]) return params - def get_flags(self, config: Optional["BuildConfig"] = None) -> List[str]: + def get_flags(self, config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: '''Since compiler need path-specific information (and the `run` method in tool does not provide the path), a compiler will return an empty list as flags. All compiler flags diff --git a/source/fab/tools/compiler_suite_tool.py b/source/fab/tools/compiler_suite_tool.py new file mode 100644 index 000000000..87a735ed2 --- /dev/null +++ b/source/fab/tools/compiler_suite_tool.py @@ -0,0 +1,50 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## + +"""This file contains the base class for all tools, i.e. compiler, +preprocessor, linker, archiver, Psyclone, rsync, versioning tools. + +Each tool belongs to one category (e.g. FORTRAN_COMPILER). This category +is used when adding a tool to a ToolRepository or ToolBox. +It provides basic support for running a binary, and keeping track if +a tool is actually available. +""" + +from pathlib import Path +from typing import List, Optional, Union + +from fab.tools.category import Category +from fab.tools.tool_with_flags import ToolWithFlags + + +class CompilerSuiteTool(ToolWithFlags): + '''A tool that is part of a compiler suite (typically compiler + and linker). + + :param name: name of the tool. + :param exec_name: name of the executable to start. + :param suite: name of the compiler suite. + :param category: the Category to which this tool belongs. + :param availability_option: a command line option for the tool to test + if the tool is available on the current system. Defaults to + `--version`. + ''' + def __init__( + self, + name: str, + exec_name: Union[str, Path], + suite: str, + category: Category, + availability_option: Optional[Union[str, + List[str]]] = None) -> None: + super().__init__(name, exec_name, category, + availability_option=availability_option) + self._suite = suite + + @property + def suite(self) -> str: + ''':returns: the compiler suite of this tool.''' + return self._suite diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index cd46dda23..4b9885daf 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -71,7 +71,8 @@ def has_syntax_only(self) -> bool: raise RuntimeError(f"Compiler '{self._compiler.name}' has " f"no has_syntax_only.") - def get_flags(self, config: Optional["BuildConfig"] = None) -> List[str]: + def get_flags(self, config: Optional["BuildConfig"] = None, + file_path: Optional[Path] = None) -> List[str]: ''':returns: the ProfileFlags for the given profile, combined from the wrapped compiler and this wrapper. diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 1c257a40c..e46bc0e6d 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -10,13 +10,13 @@ from __future__ import annotations from pathlib import Path -from typing import Dict, List, Optional, TYPE_CHECKING, Union +from typing import Dict, List, Optional, TYPE_CHECKING import warnings from fab.tools.category import Category from fab.tools.compiler import Compiler from fab.tools.flags import ProfileFlags -from fab.tools.tool import CompilerSuiteTool +from fab.tools.compiler_suite_tool import CompilerSuiteTool if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -239,7 +239,7 @@ def link(self, input_files: List[Path], output_file: Path, :returns: the stdout of the link command ''' - params: List[Union[str, Path]] = [] + params: List[str] = self.get_flags(config) params.extend(self._compiler.get_resolved_flags(config, Path())) diff --git a/source/fab/tools/preprocessor.py b/source/fab/tools/preprocessor.py index c5b934455..2e75d3f18 100644 --- a/source/fab/tools/preprocessor.py +++ b/source/fab/tools/preprocessor.py @@ -13,10 +13,10 @@ from typing import List, Optional, Sequence, Union from fab.tools.category import Category -from fab.tools.tool import Tool +from fab.tools.tool_with_flags import ToolWithFlags -class Preprocessor(Tool): +class Preprocessor(ToolWithFlags): '''This is the base class for any preprocessor. :param name: the name of the preprocessor. @@ -66,6 +66,22 @@ def __init__(self): super().__init__("cpp", "cpp", Category.FORTRAN_PREPROCESSOR) self.add_flags(["-traditional-cpp", "-P"]) + def preprocess(self, input_file: Path, output_file: Path, + add_flags: Optional[Sequence[Union[Path, str]]] = None): + '''Calls the preprocessor to process the specified input file, + creating the requested output file. + + :param input_file: input file. + :param output_file: the output filename. + :param add_flags: List with additional flags to be used. + ''' + params: List[Union[str, Path]] = ["-traditional-cpp", "-P"] + + if add_flags: + params.extend(add_flags) + + super().preprocess(input_file, output_file, params) + # ============================================================================ class Fpp(Preprocessor): diff --git a/source/fab/tools/tool.py b/source/fab/tools/tool.py index d26738c78..9e332f0c2 100644 --- a/source/fab/tools/tool.py +++ b/source/fab/tools/tool.py @@ -19,7 +19,6 @@ from typing import Dict, List, Optional, Sequence, TYPE_CHECKING, Union from fab.tools.category import Category -from fab.tools.flags import AbstractFlags, ProfileFlags if TYPE_CHECKING: from fab.build_config import BuildConfig @@ -43,7 +42,6 @@ def __init__(self, name: str, exec_name: Union[str, Path], self._logger = logging.getLogger(__name__) self._name = name self._exec_path = Path(exec_name) - self._flags = ProfileFlags() self._category = category if availability_option: self._availability_option = availability_option @@ -124,36 +122,6 @@ def category(self) -> Category: ''':returns: the category of this tool.''' return self._category - @property - def flags(self) -> ProfileFlags: - ''':returns: the profile flags for this tool.''' - return self._flags - - def get_flags(self, config: Optional["BuildConfig"] = None) -> List[str]: - ''':returns: the flags to be used with this tool.''' - return self.flags.get_flags(config) - - def add_flags(self, new_flags: Union[AbstractFlags, str, List[str]], - profile: Optional[str] = None): - '''Adds the specified flags to the list of flags. - - :param new_flags: A single string or list of strings which are the - flags to be added. - ''' - self._flags.add_flags(new_flags, profile) - - def define_profile(self, - name: str, - inherit_from: Optional[str] = None): - '''Defines a new profile name, and allows to specify if this new - profile inherit settings from an existing profile. - - :param name: Name of the profile to define. - :param inherit_from: Optional name of a profile to inherit - settings from. - ''' - self._flags.define_profile(name, inherit_from) - @property def logger(self) -> logging.Logger: ''':returns: a logger object for convenience.''' @@ -190,7 +158,7 @@ def run(self, :raises RuntimeError: if the code is not available. :raises RuntimeError: if the return code of the executable is not 0. """ - command = [str(self.exec_path)] + self.get_flags(config) + command = [str(self.exec_path)] if additional_parameters: if isinstance(additional_parameters, str): command.append(additional_parameters) @@ -223,28 +191,3 @@ def run(self, if capture_output: return res.stdout.decode() return "" - - -class CompilerSuiteTool(Tool): - '''A tool that is part of a compiler suite (typically compiler - and linker). - - :param name: name of the tool. - :param exec_name: name of the executable to start. - :param suite: name of the compiler suite. - :param category: the Category to which this tool belongs. - :param availability_option: a command line option for the tool to test - if the tool is available on the current system. Defaults to - `--version`. - ''' - def __init__(self, name: str, exec_name: Union[str, Path], suite: str, - category: Category, - availability_option: Optional[Union[str, List[str]]] = None): - super().__init__(name, exec_name, category, - availability_option=availability_option) - self._suite = suite - - @property - def suite(self) -> str: - ''':returns: the compiler suite of this tool.''' - return self._suite diff --git a/source/fab/tools/tool_with_flags.py b/source/fab/tools/tool_with_flags.py new file mode 100644 index 000000000..01815ba58 --- /dev/null +++ b/source/fab/tools/tool_with_flags.py @@ -0,0 +1,79 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## + +"""This file contains the base class for a tool with flags. +It is the base class for compiler, linker, and pre-processor. + +""" + +from pathlib import Path +from typing import List, Optional, TYPE_CHECKING, Union + +from fab.tools.category import Category +from fab.tools.flags import AbstractFlags, ProfileFlags +from fab.tools.tool import Tool + +if TYPE_CHECKING: + from fab.build_config import BuildConfig + + +class ToolWithFlags(Tool): + '''This is the base class for all tools that provide flags. + Note that the run method of the Tool base class is not overwritten + to provide the flags to the base class, that needs to be done + by the individual derived tools. + + :param name: name of the tool. + :param exec_name: name or full path of the executable to start. + :param category: the Category to which this tool belongs. + :param availability_option: a command line option for the tool to test + if the tool is available on the current system. Defaults to + `--version`. + ''' + + def __init__( + self, + name: str, + exec_name: Union[str, Path], + category: Category = Category.MISC, + availability_option: Optional[Union[str, + List[str]]] = None) -> None: + + super().__init__(name, exec_name, category, availability_option) + self._flags = ProfileFlags() + + @property + def flags(self) -> ProfileFlags: + ''':returns: the profile flags for this tool.''' + return self._flags + + def get_flags(self, + config: Optional["BuildConfig"], + file_path: Optional[Path] = None) -> List[str]: + ''':returns: the flags to be used with this tool.''' + return self.flags.get_flags(config, file_path) + + def add_flags(self, + new_flags: Union[AbstractFlags, str, List[str]], + profile: Optional[str] = None): + '''Adds the specified flags to the list of flags. + + :param new_flags: A single string or list of strings which are the + flags to be added. + ''' + self._flags.add_flags(new_flags, profile) + + def define_profile(self, + name: str, + inherit_from: Optional[str] = None): + '''Defines a new profile name, and allows to specify if this new + profile inherit settings from an existing profile. + + :param name: Name of the profile to define. + :param inherit_from: Optional name of a profile to inherit + settings from. + ''' + self._flags.define_profile(name, inherit_from) diff --git a/tests/unit_tests/steps/test_preprocess.py b/tests/unit_tests/steps/test_preprocess.py index 077582210..38225572f 100644 --- a/tests/unit_tests/steps/test_preprocess.py +++ b/tests/unit_tests/steps/test_preprocess.py @@ -20,61 +20,57 @@ from fab.tools.tool_repository import ToolRepository -class Test_preprocess_fortran: +def test_big_little(tmp_path: Path, + stub_tool_repository: ToolRepository, + fake_process: FakeProcess) -> None: + """ + Tests big F90 files are preprocessed and little f90 files are copied. + """ + version_command = ['cpp', '--version'] + fake_process.register(version_command, stdout='1.2.3') + process_command = ['cpp', '-traditional-cpp', '-P', + str(tmp_path / 'proj/source/big.F90'), + str(tmp_path / 'proj/build_output/big.f90')] + fake_process.register(process_command) + config = BuildConfig('proj', ToolBox(), fab_workspace=tmp_path, + multiprocessing=False) + config.source_root.mkdir(parents=True) + big_f90 = Path(config.source_root / 'big.F90') + big_f90.write_text("Big F90 file.", encoding="utf-8") + little_f90 = Path(config.source_root / 'little.f90') + little_f90.write_text("Little f90 file.", encoding="utf-8") - def test_big_little(self, tmp_path: Path, - stub_tool_repository: ToolRepository, - fake_process: FakeProcess) -> None: - """ - Tests big F90 files are preprocessed and little f90 files are copied. - """ - version_command = ['cpp', '-traditional-cpp', '-P', '--version'] - fake_process.register(version_command, stdout='1.2.3') - process_command = ['cpp', '-traditional-cpp', '-P', - str(tmp_path / 'proj/source/big.F90'), - str(tmp_path / 'proj/build_output/big.f90')] - fake_process.register(process_command) + def source_getter(_: ArtefactStore) -> list[Path]: + return [big_f90, little_f90] - config = BuildConfig('proj', ToolBox(), fab_workspace=tmp_path, - multiprocessing=False) - config.source_root.mkdir(parents=True) - big_f90 = Path(config.source_root / 'big.F90') - big_f90.write_text("Big F90 file.") - little_f90 = Path(config.source_root / 'little.f90') - little_f90.write_text("Little f90 file.") + with warns(UserWarning, + match="_metric_send_conn not set, cannot send metrics"): + preprocess_fortran(config=config, source=source_getter) + assert (config.build_output / 'little.f90').read_text() \ + == "Little f90 file." + assert fake_process.call_count(process_command) == 1 - def source_getter(artefact_store: ArtefactStore) -> list[Path]: - return [big_f90, little_f90] - with warns(UserWarning, - match="_metric_send_conn not set, cannot send metrics"): - preprocess_fortran(config=config, source=source_getter) +def test_wrong_exe(stub_tool_repository: ToolRepository, + tmp_path: Path, + fake_process: FakeProcess) -> None: + """ + Tests detection of wrong executable. - assert (config.build_output / 'little.f90').read_text() \ - == "Little f90 file." - assert fake_process.call_count(process_command) == 1 + ToDo: Can this ever happen? Don't like messing with "private" state. + """ + fake_process.register(['cpp', '--version']) + tool_box = ToolBox() + # Take the C preprocessor + cpp = tool_box[Category.C_PREPROCESSOR] + # And set its category to FORTRAN_PREPROCESSOR + cpp._category = Category.FORTRAN_PREPROCESSOR + # Now overwrite the Fortran preprocessor with the re-categorised + # C preprocessor: + tool_box.add_tool(cpp, silent_replace=True) - def test_wrong_exe(self, stub_tool_repository: ToolRepository, - tmp_path: Path, - fake_process: FakeProcess) -> None: - """ - Tests detection of wrong executable. - - ToDo: Can this ever happen? Don't like messing with "private" state. - """ - fake_process.register(['cpp', '-traditional-cpp', '-P', '--version']) - fake_process.register(['cpp', '--version']) - tool_box = ToolBox() - # Take the C preprocessor - cpp = tool_box[Category.C_PREPROCESSOR] - # And set its category to FORTRAN_PREPROCESSOR - cpp._category = Category.FORTRAN_PREPROCESSOR - # Now overwrite the Fortran preprocessor with the re-categorised - # C preprocessor: - tool_box.add_tool(cpp, silent_replace=True) - - config = BuildConfig('proj', tool_box, fab_workspace=tmp_path) - with raises(RuntimeError) as err: - preprocess_fortran(config=config) - assert str(err.value) == "Unexpected tool 'cpp' of type '' instead of CppFortran" + config = BuildConfig('proj', tool_box, fab_workspace=tmp_path) + with raises(RuntimeError) as err: + preprocess_fortran(config=config) + assert str(err.value) == "Unexpected tool 'cpp' of type '' instead of CppFortran" diff --git a/tests/unit_tests/tools/test_ar.py b/tests/unit_tests/tools/test_ar.py index 3c3f173f9..da912552d 100644 --- a/tests/unit_tests/tools/test_ar.py +++ b/tests/unit_tests/tools/test_ar.py @@ -10,9 +10,8 @@ from pytest_subprocess.fake_process import FakeProcess -from tests.conftest import ExtendedRecorder, call_list - from fab.tools import Category, Ar +from tests.conftest import ExtendedRecorder, call_list def test_constructor() -> None: @@ -23,7 +22,6 @@ def test_constructor() -> None: assert ar.category == Category.AR assert ar.name == "ar" assert ar.exec_name == "ar" - assert ar.get_flags() == [] def test_check_available(subproc_record: ExtendedRecorder) -> None: diff --git a/tests/unit_tests/tools/test_compiler_suite_tool.py b/tests/unit_tests/tools/test_compiler_suite_tool.py new file mode 100644 index 000000000..617b7ec19 --- /dev/null +++ b/tests/unit_tests/tools/test_compiler_suite_tool.py @@ -0,0 +1,29 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## +""" +Tests tooling base classes. +""" +import logging +from pathlib import Path + +from fab.tools.category import Category +from fab.tools.compiler_suite_tool import CompilerSuiteTool + + +def test_compiler_suite_toolconstructor() -> None: + """ + Tests construction from argument list. + """ + tool = CompilerSuiteTool(name="gfortran", exec_name="gfortran", + suite="gnu", category=Category.FORTRAN_COMPILER) + assert tool.suite == "gnu" + assert str(tool) == "CompilerSuiteTool - gfortran: gfortran" + assert tool.exec_name == "gfortran" + assert tool.exec_path == Path("gfortran") + assert tool.name == "gfortran" + assert tool.category == Category.FORTRAN_COMPILER + assert isinstance(tool.logger, logging.Logger) + assert tool.is_compiler diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index d075d9c11..5bf4fca7e 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -21,7 +21,8 @@ from fab.tools.linker import Linker -def test_c_linker(stub_c_compiler: CCompiler) -> None: +def test_c_linker(stub_c_compiler: CCompiler, + stub_configuration: BuildConfig) -> None: """ Tests construction from C compiler """ @@ -29,18 +30,20 @@ def test_c_linker(stub_c_compiler: CCompiler) -> None: assert linker.category == Category.LINKER assert linker.name == "linker-some C compiler" assert linker.exec_name == "scc" + assert linker.compiler is stub_c_compiler assert linker.suite == "stub" - assert linker.get_flags() == [] + assert linker.get_flags(stub_configuration) == [] assert linker.output_flag == "-o" -def test_fortran_linker(stub_fortran_compiler: FortranCompiler) -> None: +def test_fortran_linker(stub_fortran_compiler: FortranCompiler, + stub_configuration: BuildConfig) -> None: linker = Linker(stub_fortran_compiler) assert linker.category == Category.LINKER assert linker.name == "linker-some Fortran compiler" assert linker.exec_name == "sfc" assert linker.suite == "stub" - assert linker.get_flags() == [] + assert linker.get_flags(stub_configuration) == [] @mark.parametrize("mpi", [True, False]) @@ -182,21 +185,20 @@ def test_linker_add_lib_flags_overwrite_silent(stub_linker: Linker) -> None: assert result == ["-t", "-b"] -class TestLinkerLinking: - def test_c(self, stub_c_compiler: CCompiler, - stub_configuration: BuildConfig, - subproc_record: ExtendedRecorder) -> None: - """ - Tests linkwhen no additional libraries are specified. - """ - linker = Linker(compiler=stub_c_compiler) - # Add a library to the linker, but don't use it in the link step - linker.add_lib_flags("customlib", ["-lcustom", "-jcustom"]) - - linker.link([Path("a.o")], Path("a.out"), config=stub_configuration) - assert subproc_record.invocations() == [ - ['scc', "a.o", "-o", "a.out"] - ] +def test_c(stub_c_compiler: CCompiler, + stub_configuration: BuildConfig, + subproc_record: ExtendedRecorder) -> None: + """ + Tests linking when no additional libraries are specified. + """ + linker = Linker(compiler=stub_c_compiler) + # Add a library to the linker, but don't use it in the link step + linker.add_lib_flags("customlib", ["-lcustom", "-jcustom"]) + + linker.link([Path("a.o")], Path("a.out"), config=stub_configuration) + assert subproc_record.invocations() == [ + ['scc', "a.o", "-o", "a.out"] + ] def test_c_with_libraries(stub_c_compiler: CCompiler, diff --git a/tests/unit_tests/tools/test_preprocessor.py b/tests/unit_tests/tools/test_preprocessor.py index fb91a8b63..6470600cb 100644 --- a/tests/unit_tests/tools/test_preprocessor.py +++ b/tests/unit_tests/tools/test_preprocessor.py @@ -9,16 +9,14 @@ from logging import Logger from pathlib import Path -from tests.conftest import ExtendedRecorder - from pytest import mark from pytest_subprocess.fake_process import FakeProcess -from tests.conftest import call_list - from fab.tools.category import Category from fab.tools.preprocessor import Cpp, CppFortran, Fpp, Preprocessor +from tests.conftest import call_list, ExtendedRecorder + def test_constructor() -> None: """ @@ -66,7 +64,7 @@ def test_is_not_available(self, fake_process: FakeProcess) -> None: """ Tests CPP in "traditional" mode. """ - command = ['cpp', '-traditional-cpp', '-P', '--version'] + command = ['cpp', '--version'] fake_process.register(command, returncode=1) cppf = CppFortran() @@ -79,5 +77,6 @@ def test_preprocess(self, subproc_record: ExtendedRecorder) -> None: cppf.preprocess(Path("a.in"), Path("a.out"), ["-DDO_SOMETHING"]) assert subproc_record.invocations() == [ ["cpp", "-traditional-cpp", "-P", "a.in", "a.out"], - ["cpp", "-traditional-cpp", "-P", "-DDO_SOMETHING", "a.in", "a.out"] + ["cpp", "-traditional-cpp", "-P", "-DDO_SOMETHING", + "a.in", "a.out"] ] diff --git a/tests/unit_tests/tools/test_psyclone.py b/tests/unit_tests/tools/test_psyclone.py index da02dfa10..6f28c9ef2 100644 --- a/tests/unit_tests/tools/test_psyclone.py +++ b/tests/unit_tests/tools/test_psyclone.py @@ -30,8 +30,6 @@ def test_constructor(): assert psyclone.category == Category.PSYCLONE assert psyclone.name == "psyclone" assert psyclone.exec_name == "psyclone" - # pylint: disable=use-implicit-booleaness-not-comparison - assert psyclone.get_flags() == [] @mark.parametrize("version", ["2.4.0", "2.5.0", "3.0.0", "3.1.0"]) @@ -373,6 +371,6 @@ def test_type_checking_import(monkeypatch) -> None: monkeypatch.setattr(typing, 'TYPE_CHECKING', True) # This import will not actually re-import, since the module # is already imported. But we need this in order to call reload: - # pylint: disable=import-outside-toplevel + # pylint: disable=import-outside-toplevel, reimported import fab.tools.psyclone reload(fab.tools.psyclone) diff --git a/tests/unit_tests/tools/test_rsync.py b/tests/unit_tests/tools/test_rsync.py index b6c9db4b3..bc2bd4ec2 100644 --- a/tests/unit_tests/tools/test_rsync.py +++ b/tests/unit_tests/tools/test_rsync.py @@ -24,7 +24,6 @@ def test_constructor(): assert rsync.category == Category.RSYNC assert rsync.name == "rsync" assert rsync.exec_name == "rsync" - assert rsync.get_flags() == [] def test_check_available(fake_process: FakeProcess) -> None: diff --git a/tests/unit_tests/tools/test_tool.py b/tests/unit_tests/tools/test_tool.py index 7defe84b9..4fd6515f0 100644 --- a/tests/unit_tests/tools/test_tool.py +++ b/tests/unit_tests/tools/test_tool.py @@ -15,8 +15,8 @@ from tests.conftest import ExtendedRecorder, call_list, not_found_callback from fab.tools.category import Category -from fab.tools.flags import ProfileFlags -from fab.tools.tool import CompilerSuiteTool, Tool +from fab.tools.tool import Tool +from fab.tools.compiler_suite_tool import CompilerSuiteTool def test_constructor() -> None: @@ -127,44 +127,6 @@ def test_run_missing(fake_process: FakeProcess) -> None: assert "this is stderr" in str(err.value) -def test_tool_flags_no_profile() -> None: - """ - Test that flags without using a profile work as expected. - """ - tool = Tool("some tool", "stool", Category.MISC) - assert tool.get_flags() == [] - tool.add_flags("-a") - assert tool.get_flags() == ["-a"] - tool.add_flags(["-b", "-c"]) - assert tool.get_flags() == ["-a", "-b", "-c"] - - -def test_tool_profiles(stub_configuration) -> None: - '''Test that profiles work as expected. These tests use internal - implementation details of ProfileFlags, but we need to test that the - exposed flag-related API works as expected - - ''' - tool = Tool("gfortran", "gfortran", Category.FORTRAN_COMPILER) - # Make sure by default we get ProfileFlags - assert isinstance(tool._flags, ProfileFlags) - assert tool.get_flags() == [] - - # Define a profile with no inheritance - stub_configuration.set_profile("mode1") - tool.define_profile("mode1") - assert tool.get_flags(stub_configuration) == [] - tool.add_flags("-flag1", "mode1") - assert tool.get_flags(stub_configuration) == ["-flag1"] - - # Define a profile with inheritance - tool.define_profile("mode2", "mode1") - stub_configuration.set_profile("mode2") - assert tool.get_flags(stub_configuration) == ["-flag1"] - tool.add_flags("-flag2", "mode2") - assert tool.get_flags(stub_configuration) == ["-flag1", "-flag2"] - - class TestToolRun: """ Tests tool run method. diff --git a/tests/unit_tests/tools/test_tool_with_flags.py b/tests/unit_tests/tools/test_tool_with_flags.py new file mode 100644 index 000000000..3b51cb848 --- /dev/null +++ b/tests/unit_tests/tools/test_tool_with_flags.py @@ -0,0 +1,63 @@ +############################################################################## +# (c) Crown copyright Met Office. All rights reserved. +# For further details please refer to the file COPYRIGHT +# which you should have received as part of this distribution +############################################################################## +""" +xtests tooling base classes. +""" +from pathlib import Path + +from fab.tools.category import Category +from fab.tools.flags import ProfileFlags +from fab.tools.tool_with_flags import ToolWithFlags + + +def test_tool_with_flags_constructor(stub_configuration) -> None: + """ + Tests construction from argument list. + """ + tool = ToolWithFlags("gnu", "gfortran", Category.FORTRAN_COMPILER) + assert isinstance(tool._flags, ProfileFlags) + # pylint: disable=use-implicit-booleaness-not-comparison + assert tool.get_flags(stub_configuration, Path()) == [] + + +def test_tool_with_flags_no_profile(stub_configuration) -> None: + """ + Test that flags without using a profile work as expected. + """ + tool = ToolWithFlags("some tool", "stool", Category.MISC) + # pylint: disable=use-implicit-booleaness-not-comparison + assert tool.get_flags(stub_configuration, Path()) == [] + tool.add_flags("-a") + assert tool.get_flags(stub_configuration, Path()) == ["-a"] + tool.add_flags(["-b", "-c"]) + assert tool.get_flags(stub_configuration, Path()) == ["-a", "-b", "-c"] + + +def test_tool_with_flags_profiles(stub_configuration) -> None: + '''xtest that profiles work as expected. These xtests use internal + implementation details of ProfileFlags, but we need to xtest that the + exposed flag-related API works as expected + + ''' + # pylint: disable=use-implicit-booleaness-not-comparison + tool = ToolWithFlags("gfortran", "gfortran", Category.FORTRAN_COMPILER) + # Make sure by default we get ProfileFlags + assert isinstance(tool._flags, ProfileFlags) + assert tool.get_flags(stub_configuration) == [] + + # Define a profile with no inheritance + stub_configuration.set_profile("mode1") + tool.define_profile("mode1") + assert tool.get_flags(stub_configuration) == [] + tool.add_flags("-flag1", "mode1") + assert tool.get_flags(stub_configuration) == ["-flag1"] + + # Define a profile with inheritance + tool.define_profile("mode2", "mode1") + stub_configuration.set_profile("mode2") + assert tool.get_flags(stub_configuration) == ["-flag1"] + tool.add_flags("-flag2", "mode2") + assert tool.get_flags(stub_configuration) == ["-flag1", "-flag2"] diff --git a/tests/unit_tests/tools/test_versioning.py b/tests/unit_tests/tools/test_versioning.py index bf9f41e27..200be8696 100644 --- a/tests/unit_tests/tools/test_versioning.py +++ b/tests/unit_tests/tools/test_versioning.py @@ -31,7 +31,6 @@ def test_git_constructor(self): '''Test the git constructor.''' git = Git() assert git.category == Category.GIT - assert git.get_flags() == [] def test_git_check_available(self, fake_process: FakeProcess) -> None: """ @@ -236,7 +235,6 @@ def test_svn_constructor(self): """ svn = Subversion() assert svn.category == Category.SUBVERSION - assert svn.get_flags() == [] assert svn.name == "Subversion" assert svn.exec_name == "svn" @@ -395,7 +393,6 @@ def test_extract_from_http(self, repo: Tuple[Path, Path], tmp_path: Path): TODO: This is hard to test without a full Apache installation. For the moment we forgo the test on the basis that it's too hard. """ - pass # ============================================================================ @@ -409,6 +406,5 @@ def test_fcm_constructor(self): """ fcm = Fcm() assert fcm.category == Category.FCM - assert fcm.get_flags() == [] assert fcm.name == "FCM" assert fcm.exec_name == "fcm" From 28716cb61a496c76e36bcfa54db475e4b9c731d5 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Tue, 14 Oct 2025 01:05:35 +1100 Subject: [PATCH 18/30] #502 Removed now unnecessary config argument for run. --- source/fab/tools/compiler.py | 5 ++--- source/fab/tools/compiler_wrapper.py | 2 +- source/fab/tools/tool.py | 6 +++--- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 0d9acc6c5..e2ca8c0bc 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -207,7 +207,7 @@ def compile_file(self, input_file: Path, params = self.get_all_commandline_options(config, input_file, output_file, add_flags) - return self.run(config=config, cwd=input_file.parent, + return self.run(cwd=input_file.parent, additional_parameters=params) def check_available(self) -> bool: @@ -483,8 +483,7 @@ def compile_file(self, input_file: Path, output_file, add_flags, syntax_only) - self.run(config=config, cwd=input_file.parent, - additional_parameters=params) + self.run(cwd=input_file.parent, additional_parameters=params) # ============================================================================ diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index 4b9885daf..4576dc040 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -178,7 +178,7 @@ def compile_file(self, input_file: Path, config, input_file, output_file, add_flags=add_flags, syntax_only=syntax_only) - self.run(config=config, cwd=input_file.parent, + self.run(cwd=input_file.parent, additional_parameters=flags) diff --git a/source/fab/tools/tool.py b/source/fab/tools/tool.py index 9e332f0c2..4c0bb1526 100644 --- a/source/fab/tools/tool.py +++ b/source/fab/tools/tool.py @@ -135,7 +135,6 @@ def __str__(self): def run(self, additional_parameters: Optional[ Union[str, Sequence[Union[Path, str]]]] = None, - config: Optional["BuildConfig"] = None, env: Optional[Dict[str, str]] = None, cwd: Optional[Union[Path, str]] = None, capture_output=True) -> str: @@ -146,11 +145,12 @@ def run(self, List of strings or paths to be sent to :func:`subprocess.run` as additional parameters for the command. Any path will be converted to a normal string. - :param config: the config object, used for accessing compilation mode - and paths (if templating is required) :param env: Optional env for the command. By default it will use the current session's environment. + :param cwd: + Optional working directory for the command. By default it will + use the current working directory. :param capture_output: If True, capture and return stdout. If False, the command will print its output directly to the console. From 43d206d1e2e5a6f61abfb1b4f2ba8a24e3c8549f Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Tue, 14 Oct 2025 01:07:03 +1100 Subject: [PATCH 19/30] #502 Fixed flake8. --- source/fab/tools/tool.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/source/fab/tools/tool.py b/source/fab/tools/tool.py index 4c0bb1526..9272b988f 100644 --- a/source/fab/tools/tool.py +++ b/source/fab/tools/tool.py @@ -16,13 +16,10 @@ import logging from pathlib import Path import subprocess -from typing import Dict, List, Optional, Sequence, TYPE_CHECKING, Union +from typing import Dict, List, Optional, Sequence, Union from fab.tools.category import Category -if TYPE_CHECKING: - from fab.build_config import BuildConfig - class Tool: '''This is the base class for all tools. It stores the name of the tool, From 5c3312562d066e23a73672d44d8479193f653279 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Tue, 14 Oct 2025 10:35:25 +1100 Subject: [PATCH 20/30] #313 Finally remove special handling of flags in compiler. --- source/fab/tools/compiler.py | 20 +++++-------------- source/fab/tools/compiler_wrapper.py | 8 ++------ source/fab/tools/linker.py | 4 ++-- tests/unit_tests/tools/test_compiler.py | 5 ++--- .../unit_tests/tools/test_compiler_wrapper.py | 9 +++------ 5 files changed, 14 insertions(+), 32 deletions(-) diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index e2ca8c0bc..f14ad7641 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -119,7 +119,7 @@ def get_hash(self, """ all_params = (self.name + self.get_version_string() + - str(self.get_resolved_flags(config, file_path))) + str(self.get_flags(config, file_path))) return string_checksum(all_params) def get_all_commandline_options( @@ -150,7 +150,7 @@ def get_all_commandline_options( # Explicitly add all compilation flags here, where the input # path can be provided to properly resolve path-specific flags. - params += self.get_resolved_flags(config, input_file) + params += self.get_flags(config, input_file) if add_flags: if self.openmp_flag in add_flags: @@ -165,20 +165,10 @@ def get_all_commandline_options( def get_flags(self, config: Optional["BuildConfig"] = None, file_path: Optional[Path] = None) -> List[str]: - '''Since compiler need path-specific information (and the - `run` method in tool does not provide the path), a compiler - will return an empty list as flags. All compiler flags - are added explicitly by `get_all_commandline_options`, - where the input path can be provided. - - :returns: the flags to be added automatically by tool, which - is just empty, since all compiler flags are added explicitly. - ''' - return [] - - def get_resolved_flags(self, config: "BuildConfig", - file_path: Path) -> List[str]: """ + The flags to use when compiling the specified flag. All + AbstractFlags (e.g. MatchFlags, ...) will be resolved. + :param config: The build configuration to use. :param file_path: the path to the file to be compiled. diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index 4576dc040..5f45bf54c 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -78,12 +78,8 @@ def get_flags(self, config: Optional["BuildConfig"] = None, :param profile: the profile to use. ''' - return [] - - def get_resolved_flags(self, config: "BuildConfig", - file_path: Path) -> List[str]: - return (self._compiler.get_resolved_flags(config, file_path) + - super().get_resolved_flags(config, file_path)) + return (self._compiler.get_flags(config, file_path) + + super().get_flags(config, file_path)) def set_module_output_path(self, path: Path): '''Sets the output path for modules. diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index e46bc0e6d..293724010 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -129,7 +129,7 @@ def get_profile_flags(self, config: "BuildConfig") -> List[str]: # A compiler can have path specific flags. Since we are only # interested in linking here, path-specific flags are not # needed, so we provide a dummy file-path. - return flags + self._compiler.get_resolved_flags(config, Path("")) + return flags + self._compiler.get_flags(config, Path("")) def get_lib_flags(self, lib: str) -> List[str]: '''Gets the standard flags for a standard library @@ -241,7 +241,7 @@ def link(self, input_files: List[Path], output_file: Path, params: List[str] = self.get_flags(config) - params.extend(self._compiler.get_resolved_flags(config, Path())) + params.extend(self._compiler.get_flags(config, Path())) if config.openmp: params.append(self._compiler.openmp_flag) diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index 52aef644a..39842fda4 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -143,10 +143,9 @@ def test_compiler_path_specific_flags(stub_configuration, fc.add_flags("-always-flag") fc.add_flags(contain_flag) - flags = fc.get_resolved_flags(stub_configuration, Path(".")) + flags = fc.get_flags(stub_configuration, Path(".")) assert flags == ["-always-flag"] - flags = fc.get_resolved_flags(stub_configuration, - Path("/somewhere/myfile.F90")) + flags = fc.get_flags(stub_configuration, Path("/somewhere/myfile.F90")) assert flags == ["-always-flag", "-myflag"] compiler_info = "some Fortran compiler1.2['-always-flag']" diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index 804dc879c..1c1a0b99d 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -265,15 +265,12 @@ def test_flags_independent(stub_c_compiler: CCompiler, assert wrapper.get_flags() == [] stub_c_compiler.add_flags(['-a', '-b']) - # Compiler flags are handled differently in a compiler (since a compiler - # needs path-specific flags, but the generic get_flags call from Tools - # does not provide a path). So the compiler should report no flags: - assert stub_c_compiler.get_flags() == [] + assert stub_c_compiler.get_flags() == ["-a", "-b"] resolved_flags = stub_c_compiler.get_all_commandline_options( stub_configuration, Path('/in'), Path("/out")) assert resolved_flags == ['-c', '-a', '-b', 'in', '-o', '/out'] - assert wrapper.get_flags() == [] + assert wrapper.get_flags() == ["-a", "-b"] # We need to test `get_all_commandline_options` to check the correct # behaviour of flags, which can resolve path-specific flags. @@ -293,7 +290,7 @@ def test_flags_independent(stub_c_compiler: CCompiler, # And the compiler wrapper should report the wrapped compiler's flag # followed by the wrapper flag (i.e. the wrapper flag can therefore # overwrite the wrapped compiler's flags) - assert wrapper.get_flags() == [] + assert wrapper.get_flags() == ["-a", "-b", "-d", "-e"] resolved_flags = wrapper.get_all_commandline_options(stub_configuration, Path("/in"), Path("/out")) From c449bc077d2db380841c846b2adeb75899e4725a Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Tue, 14 Oct 2025 10:38:51 +1100 Subject: [PATCH 21/30] #313 Fixed debugging typos. --- tests/unit_tests/tools/test_tool_with_flags.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/unit_tests/tools/test_tool_with_flags.py b/tests/unit_tests/tools/test_tool_with_flags.py index 3b51cb848..51c4c62e8 100644 --- a/tests/unit_tests/tools/test_tool_with_flags.py +++ b/tests/unit_tests/tools/test_tool_with_flags.py @@ -3,9 +3,11 @@ # For further details please refer to the file COPYRIGHT # which you should have received as part of this distribution ############################################################################## + """ -xtests tooling base classes. +Tests ToolsWithFlags """ + from pathlib import Path from fab.tools.category import Category @@ -37,11 +39,12 @@ def test_tool_with_flags_no_profile(stub_configuration) -> None: def test_tool_with_flags_profiles(stub_configuration) -> None: - '''xtest that profiles work as expected. These xtests use internal - implementation details of ProfileFlags, but we need to xtest that the + """ + Test that profiles work as expected. These tests use internal + implementation details of ProfileFlags, but we need to test that the exposed flag-related API works as expected + """ - ''' # pylint: disable=use-implicit-booleaness-not-comparison tool = ToolWithFlags("gfortran", "gfortran", Category.FORTRAN_COMPILER) # Make sure by default we get ProfileFlags From 68fcb6fffb39848d0baef95522a5cd02e984f671 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Tue, 18 Nov 2025 15:19:14 +1100 Subject: [PATCH 22/30] #525 Add code and test to recognise a Fortran compiler wrapper to be Fortran when searching for a default linker. --- source/fab/tools/tool_repository.py | 3 +++ tests/unit_tests/tools/test_tool_repository.py | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/source/fab/tools/tool_repository.py b/source/fab/tools/tool_repository.py index ed23cab16..a7ed86749 100644 --- a/source/fab/tools/tool_repository.py +++ b/source/fab/tools/tool_repository.py @@ -290,6 +290,9 @@ def get_default(self, category: Category, if category == Category.LINKER: tool = cast(Linker, tool) compiler = tool.compiler + # Find the real compiler if we have a compiler wrapper: + while isinstance(compiler, CompilerWrapper): + compiler = compiler.compiler # Ignore C linker if Fortran is requested and vice versa: if (enforce_fortran_linker and not isinstance(compiler, FortranCompiler)): diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index 3e4a803c0..c80efc5ae 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -16,6 +16,7 @@ from fab.tools.category import Category from fab.tools.compiler import Compiler, FortranCompiler, Gfortran, Ifort from fab.tools.compiler_wrapper import Mpif90 +from fab.tools.linker import Linker from fab.tools.tool_repository import ToolRepository from tests.conftest import call_list @@ -124,6 +125,16 @@ def test_get_default(stub_tool_repository, stub_fortran_compiler, ar = stub_tool_repository.get_default(Category.AR) assert isinstance(ar, Ar) + # Now add a linker around a compiler wrapper, to make sure the compiler + # wrapper is recognised as a Fortran compiler: + linker = Linker(Mpif90(fc)) + linker._is_available = True + stub_tool_repository.add_tool(linker) + for_link = stub_tool_repository.get_default(Category.LINKER, mpi=True, + openmp=False, + enforce_fortran_linker=True) + assert for_link is linker + def test_get_default_error_invalid_category() -> None: """ From 7b8f511c35be8eaee9c084294ece7e0cf78429c7 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Tue, 18 Nov 2025 15:46:11 +1100 Subject: [PATCH 23/30] #525 Added additional test to cover additional line. --- .../unit_tests/tools/test_tool_repository.py | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index c80efc5ae..5fb42b5a3 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -15,7 +15,7 @@ from fab.tools.ar import Ar from fab.tools.category import Category from fab.tools.compiler import Compiler, FortranCompiler, Gfortran, Ifort -from fab.tools.compiler_wrapper import Mpif90 +from fab.tools.compiler_wrapper import Mpicc, Mpif90 from fab.tools.linker import Linker from fab.tools.tool_repository import ToolRepository @@ -125,16 +125,33 @@ def test_get_default(stub_tool_repository, stub_fortran_compiler, ar = stub_tool_repository.get_default(Category.AR) assert isinstance(ar, Ar) - # Now add a linker around a compiler wrapper, to make sure the compiler + +def test_get_default_linker_with_wrapper(stub_tool_repository, + stub_fortran_compiler, + stub_c_compiler) -> None: + """ + Tests that we get the right linker if compiler wrapper are used. + """ + + # Add a linker around a compiler wrapper, to test that the compiler # wrapper is recognised as a Fortran compiler: - linker = Linker(Mpif90(fc)) + linker = Linker(Mpif90(stub_fortran_compiler)) linker._is_available = True stub_tool_repository.add_tool(linker) for_link = stub_tool_repository.get_default(Category.LINKER, mpi=True, - openmp=False, + openmp=True, enforce_fortran_linker=True) assert for_link is linker + # Now the same for a linker around a C compiler wrapper: + linker = Linker(Mpicc(stub_c_compiler)) + linker._is_available = True + stub_tool_repository.add_tool(linker) + cc_link = stub_tool_repository.get_default(Category.LINKER, mpi=True, + openmp=True, + enforce_fortran_linker=False) + assert cc_link is linker + def test_get_default_error_invalid_category() -> None: """ @@ -163,6 +180,11 @@ def test_get_default_error_missing_mpi() -> None: assert str(err.value) == ("Invalid or missing openmp specification " "for 'FORTRAN_COMPILER'.") + with raises(RuntimeError) as err: + tr.get_default(Category.LINKER, mpi=True, openmp=True) + assert str(err.value) == ("Invalid or missing enforce_fortran_linker " + "specification for 'LINKER'.") + def test_get_default_error_missing_openmp() -> None: """ From a8f0c66689a2a1496f20938cd2b7c20602975077 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 20 Nov 2025 16:26:12 +1100 Subject: [PATCH 24/30] #313 Remove old way of providing path-specific flags per compiler. --- source/fab/fab_base/fab_base.py | 4 --- .../fab_base/site_specific/default/config.py | 34 +++++-------------- .../site_specific/default/setup_cray.py | 10 +++--- .../site_specific/default/setup_gnu.py | 10 +++--- .../default/setup_intel_classic.py | 12 +++---- .../site_specific/default/setup_intel_llvm.py | 12 +++---- .../site_specific/default/setup_nvidia.py | 10 +++--- .../fab_base/site_specific/default/config.py | 10 +----- 8 files changed, 31 insertions(+), 71 deletions(-) diff --git a/source/fab/fab_base/fab_base.py b/source/fab/fab_base/fab_base.py index c4fe2f8e6..3b2c47a7f 100755 --- a/source/fab/fab_base/fab_base.py +++ b/source/fab/fab_base/fab_base.py @@ -672,8 +672,6 @@ def compile_c_step( alternative source can also be passed to Fab for compilation. """ site_path_flags: List[AddFlags] = [] - if self._site_config: - site_path_flags = self._site_config.get_path_flags(self._config) if not common_flags: common_flags = [] assert isinstance(common_flags, list) @@ -700,8 +698,6 @@ def compile_fortran_step( to Fab compile_fortran, default is None. """ site_path_flags: List[AddFlags] = [] - if self._site_config: - site_path_flags = self._site_config.get_path_flags(self._config) if not common_flags: common_flags = [] if not path_flags: diff --git a/source/fab/fab_base/site_specific/default/config.py b/source/fab/fab_base/site_specific/default/config.py index bc611d297..264d1d7ce 100644 --- a/source/fab/fab_base/site_specific/default/config.py +++ b/source/fab/fab_base/site_specific/default/config.py @@ -6,10 +6,9 @@ ''' import argparse -from typing import cast, Dict, List -from fab.build_config import AddFlags, BuildConfig -from fab.tools import Category, Compiler, ToolRepository +from fab.build_config import BuildConfig +from fab.tools import Category, ToolRepository from fab.fab_base.site_specific.default.setup_cray import setup_cray from fab.fab_base.site_specific.default.setup_gnu import setup_gnu @@ -30,11 +29,6 @@ class Config: def __init__(self) -> None: self._args: argparse.Namespace - # Stores for each compiler suite a mapping of profiles to the list of - # path-specific flags to use. - # _path_flags[suite][profile] - self._path_flags: Dict[str, Dict[str, List[AddFlags]]] = {} - @property def args(self) -> argparse.Namespace: ''' @@ -42,7 +36,7 @@ def args(self) -> argparse.Namespace: ''' return self._args - def get_valid_profiles(self) -> List[str]: + def get_valid_profiles(self) -> list[str]: ''' Determines the list of all allowed compiler profiles. The first entry in this list is the default profile to be used. This method @@ -101,16 +95,6 @@ def update_toolbox(self, build_config: BuildConfig) -> None: self.setup_nvidia(build_config) self.setup_cray(build_config) - def get_path_flags(self, build_config: BuildConfig) -> List[AddFlags]: - ''' - Returns the path-specific flags to be used. - TODO #313: Ideally we have only one kind of flag, but as a quick - work around we provide this method. - ''' - compiler = build_config.tool_box[Category.FORTRAN_COMPILER] - compiler = cast(Compiler, compiler) - return self._path_flags[compiler.suite].get(build_config.profile, []) - def setup_cray(self, build_config: BuildConfig) -> None: ''' This method sets up the Cray compiler and linker flags. @@ -120,7 +104,7 @@ def setup_cray(self, build_config: BuildConfig) -> None: :param build_config: the Fab build configuration instance ''' - self._path_flags["cray"] = setup_cray(build_config, self.args) + setup_cray(build_config, self.args) def setup_gnu(self, build_config: BuildConfig) -> None: ''' @@ -131,7 +115,7 @@ def setup_gnu(self, build_config: BuildConfig) -> None: :param build_config: the Fab build configuration instance ''' - self._path_flags["gnu"] = setup_gnu(build_config, self.args) + setup_gnu(build_config, self.args) def setup_intel_classic(self, build_config: BuildConfig) -> None: ''' @@ -142,8 +126,7 @@ def setup_intel_classic(self, build_config: BuildConfig) -> None: :param build_config: the Fab build configuration instance ''' - self._path_flags["intel_classic"] = setup_intel_classic(build_config, - self.args) + setup_intel_classic(build_config, self.args) def setup_intel_llvm(self, build_config: BuildConfig) -> None: ''' @@ -154,8 +137,7 @@ def setup_intel_llvm(self, build_config: BuildConfig) -> None: :param build_config: the Fab build configuration instance ''' - self._path_flags["intel-llvm"] = setup_intel_llvm(build_config, - self.args) + setup_intel_llvm(build_config, self.args) def setup_nvidia(self, build_config: BuildConfig) -> None: ''' @@ -166,4 +148,4 @@ def setup_nvidia(self, build_config: BuildConfig) -> None: :param build_config: the Fab build configuration instance ''' - self._path_flags["nvidia"] = setup_nvidia(build_config, self.args) + setup_nvidia(build_config, self.args) diff --git a/source/fab/fab_base/site_specific/default/setup_cray.py b/source/fab/fab_base/site_specific/default/setup_cray.py index dd6875013..db615a109 100644 --- a/source/fab/fab_base/site_specific/default/setup_cray.py +++ b/source/fab/fab_base/site_specific/default/setup_cray.py @@ -8,14 +8,14 @@ ''' import argparse -from typing import cast, Dict, List +from typing import cast -from fab.build_config import AddFlags, BuildConfig +from fab.build_config import BuildConfig from fab.tools import Category, Compiler, Linker, ToolRepository def setup_cray(build_config: BuildConfig, - args: argparse.Namespace) -> Dict[str, List[AddFlags]]: + args: argparse.Namespace) -> None: # pylint: disable=unused-argument, too-many-branches ''' Defines the default flags for ftn. @@ -30,7 +30,7 @@ def setup_cray(build_config: BuildConfig, ftn = cast(Compiler, ftn) if not ftn.is_available: - return {} + return # The base flags # ============== @@ -101,5 +101,3 @@ def setup_cray(build_config: BuildConfig, # You can use: # ftn = tr.get_tool(Category.FORTRAN_COMPILER, "crayftn-gfortran") # ftn.add_flags("-fallow-argument-mismatch") - - return {} diff --git a/source/fab/fab_base/site_specific/default/setup_gnu.py b/source/fab/fab_base/site_specific/default/setup_gnu.py index f22110ad2..6329f5f15 100644 --- a/source/fab/fab_base/site_specific/default/setup_gnu.py +++ b/source/fab/fab_base/site_specific/default/setup_gnu.py @@ -8,9 +8,9 @@ ''' import argparse -from typing import cast, Dict, List +from typing import cast -from fab.build_config import AddFlags, BuildConfig +from fab.build_config import BuildConfig from fab.tools.category import Category from fab.tools.linker import Linker from fab.tools.tool_repository import ToolRepository @@ -18,7 +18,7 @@ def setup_gnu(build_config: BuildConfig, - args: argparse.Namespace) -> Dict[str, List[AddFlags]]: + args: argparse.Namespace) -> None: # pylint: disable=unused-argument ''' Defines the default flags for all GNU compilers and linkers. @@ -34,7 +34,7 @@ def setup_gnu(build_config: BuildConfig, if not gfortran.is_available: gfortran = tr.get_tool(Category.FORTRAN_COMPILER, "mpif90-gfortran") if not gfortran.is_available: - return {} + return gfortran = cast(Compiler, gfortran) @@ -73,5 +73,3 @@ def setup_gnu(build_config: BuildConfig, # Add more flags to be always used, e.g.: # linker.add_post_lib_flags(["-lstdc++"], "base") - - return {} diff --git a/source/fab/fab_base/site_specific/default/setup_intel_classic.py b/source/fab/fab_base/site_specific/default/setup_intel_classic.py index 5d9e8abea..87b77bad7 100644 --- a/source/fab/fab_base/site_specific/default/setup_intel_classic.py +++ b/source/fab/fab_base/site_specific/default/setup_intel_classic.py @@ -8,21 +8,21 @@ ''' import argparse -from typing import cast, Dict, List +from typing import cast -from fab.build_config import AddFlags, BuildConfig +from fab.build_config import BuildConfig from fab.tools import Category, Compiler, Linker, ToolRepository def setup_intel_classic(build_config: BuildConfig, - args: argparse.Namespace) -> Dict[str, List[AddFlags]]: + args: argparse.Namespace) -> None: # pylint: disable=unused-argument, too-many-locals ''' Defines the default flags for all Intel classic compilers and linkers. :param build_config: the Fab build config instance from which required parameters can be taken. - :param argparse.Namespace args: all command line options + :param args: all command line options ''' tr = ToolRepository() @@ -38,7 +38,7 @@ def setup_intel_classic(build_config: BuildConfig, if not ifort.is_available: # Since some flags depends on version, the code below requires # that the intel compiler actually works. - return {} + return # The base flags # ============== @@ -75,5 +75,3 @@ def setup_intel_classic(build_config: BuildConfig, # Add more flags to be always used, e.g.: # linker.add_post_lib_flags(["-lstdc++"], "base") - - return {} diff --git a/source/fab/fab_base/site_specific/default/setup_intel_llvm.py b/source/fab/fab_base/site_specific/default/setup_intel_llvm.py index c9caed575..a91a76594 100644 --- a/source/fab/fab_base/site_specific/default/setup_intel_llvm.py +++ b/source/fab/fab_base/site_specific/default/setup_intel_llvm.py @@ -8,21 +8,21 @@ ''' import argparse -from typing import cast, Dict, List +from typing import cast -from fab.build_config import AddFlags, BuildConfig +from fab.build_config import BuildConfig from fab.tools import Category, Compiler, Linker, ToolRepository def setup_intel_llvm(build_config: BuildConfig, - args: argparse.Namespace) -> Dict[str, List[AddFlags]]: + args: argparse.Namespace) -> None: # pylint: disable=unused-argument, too-many-locals ''' Defines the default flags for all Intel llvm compilers. :param build_config: the Fab build config instance from which required parameters can be taken. - :param argparse.Namespace args: all command line options + :param args: all command line options ''' tr = ToolRepository() @@ -33,7 +33,7 @@ def setup_intel_llvm(build_config: BuildConfig, ifx = tr.get_tool(Category.FORTRAN_COMPILER, "mpif90-ifx") ifx = cast(Compiler, ifx) if not ifx.is_available: - return {} + return # The base flags # ============== @@ -64,5 +64,3 @@ def setup_intel_llvm(build_config: BuildConfig, # Add more flags to be always used, e.g.: # linker.add_post_lib_flags(["-lstdc++"], "base") - - return {} diff --git a/source/fab/fab_base/site_specific/default/setup_nvidia.py b/source/fab/fab_base/site_specific/default/setup_nvidia.py index 5055f05b7..08c16d104 100644 --- a/source/fab/fab_base/site_specific/default/setup_nvidia.py +++ b/source/fab/fab_base/site_specific/default/setup_nvidia.py @@ -8,14 +8,14 @@ ''' import argparse -from typing import cast, Dict, List +from typing import cast -from fab.build_config import AddFlags, BuildConfig +from fab.build_config import BuildConfig from fab.tools import Category, Compiler, Linker, ToolRepository def setup_nvidia(build_config: BuildConfig, - args: argparse.Namespace) -> Dict[str, List[AddFlags]]: + args: argparse.Namespace) -> None: # pylint: disable=unused-argument ''' Defines the default flags for nvfortran. @@ -33,7 +33,7 @@ def setup_nvidia(build_config: BuildConfig, nvfortran = tr.get_tool(Category.FORTRAN_COMPILER, "mpif90-nvfortran") nvfortran = cast(Compiler, nvfortran) if not nvfortran.is_available: - return {} + return # The base flags # ============== @@ -66,5 +66,3 @@ def setup_nvidia(build_config: BuildConfig, # Always link with C++ libs # linker.add_post_lib_flags(["-c++libs"], "base") - - return {} diff --git a/tests/unit_tests/fab_base/site_specific/default/config.py b/tests/unit_tests/fab_base/site_specific/default/config.py index dae897f66..369195d97 100644 --- a/tests/unit_tests/fab_base/site_specific/default/config.py +++ b/tests/unit_tests/fab_base/site_specific/default/config.py @@ -8,7 +8,7 @@ import argparse from typing import List -from fab.build_config import AddFlags, BuildConfig +from fab.build_config import BuildConfig from fab.tools import Category, ToolRepository @@ -83,11 +83,3 @@ def handle_command_line_options(self, args: argparse.Namespace) -> None: # Keep a copy of the args, so they can be used when # initialising compilers self._args = args - - def get_path_flags(self, build_config: BuildConfig) -> List[AddFlags]: - ''' - Returns the path-specific flags to be used. - TODO #313: Ideally we have only one kind of flag, but as a quick - work around we provide this method. - ''' - return [] From 1fa7adca87d95c10255dcc0118f1708d8033183c Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Fri, 28 Nov 2025 17:15:00 +1100 Subject: [PATCH 25/30] #313 Removed __eq__ method, which was only used to simplify testing, and made git's code quality and flake unhappy. --- source/fab/tools/flags.py | 27 +++------------ tests/unit_tests/tools/test_flags.py | 50 +++++++++++++++------------- 2 files changed, 30 insertions(+), 47 deletions(-) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index fb20c5cd7..5fe82e2e8 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -50,13 +50,12 @@ import logging from pathlib import Path from string import Template -from typing import Dict, List, Optional, TYPE_CHECKING, Union +from typing import Dict, List, Optional, Union import warnings from fab.util import string_checksum -if TYPE_CHECKING: - from fab.build_config import AddFlags, BuildConfig +from fab.build_config import AddFlags, BuildConfig logger = logging.getLogger(__name__) @@ -122,21 +121,6 @@ def __init__(self, flags: Optional[Union[str, List[str]]] = None) -> None: else: self._flags = [] - def __eq__(self, other: object) -> bool: - """ - :returns: if two AlwaysFlags are identical - """ - if not isinstance(other, AlwaysFlags): - raise NotImplementedError( - f"Cannot compare '{type(self).__name__}' with object " - f"of type '{type(other).__name__}'.") - # Flake insists to use isinstance. But in this case - # we explicitly do not want to allow subclasses, e.g. - # AlwaysFlag should never be equal to a MatchFlag - # pylint: disable=unidiomatic-typecheck - return (type(self) == type(other) and # noqa - self._flags == other._flags) - @staticmethod def replace_template(string_list: List[str], config: Optional["BuildConfig"] = None, @@ -316,8 +300,8 @@ def __init__( self, list_of_flags: Optional[Union[AbstractFlags, str, List[str]]] = None, - add_flags: Optional[Union["AddFlags", - List["AddFlags"]]] = None) -> None: + add_flags: Optional[Union[AddFlags, + List[AddFlags]]] = None) -> None: self._logger = logging.getLogger(__name__) super().__init__() if isinstance(list_of_flags, (str, list)): @@ -325,9 +309,6 @@ def __init__( elif list_of_flags: self.append(list_of_flags) if add_flags: - # pylint: disable=import-outside-toplevel - # TODO: circular import otherwise - from fab.build_config import AddFlags if isinstance(add_flags, AddFlags): add_flags = [add_flags] # Convert old-style AddFlags to the new MatchFlags: diff --git a/tests/unit_tests/tools/test_flags.py b/tests/unit_tests/tools/test_flags.py index 5fa0ccb1a..87ddcf869 100644 --- a/tests/unit_tests/tools/test_flags.py +++ b/tests/unit_tests/tools/test_flags.py @@ -29,14 +29,6 @@ def test_always_flags(stub_configuration): af = AlwaysFlags(["-g", "-O2"]) assert af.get_flags() == ["-g", "-O2"] - # Comparison: First different derived type: - cf_copy = ContainFlags("XX", ["-g", "-O2"]) - assert af != cf_copy - af_copy = AlwaysFlags(["-g", "-O2"]) - assert af_copy == af - af_copy = AlwaysFlags(["-O2", "-g"]) - assert af_copy != af - # Templating af = AlwaysFlags(["$source", "$output"]) assert (af.get_flags(stub_configuration) == @@ -49,13 +41,6 @@ def test_always_flags(stub_configuration): str(stub_configuration.build_output), "/my"]) - # Test comparison of different objects - with pytest.raises(NotImplementedError) as err: - # pylint: disable=pointless-statement - af == 1 - assert ("Cannot compare 'AlwaysFlags' with object of type 'int'." - in str(err.value)) - def test_always_flags_remove_flags(): '''Test remove_flags functionality.''' @@ -132,8 +117,9 @@ def test_flags_adding(): f1.add_flags(["-b", "-c"]) assert len(f1) == 2 assert f1.get_flags() == ["-a", "-b", "-c"] - assert f1[0] == AlwaysFlags("-a") - assert f1[1] == AlwaysFlags(["-b", "-c"]) + assert len(f1) == 2 + assert f1[0].get_flags() == ["-a"] + assert f1[1].get_flags() == ["-b", "-c"] # Check functionality when adding a flag object: af1 = AlwaysFlags("-g") @@ -183,10 +169,17 @@ def test_profile_flags_with_profile(): pf.define_profile("base") assert pf["base"] == [] pf.add_flags("-base", "base") - assert pf["base"] == [AlwaysFlags("-base")] + + assert len(pf["base"]) == 1 + assert isinstance(pf["base"][0], AlwaysFlags) + assert pf["base"][0].get_flags() == ["-base"] + pf.add_flags(["-base2", "-base3"], "base") - assert pf["base"] == [AlwaysFlags("-base"), - AlwaysFlags(["-base2", "-base3"])] + assert len(pf["base"]) == 2 + assert isinstance(pf["base"][0], AlwaysFlags) + assert isinstance(pf["base"][1], AlwaysFlags) + assert pf["base"][0].get_flags() == ["-base"] + assert pf["base"][1].get_flags() == ["-base2", "-base3"] # Check that we get an exception if we specify a profile # that does not exist @@ -198,10 +191,15 @@ def test_profile_flags_with_profile(): def test_profile_flags_constructor_args(): '''Tests various constructor argument combinations.''' pf = ProfileFlags("-g") - assert pf[""] == [AlwaysFlags("-g")] + assert len(pf[""]) == 1 + assert isinstance(pf[""][0], AlwaysFlags) + assert pf[""][0].get_flags() == ["-g"] + pf = ProfileFlags("-g", profile="prof") assert pf[""] == [] - assert pf["prof"] == [AlwaysFlags("-g")] + assert len(pf["prof"]) == 1 + assert isinstance(pf["prof"][0], AlwaysFlags) + assert pf["prof"][0].get_flags() == ["-g"] def test_profile_flags_without_profile(): @@ -210,9 +208,13 @@ def test_profile_flags_without_profile(): assert pf[""] == [] assert pf[None] == [] pf.add_flags("-base") - assert pf[""] == [AlwaysFlags("-base")] + assert len(pf[""]) == 1 + assert isinstance(pf[""][0], AlwaysFlags) + assert pf[""][0].get_flags() == ["-base"] pf.add_flags(["-base2", "-base3"]) - assert pf[""] == [AlwaysFlags("-base"), AlwaysFlags(["-base2", "-base3"])] + assert len(pf[""]) == 2 + assert pf[""][0].get_flags() == ["-base"] + assert pf[""][1].get_flags() == ["-base2", "-base3"] # Check that we get an exception if we specify a profile with pytest.raises(KeyError) as err: From a60d1e46b7c335034f7297eb0cb11e9fb73375a8 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Fri, 28 Nov 2025 17:15:25 +1100 Subject: [PATCH 26/30] #313 Fixed typing inconsistency. --- source/fab/tools/tool_with_flags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/fab/tools/tool_with_flags.py b/source/fab/tools/tool_with_flags.py index 01815ba58..673716d5a 100644 --- a/source/fab/tools/tool_with_flags.py +++ b/source/fab/tools/tool_with_flags.py @@ -51,7 +51,7 @@ def flags(self) -> ProfileFlags: return self._flags def get_flags(self, - config: Optional["BuildConfig"], + config: Optional["BuildConfig"] = None, file_path: Optional[Path] = None) -> List[str]: ''':returns: the flags to be used with this tool.''' return self.flags.get_flags(config, file_path) From 1b13a74b6c04513a7d5d0bc4fc29bbb7924ad3ee Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 28 Jan 2026 15:51:46 +1100 Subject: [PATCH 27/30] #313 Added documentation for new Flag classes. --- Documentation/source/advanced_config.rst | 140 ++++++++++++++++++++++- 1 file changed, 139 insertions(+), 1 deletion(-) diff --git a/Documentation/source/advanced_config.rst b/Documentation/source/advanced_config.rst index fa2ced661..cfa63d64d 100644 --- a/Documentation/source/advanced_config.rst +++ b/Documentation/source/advanced_config.rst @@ -32,7 +32,7 @@ In not-representative tests, importing tool using ``api`` takes around The list of functions and classes provided: -.. literalinclude:: ../../source/fab/api/__init__.py +.. literalinclude:: ../../source/fab/api.py :start-after: __all__ = [ :end-before: ] @@ -404,6 +404,144 @@ instance itself, as shown further above, is the better approach. Path-specific flags ------------------- +A common use case in complex applications is the need to specify +file-specific compiler flags. For example, a file might need to enable +additional optimisations, or disable other optimisations. + +Fab uses a set of classes to manage flags consistently across +all tools. + +AlwaysFlags + This object represents a list of flags that are always used, i.e. + they are independent of the filename being processed. It provides + templating capabilities, i.e. a set of keywords starting with `$` + will be replaced with a value. The following keywords are defined + if a config file is specified: + + - ``$source`` for */source* + - ``$output`` for */build_output* + - ``$relative`` for ** + + These values are taken from the build configuration (if specified). + + Example: + + .. code-block:: + :linenos: + :caption: Creating an AlwaysFlags instance + + from fab.api import AlwaysFlags + + # Assuming that build_config specifies /some/path/fab + # as project directory for Fab: + af = AlwaysFlags(["-g", "-O2", "-I$source/um/include/other"]) + assert (af.get_flags(build_config) == + ["-g", "-O2", "-I/some/path/fab/source/um/include/other"]) + + +MatchFlags (based on ``AlwaysFlags``) + This represents a set of flags that are only applied if a wildcard + search matches the source file to be processed. Note that a wildcard + search must in general match the full path (unless the pattern starts + with a ``*``). The templates explained in the ``AlwaysFlags`` can be + used to create absolute paths in the pattern as well. + + Example: + + .. code-block:: + :linenos: + :caption: Creating a MatchFlags instance + + from fab.api import MatchFlags + + mf = MatchFlags("/some/dir", "-g") + + assert mf.get_flags(file_path=Path("/some/dir")) == ["-g"] + assert mf.get_flags(file_path=Path("/other/some/dir")) == [] + + # Starting a pattern with * will match a: + mf = MatchFlags("*/some/dir", "-g") + assert mf.get_flags(file_path=Path("/other/some/dir")) == ["-g"] + + +ContainFlags (based on ``AlwaysFlags``) + Flags that are only applied if the file path contains the specified + string. The difference to ``MatchFlags`` is that ``ContainFlags`` do not + need the full path to be specified. + + Example: + + .. code-block:: + :linenos: + :caption: Creating a ContainFlags instance + + from fab.api import ContainFlags + + cf = ContainFlags(pattern="/some/dir", "-g") + + assert cf.get_flags(file_path=Path("/not/in/root/some/dir")) == ["-g"] + assert cf.get_flags(file_path=Path("/some/other")) == [] + +FlagList: + This class represents the main class to manage a set of flags. + Each of its members is either an ``AlwaysFlags``, ``MatchFlags`` + or ``ContainFlags``. When the list is evaluated, each individual + flag instance is evaluated and will depending on the processed + file add flags or not. + + This class provides convenient constructor which will convert + a string or list of strings to an ``AlwaysFlags`` instance. + It also provides backwards compatibility to the previous + ``AddFlag`` object (see :ref:`add_flag`): + + Example: + + .. code-block:: + :linenos: + :caption: Creating a FlagList + + from fab.api import ContainFlags, FlagList + from fab.build_config import AddFlags + + cf = ContainFlags(pattern="/some/dir", "-g") + add_flags = AddFlags(match="/some/pattern", flags=["-g", "-O0"]) + # This will trigger conversion of add_flags to MathFlags + flag_list = FlagList(add_flags=[add_flags]) + +ProfileFlags: + Tools have a ``ProfileFlags`` as member, which they use to manage + all their flags. Depending on the selected profile, this ``ProfileFlags`` + members returns the ``FlagList`` to use. As explained earlier, each of + the members of FlagList can be any of the previous flag types to give + flexibility for a user to specify the compilation flags. See + :ref:`Advanced Flags` for details about profiling modes. + + Example: + + .. code-block:: + :linenos: + :caption: Setting up compiler for the UM + + tr = ToolRepository() + ifx = tr.get_tool(Category.FORTRAN_COMPILER, "ifx") + + # These will be converted to an AlwaysFlag: + ifx.add_flags(["-stand", "f08"], "base") + + # ifx 2025.2 has no omp_version in omp_lib, so we + # have to disable OpenMP: + if (2025, 2) <= ifx.get_version() < (2025, 3): + ifx.add_flags(ContainFlags("/um_shell.", ["-qno-openmp"]), "base") + + # Prevent loss of comparison across decompositions when OpenMP on + ifx.add_flags(ContainFlags("/ni_imp_ctl.", "-qno-openmp"), "base") + + + +.. _add_flag: + +Path-specific flags - old style +------------------------------- For preprocessing and compilation, we sometimes need to specify flags *per-file*. These steps accept both common flags and *path specific* flags. From 1491039d9b64f842e95e28b5954a5229c8fda7e6 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 28 Jan 2026 16:16:04 +1100 Subject: [PATCH 28/30] #313 Minor code cleanups. --- source/fab/api.py | 7 +++++++ source/fab/fab_base/fab_base.py | 7 ++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/source/fab/api.py b/source/fab/api.py index e447f0b25..a5b005754 100644 --- a/source/fab/api.py +++ b/source/fab/api.py @@ -30,10 +30,12 @@ from fab.tools.category import Category from fab.tools.compiler import Compiler, Ifort from fab.tools.compiler_wrapper import CompilerWrapper +from fab.tools.flags import AlwaysFlags, ContainFlags, FlagList, MatchFlags from fab.tools.linker import Linker from fab.tools.tool import Tool from fab.tools.tool_box import ToolBox from fab.tools.tool_repository import ToolRepository +from fab.tools.shell import Shell from fab.util import common_arg_parser from fab.util import file_checksum, log_or_dot, TimerLogger from fab.util import get_fab_workspace @@ -41,6 +43,7 @@ __all__ = [ "AddFlags", + "AlwaysFlags", "analyse", "archive_objects", "ArtefactSet", @@ -53,10 +56,12 @@ "CompilerWrapper", "compile_c", "compile_fortran", + "ContainFlags", "c_pragma_injector", "Exclude", "fcm_export", "file_checksum", + "FlagList", "get_fab_workspace", "git_checkout", "grab_folder", @@ -69,12 +74,14 @@ "link_exe", "link_shared_object", "log_or_dot", + "MatchFlags", "preprocess_c", "preprocess_fortran", "preprocess_x90", "psyclone", "root_inc_files", "run_mp", + "Shell", "step", "SuffixFilter", "TimerLogger", diff --git a/source/fab/fab_base/fab_base.py b/source/fab/fab_base/fab_base.py index df4fd5202..0e4c5a18d 100755 --- a/source/fab/fab_base/fab_base.py +++ b/source/fab/fab_base/fab_base.py @@ -673,17 +673,15 @@ def compile_c_step( all C files. Optionally, common flags, path-specific flags and alternative source can also be passed to Fab for compilation. """ - site_path_flags: List[AddFlags] = [] if not common_flags: common_flags = [] - assert isinstance(common_flags, list) if not path_flags: path_flags = [] compile_c(self.config, common_flags=(common_flags + self.c_compiler_flags_commandline), - path_flags=path_flags + site_path_flags) + path_flags=path_flags) def compile_fortran_step( self, @@ -699,7 +697,6 @@ def compile_fortran_step( :param path_flags: optional list of path-specific flags to be passed to Fab compile_fortran, default is None. """ - site_path_flags: List[AddFlags] = [] if not common_flags: common_flags = [] if not path_flags: @@ -707,7 +704,7 @@ def compile_fortran_step( compile_fortran(self.config, common_flags=(common_flags + self.fortran_compiler_flags_commandline), - path_flags=path_flags + site_path_flags) + path_flags=path_flags) def link_step(self) -> None: """ From c544a62ce10873ea32b1b9fc48bbdfc827a7c11e Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 28 Jan 2026 16:16:47 +1100 Subject: [PATCH 29/30] #313 Handle missing config file better. --- source/fab/tools/flags.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index 5fe82e2e8..9dd7590e4 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -28,9 +28,8 @@ string. The difference to MatchFlags is that ContainFlags do not need the full path to be specified. -Flags: +FlagList: Manages a list of flags, each of which is an instance of an AbstractFlag. - #TODO: Rename to FlagsList ProfileFlags: Manages a set of flags for specific profiles, including inheritance. @@ -145,8 +144,13 @@ def replace_template(string_list: List[str], if config: params['source'] = config.source_root params['output'] = config.build_output + else: + params['source'] = "/" + params['output'] = "/" if file_path: params['relative'] = file_path.parent + else: + params['relative'] = "." # Use templating to render any relative paths in our flags return [Template(i).substitute(params) for i in string_list] @@ -217,8 +221,8 @@ class MatchFlags(AlwaysFlags): templated expressions `$relative`, `$source`, and `$output` in the pattern as well. - :param flags: a string or list of strings with command line flags. :param pattern: the wildcard pattern which is used when matching. + :param flags: a string or list of strings with command line flags. """ def __init__(self, pattern: str, @@ -292,8 +296,8 @@ class FlagList(List[AbstractFlags]): list with some additional functionality. :param list_of_flags: List of parameters to initialise this object with. - :param path_flags: List of old-style PathFlags, which will be converted - to the new MatchFlags + :param add_flags: List of old-style AddFlags, which will be converted + to the new MatchFlags. ''' def __init__( @@ -357,8 +361,8 @@ def add_flags(self, new_flags: Union[AbstractFlags, str, List[str]]) -> None: '''Adds the specified flags to the list of flags. - :param new_flags: A single string or list of strings which are the - flags to be added. + :param new_flags: New flags to be added. Can be either an class + derived from AbstractFlags, a single string or list of strings. ''' if isinstance(new_flags, AbstractFlags): @@ -403,7 +407,8 @@ def __init__(self: "ProfileFlags", flags: Optional[Union[AbstractFlags, str, List[str]]] = None, profile: str = "") -> None: # Stores the flags for each profile mode. The key is the (lower case) - # name of the profile mode, and it contains a list of flags + # name of the profile mode, and it contains a list of flags. + # Initialise the dict with the default (empty) profile self._profiles: Dict[str, FlagList] = {"": FlagList()} # This dictionary stores an optional inheritance, where one mode From 222f7954c5bb2dc1f648396ec463a7fc98ca696f Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 28 Jan 2026 16:29:08 +1100 Subject: [PATCH 30/30] #313 Fixed mypy errors. --- source/fab/tools/flags.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/fab/tools/flags.py b/source/fab/tools/flags.py index 9dd7590e4..5ec01d9fb 100644 --- a/source/fab/tools/flags.py +++ b/source/fab/tools/flags.py @@ -145,12 +145,12 @@ def replace_template(string_list: List[str], params['source'] = config.source_root params['output'] = config.build_output else: - params['source'] = "/" - params['output'] = "/" + params['source'] = Path("/") + params['output'] = Path("/") if file_path: params['relative'] = file_path.parent else: - params['relative'] = "." + params['relative'] = Path(".") # Use templating to render any relative paths in our flags return [Template(i).substitute(params) for i in string_list]