From 3adffd37723d838ddc9a1adfecb0a94b5f82d290 Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Wed, 5 Feb 2025 15:38:38 +1100 Subject: [PATCH 1/3] More linker wrapper improvements (#383) * Support new and old style of PSyclone command line (no more nemo api etc) * Fix mypy errors. * Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa. * Updated comment. * Removed mixing, use a simple regex instead. * Added support for ifx/icx compiler as intel-llvm class. * Added support for nvidia compiler. * Add preliminary support for Cray compiler. * Added Cray compiler wrapper ftn and cc. * Follow a more consistent naming scheme for crays, even though the native compiler names are longer (crayftn-cray, craycc-cray). * Changed names again. * Renamed cray compiler wrapper to be CrayCcWrapper and CrayFtnWrapper, to avoid confusion with Craycc. * Fixed incorrect name in comments. * Additional compilers (#349) * Moved OBJECT_ARCHIVES from constants to ArtefactSet. * Moved PRAGMAD_C from constants to ArtefactSet. * Turned 'all_source' into an enum. * Allow integer as revision. * Fixed flake8 error. * Removed specific functions to add/get fortran source files etc. * Removed non-existing and unneccessary collections. * Try to fix all run_configs. * Fixed rebase issues. * Added replace functionality to ArtefactStore, updated test_artefacts to cover all lines in that file. * Started to replace artefacts when files are pre-processed. * Removed linker argument from linking step in all examples. * Try to get jules to link. * Fixed build_jules. * Fixed other issues raised in reviews. * Try to get jules to link. * Fixed other issues raised in reviews. * Simplify handling of X90 files by replacing the X90 with x90, meaning only one artefact set is involved when running PSyclone. * Make OBJECT_ARCHIVES also a dict, migrate more code to replace/add files to the default build artefact collections. * Fixed some examples. * Fix flake8 error. * Fixed failing tests. * Support empty comments. * Fix preprocessor to not unnecessary remove and add files that are already in the output directory. * Allow find_soure_files to be called more than once by adding files (not replacing artefact). * Updated lfric_common so that files created by configurator are written in build (not source). * Use c_build_files instead of pragmad_c. * Removed unnecessary str. * Documented the new artefact set handling. * Fixed typo. * Make the PSyclone API configurable. * Fixed formatting of documentation, properly used ArtefactSet names. * Support .f and .F Fortran files. * Removed setter for tool.is_available, which was only used for testing. * #3 Fix documentation and coding style issues from review. * Renamed Categories into Category. * Minor coding style cleanup. * Removed more unnecessary (). * Re-added (invalid) grab_pre_build call. * Fixed typo. * Renamed set_default_vendor to set_default_compiler_suite. * Renamed VendorTool to CompilerSuiteTool. * Also accept a Path as exec_name specification for a tool. * Move the check_available function into the base class. * Fixed some types and documentation. * Fix typing error. * Added explanation for meta-compiler. * Improved error handling and documentation. * Replace mpiifort with mpifort to be a tiny bit more portable. * Use classes to group tests for git/svn/fcm together. * Fixed issue in get_transformation script, and moved script into lfric_common to remove code duplication. * Code improvement as suggested by review. * Fixed run config * Added reference to ticket. * Updated type information. * More typing fixes. * Fixed typing warnings. * As requested by reviewer removed is_working_copy functionality. * Issue a warning (which can be silenced) when a tool in a toolbox is replaced. * Fixed flake8. * Fixed flake8. * Fixed failing test. * Addressed issues raised in review. * Removed now unnecessary operations. * Updated some type information. * Fixed all references to APIs to be consistent with PSyclone 2.5. * Added api to the checksum computation. * Fixed type information. * Added test to verify that changing the api changes the checksum. * Make compiler version a tuple of integers * Update some tests to use tuple versions * Explicitly test handling of bad version format * Fix formatting * Tidying up * Make compiler raise an error for any invalid version string Assume these compilers don't need to be hashed. Saves dealing with empty tuples. * Check compiler version string for compiler name * Fix formatting * Add compiler.get_version_string() method Includes other cleanup from PR comments * Add mpi and openmp settings to BuildConfig, made compiler MPI aware. * Looks like the circular dependency has been fixed. * Revert "Looks like the circular dependency has been fixed." ... while it works with the tests, a real application still triggered it. This reverts commit 150dc379af9df8c38e623fae144a0d5196319f10. * Don't even try to find a C compiler if no C files are to be compiled. * Updated gitignore to ignore (recently renamed) documentation. * Fixed failing test. * Return from compile Fortran early if there are no files to compiles. Fixed coding style. * Add MPI enables wrapper for intel and gnu compiler. * Fixed test. * Automatically add openmp flag to compiler and linker based on BuildConfig. * Removed enforcement of keyword parameters, which is not supported in python 3.7. * Fixed failing test. * Support more than one tool of a given suite by sorting them. * Use different version checkout for each compiler vendor with mixins * Refactoring, remove unittest compiler class * Fix some mypy errors * Use 'Union' type hint to fix build checks * Added option to add flags to a tool. * Introduce proper compiler wrapper, used this to implement properly wrapper MPI compiler. * Fixed typo in types. * Return run_version_command to base Compiler class Provides default version command that can be overridden for other compilers. Also fix some incorrect tests Other tidying * Add a missing type hint * Added (somewhat stupid) 'test' to reach 100% coverage of PSyclone tool. * Simplified MPI support in wrapper. * More compiler wrapper coverage. * Removed duplicated function. * Removed debug print. * Removed permanently changing compiler attributes, which can cause test failures later. * More test for C compiler wrapper. * More work on compiler wrapper tests. * Fixed version and availability handling, added missing tests for 100% coverage. * Fixed typing error. * Try to fix python 3.7. * Tried to fix failing tests. * Remove inheritance from mixins and use protocol * Simplify compiler inheritance Mixins have static methods with unique names, overrides only happen in concrete classes * Updated wrapper and tests to handle error raised in get_version. * Simplified regular expressions (now tests cover detection of version numbers with only a major version). * Test for missing mixin. * Use the parsing mixing from the compiler in a compiler wrapper. * Use setattr instead of assignment to make mypy happy. * Simplify usage of compiler-specific parsing mixins. * Minor code cleanup. * Updated documentation. * Simplify usage of compiler-specific parsing mixins. * Test for missing mixin. * Fixed test. * Added missing openmp_flag property to compiler wrapper. * Don't use isinstance for consistency check, which does not work for CompilerWrappers. * Fixed isinstance test for C compilation which doesn't work with a CompilerWrapper. * Use a linker's compiler to determine MPI support. Removed mpi property from CompilerSuite. * Added more tests for invalid version numbers. * Added more test cases for invalid version number, improved regex to work as expected. * Fixed typo in test. * Fixed flake/mypy errors. * Combine wrapper flags with flags from wrapped compiler. * Made mypy happy. * Fixed test. * Split tests into smaller individual ones, fixed missing asssert in test. * Parameterised compiler version tests to also test wrapper. * Added missing MPI parameter when getting the compiler. * Fixed comments. * Order parameters to be in same order for various compiler classes. * Remove stray character * Added getter for wrapped compiler. * Fixed small error that would prevent nested compiler wrappers from being used. * Added a cast to make mypy happy. * Add simple getter for linker library flags * Add getter for linker flags by library * Fix formatting * Add optional libs argument to link function * Reorder and clean up linker tests * Make sure `Linker.link()` raises for unknown lib * Add missing type * Fix typing error * Add 'libs' argument to link_exe function * Try to add documentation for the linker libs feature * Use correct list type in link_exe hint * Add silent replace option to linker.add_lib_flags * Fixed spelling mistake in option. * Clarified documentation. * Removed unnecessary functions in CompilerWrapper. * Fixed failing test triggered by executing them in specific order (tools then steps) * Fixed line lengths. * Add tests for linker LDFLAG * Add pre- and post- lib flags to link function * Fix syntax in built-in lib flags * Remove netcdf as a built-in linker library Bash-style substitution is not currently handled * Configure pre- and post-lib flags on the Linker object Previously they were passed into the Linker.link() function * Use more realistic linker lib flags * Formatting fix * Removed mixing, use a simple regex instead. * Added support for ifx/icx compiler as intel-llvm class. * Added support for nvidia compiler. * Add preliminary support for Cray compiler. * Added Cray compiler wrapper ftn and cc. * Made mpi and openmp default to False in the BuildConfig constructor. * Removed white space. * Follow a more consistent naming scheme for crays, even though the native compiler names are longer (crayftn-cray, craycc-cray). * Changed names again. * Support compilers that do not support OpenMP. * Added documentation for openmp parameter. * Renamed cray compiler wrapper to be CrayCcWrapper and CrayFtnWrapper, to avoid confusion with Craycc. * Fixed incorrect name in comments. --------- Co-authored-by: jasonjunweilyu <161689601+jasonjunweilyu@users.noreply.github.com> Co-authored-by: Luke Hoffmann Co-authored-by: Luke Hoffmann <992315+lukehoffmann@users.noreply.github.com> * Support new and old style of PSyclone command line (no more nemo api etc) * Fix mypy errors. * Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa. * Added shell tool. * Try to make mypy happy. * Removed debug code. * ToolRepository now only returns default that are available. Updated tests to make tools as available. * Fixed typos and coding style. * Support new and old style of PSyclone command line (no more nemo api etc) * Fix mypy errors. * Added missing tests for calling psyclone, and converting old style to new stle arguments and vice versa. * Updated comment. * Fixed failing tests. * Updated fparser dependency to version 0.2. * Replace old code for handling sentinels with triggering this behaviour in fparser. Require config in constructor of Analyser classes. * Fixed tests for latest changes. * Removed invalid openmp continuation line - since now fparser fails when trying to parse this line. * Added test for disabled openmp parsing. Updated test to work with new test file. * Coding style changes. * Fix flake issues. * Fixed double _. * Make Linker inherit CompilerWrapper * Fix up tests for new Linker inheritence * Fix a flake error * Use linker wrapping to combine flags from the wrapped linker with the linker wrapper. * Minor code cleanup. * Created linker wrapper in ToolRepository. * Try making linker a CompilerSuiteTool instead of a CompilerWrapper. * Updated tests. * Fix support for post-libs. * Fixed mypy. * Removed more accesses to private members. * Added missing type hint. * Make flake8 happy. * Added missing openmp handling in linker. * Addressed issues raised in review. * Forgot that file in previous commit. * Updated linker to always require a compiler (previously it was actually never checked if the wrapped compiler actually exists, it only checked the linker). * Remove unreliable test, since there is no guarantee that a C linker is returned, see issue #379. * Fixed bug that a wrapper would not report openmp status based on the compiler. * Added more description for this test. --------- Co-authored-by: Luke Hoffmann <992315+lukehoffmann@users.noreply.github.com> Co-authored-by: jasonjunweilyu <161689601+jasonjunweilyu@users.noreply.github.com> Co-authored-by: Luke Hoffmann --- source/fab/tools/compiler.py | 4 +- source/fab/tools/linker.py | 88 +++++-------------- source/fab/tools/tool_repository.py | 16 ++-- .../unit_tests/tools/test_compiler_wrapper.py | 12 +++ tests/unit_tests/tools/test_linker.py | 23 ++--- .../unit_tests/tools/test_tool_repository.py | 6 +- 6 files changed, 53 insertions(+), 96 deletions(-) diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index b937e9d1..8e04d011 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -76,7 +76,9 @@ def mpi(self) -> bool: def openmp(self) -> bool: ''':returns: if the compiler supports openmp or not ''' - return self._openmp_flag != "" + # It is important not to use `_openmp_flag` directly, since a compiler + # wrapper overwrites `openmp_flag`. + return self.openmp_flag != "" @property def openmp_flag(self) -> str: diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 2acef01b..63a3dd2b 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -11,7 +11,7 @@ import os from pathlib import Path -from typing import cast, Dict, List, Optional, Union +from typing import Dict, List, Optional, Union import warnings from fab.tools.category import Category @@ -20,11 +20,15 @@ class Linker(CompilerSuiteTool): - '''This is the base class for any Linker. It takes either another linker - instance, or a compiler instance as parameter in the constructor. Exactly - one of these must be provided. - - :param compiler: an optional compiler instance + '''This is the base class for any Linker. It takes an existing compiler + instance as parameter, and optional another linker. The latter is used + to get linker settings - for example, linker-mpif90-gfortran will use + mpif90-gfortran as compiler (i.e. to test if it is available and get + compilation flags), and linker-gfortran as linker. This way a user + only has to specify linker flags in the most basic class (gfortran), + all other linker wrapper will inherit the settings. + + :param compiler: a compiler instance :param linker: an optional linker instance :param name: name of the linker @@ -32,35 +36,19 @@ class Linker(CompilerSuiteTool): :raises RuntimeError: if neither compiler nor linker is specified. ''' - def __init__(self, compiler: Optional[Compiler] = None, + def __init__(self, compiler: Compiler, linker: Optional[Linker] = None, - exec_name: Optional[str] = None, name: Optional[str] = None): - if linker and compiler: - raise RuntimeError("Both compiler and linker is specified in " - "linker constructor.") - if not linker and not compiler: - raise RuntimeError("Neither compiler nor linker is specified in " - "linker constructor.") self._compiler = compiler self._linker = linker - search_linker = self - while search_linker._linker: - search_linker = search_linker._linker - final_compiler = search_linker._compiler if not name: - assert final_compiler # make mypy happy - name = f"linker-{final_compiler.name}" - - if not exec_name: - # This will search for the name in linker or compiler - exec_name = self.get_exec_name() + name = f"linker-{compiler.name}" super().__init__( name=name, - exec_name=exec_name, + exec_name=compiler.exec_name, suite=self.suite, category=Category.LINKER) @@ -76,51 +64,31 @@ def check_available(self) -> bool: ''':returns: whether this linker is available by asking the wrapped linker or compiler. ''' - if self._compiler: - return self._compiler.check_available() - assert self._linker # make mypy happy - return self._linker.check_available() - - def get_exec_name(self) -> str: - ''':returns: the name of the executable by asking the wrapped - linker or compiler.''' - if self._compiler: - return self._compiler.exec_name - assert self._linker # make mypy happy - return self._linker.exec_name + return self._compiler.check_available() @property def suite(self) -> str: ''':returns: the suite this linker belongs to by getting it from - the wrapper compiler or linker.''' - return cast(CompilerSuiteTool, (self._compiler or self._linker)).suite + the wrapped compiler.''' + return self._compiler.suite @property def mpi(self) -> bool: ''':returns" whether this linker supports MPI or not by checking - with the wrapper compiler or linker.''' - if self._compiler: - return self._compiler.mpi - assert self._linker # make mypy happy - return self._linker.mpi + with the wrapped compiler.''' + return self._compiler.mpi @property def openmp(self) -> bool: - ''':returns" whether this linker supports OpenMP or not by checking - with the wrapper compiler or linker.''' - if self._compiler: - return self._compiler.openmp - assert self._linker # make mypy happy - return self._linker.openmp + ''':returns: whether this linker supports OpenMP or not by checking + with the wrapped compiler.''' + return self._compiler.openmp @property def output_flag(self) -> str: ''':returns: the flag that is used to specify the output name. ''' - if self._compiler: - return self._compiler.output_flag - assert self._linker # make mypy happy - return self._linker.output_flag + return self._compiler.output_flag def get_lib_flags(self, lib: str) -> List[str]: '''Gets the standard flags for a standard library @@ -238,18 +206,10 @@ def link(self, input_files: List[Path], output_file: Path, params: List[Union[str, Path]] = [] - # Find the compiler by following the (potentially - # layered) linker wrapper. - linker = self - while linker._linker: - linker = linker._linker - # Now we must have a compiler - compiler = linker._compiler - assert compiler # make mypy happy - params.extend(compiler.flags) + params.extend(self._compiler.flags) if openmp: - params.append(compiler.openmp_flag) + params.append(self._compiler.openmp_flag) # TODO: why are the .o files sorted? That shouldn't matter params.extend(sorted(map(str, input_files))) diff --git a/source/fab/tools/tool_repository.py b/source/fab/tools/tool_repository.py index 1bf839f8..a9749757 100644 --- a/source/fab/tools/tool_repository.py +++ b/source/fab/tools/tool_repository.py @@ -117,19 +117,19 @@ def add_tool(self, tool: Tool): compiler = cast(Compiler, tool) if isinstance(compiler, CompilerWrapper): # If we have a compiler wrapper, create a new linker using - # the linker based on the wrappped compiler. For example, when + # the linker based on the wrapped compiler. For example, when # creating linker-mpif90-gfortran, we want this to be based on - # linker-gfortran (and not on the compiler mpif90-gfortran), - # since the linker-gfortran might have library definitions - # that should be reused. So we first get the existing linker - # (since the compiler exists, a linker for this compiler was - # already created and must exist). + # linker-gfortran. The compiler mpif90-gfortran will be the + # wrapper compiler. Reason is that e.g. linker-gfortran might + # have library definitions that should be reused. So we first + # get the existing linker (since the compiler exists, a linker + # for this compiler was already created and must exist). other_linker = self.get_tool( category=Category.LINKER, name=f"linker-{compiler.compiler.name}") other_linker = cast(Linker, other_linker) - linker = Linker(linker=other_linker, - exec_name=compiler.exec_name, + linker = Linker(compiler, + linker=other_linker, name=f"linker-{compiler.name}") self[linker.category].append(linker) else: diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index 07f9a08b..11096f0c 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -257,6 +257,18 @@ def test_compiler_wrapper_flags_independent(): assert mpicc.flags == ["-a", "-b"] assert mpicc.openmp_flag == gcc.openmp_flag + # Test a compiler wrapper correctly queries the wrapper compiler for + # openmp flag: Set the wrapper to have no _openmp_flag (which is + # actually the default, since the wrapper never sets its own flag), but + # gcc does have a flag, so mpicc should report that is supports openmp. + # mpicc.openmp calls openmp of its base class (Compiler), which queries + # if an openmp flag is defined. This query must go to the openmp property, + # since the wrapper overwrites this property to return the wrapped + # compiler's flag (and not the wrapper's flag, which would not be defined) + with mock.patch.object(mpicc, "_openmp_flag", ""): + assert mpicc._openmp_flag == "" + assert mpicc.openmp + # Adding flags to the wrapper should not affect the wrapped compiler: mpicc.add_flags(["-d", "-e"]) assert gcc.flags == ["-a", "-b"] diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index 052af88d..cd2d8dc9 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -41,28 +41,15 @@ def test_linker(mock_c_compiler, mock_fortran_compiler): assert linker.flags == [] -def test_linker_constructor_error(mock_c_compiler): - '''Test the linker constructor with invalid parameters.''' - - with pytest.raises(RuntimeError) as err: - Linker() - assert ("Neither compiler nor linker is specified in linker constructor." - in str(err.value)) - with pytest.raises(RuntimeError) as err: - Linker(compiler=mock_c_compiler, linker=mock_c_compiler) - assert ("Both compiler and linker is specified in linker constructor." - in str(err.value)) - - @pytest.mark.parametrize("mpi", [True, False]) def test_linker_mpi(mock_c_compiler, mpi): '''Test that linker wrappers handle MPI as expected.''' mock_c_compiler._mpi = mpi - linker = Linker(compiler=mock_c_compiler) + linker = Linker(mock_c_compiler) assert linker.mpi == mpi - wrapped_linker = Linker(linker=linker) + wrapped_linker = Linker(mock_c_compiler, linker=linker) assert wrapped_linker.mpi == mpi @@ -80,7 +67,7 @@ def test_linker_openmp(mock_c_compiler, openmp): linker = Linker(compiler=mock_c_compiler) assert linker.openmp == openmp - wrapped_linker = Linker(linker=linker) + wrapped_linker = Linker(mock_c_compiler, linker=linker) assert wrapped_linker.openmp == openmp @@ -103,7 +90,7 @@ def test_linker_check_available(mock_c_compiler): # Then test the usage of a linker wrapper. The linker will call the # corresponding function in the wrapper linker: - wrapped_linker = Linker(linker=linker) + wrapped_linker = Linker(mock_c_compiler, linker=linker) with mock.patch('fab.tools.compiler.Compiler.get_version', return_value=(1, 2, 3)): assert wrapped_linker.check_available() @@ -342,7 +329,7 @@ def test_linker_nesting(mock_c_compiler): linker1.add_lib_flags("lib_a", ["a_from_1"]) linker1.add_lib_flags("lib_c", ["c_from_1"]) linker1.add_post_lib_flags(["post_lib1"]) - linker2 = Linker(linker=linker1) + linker2 = Linker(mock_c_compiler, linker=linker1) linker2.add_pre_lib_flags(["pre_lib2"]) linker2.add_lib_flags("lib_b", ["b_from_2"]) linker2.add_lib_flags("lib_c", ["c_from_2"]) diff --git a/tests/unit_tests/tools/test_tool_repository.py b/tests/unit_tests/tools/test_tool_repository.py index 0c7d77e5..012487d4 100644 --- a/tests/unit_tests/tools/test_tool_repository.py +++ b/tests/unit_tests/tools/test_tool_repository.py @@ -11,7 +11,7 @@ import pytest from fab.tools import (Ar, Category, FortranCompiler, Gcc, Gfortran, Ifort, - Linker, ToolRepository) + ToolRepository) def test_tool_repository_get_singleton_new(): @@ -62,10 +62,6 @@ def test_tool_repository_get_default(): openmp=False) assert isinstance(gfortran, Gfortran) - gcc_linker = tr.get_default(Category.LINKER, mpi=False, openmp=False) - assert isinstance(gcc_linker, Linker) - assert gcc_linker.name == "linker-gcc" - gcc = tr.get_default(Category.C_COMPILER, mpi=False, openmp=False) assert isinstance(gcc, Gcc) From 34469e40c8ba304a8998db5b8c8e720466294417 Mon Sep 17 00:00:00 2001 From: jasonjunweilyu <161689601+jasonjunweilyu@users.noreply.github.com> Date: Fri, 7 Feb 2025 23:10:20 +1000 Subject: [PATCH 2/3] Removed fparser stop concatenation workround after fparser update (#386) * Removed fparser stop concatenation workround after fparser update * Removed redundant imports for code check --------- Co-authored-by: Junwei Lyu --- run_configs/lfric/atm.py | 6 +----- run_configs/lfric/gungho.py | 5 +---- run_configs/lfric/lfric_common.py | 32 ------------------------------- run_configs/lfric/mesh_tools.py | 4 +--- 4 files changed, 3 insertions(+), 44 deletions(-) diff --git a/run_configs/lfric/atm.py b/run_configs/lfric/atm.py index 3f93c588..92997bbf 100755 --- a/run_configs/lfric/atm.py +++ b/run_configs/lfric/atm.py @@ -20,8 +20,7 @@ from fab.tools import ToolBox from grab_lfric import lfric_source_config, gpl_utils_source_config -from lfric_common import (API, configurator, fparser_workaround_stop_concatenation, - get_transformation_script) +from lfric_common import (API, configurator, get_transformation_script) logger = logging.getLogger('fab') @@ -250,9 +249,6 @@ def file_filtering(config): api=API, ) - # todo: do we need this one in here? - fparser_workaround_stop_concatenation(state) - analyse( state, root_symbol='lfric_atm', diff --git a/run_configs/lfric/gungho.py b/run_configs/lfric/gungho.py index 7f075c10..011afa89 100755 --- a/run_configs/lfric/gungho.py +++ b/run_configs/lfric/gungho.py @@ -22,8 +22,7 @@ from fab.tools import ToolBox from grab_lfric import lfric_source_config, gpl_utils_source_config -from lfric_common import (API, configurator, fparser_workaround_stop_concatenation, - get_transformation_script) +from lfric_common import (API, configurator, get_transformation_script) logger = logging.getLogger('fab') @@ -75,8 +74,6 @@ api=API, ) - fparser_workaround_stop_concatenation(state) - analyse( state, root_symbol='gungho_model', diff --git a/run_configs/lfric/lfric_common.py b/run_configs/lfric/lfric_common.py index fe8eae03..cbf1ba09 100644 --- a/run_configs/lfric/lfric_common.py +++ b/run_configs/lfric/lfric_common.py @@ -1,10 +1,8 @@ import logging import os -import shutil from typing import Optional from pathlib import Path -from fab.artefacts import ArtefactSet from fab.build_config import BuildConfig from fab.steps import step from fab.steps.find_source_files import find_source_files @@ -84,36 +82,6 @@ def configurator(config, lfric_source: Path, gpl_utils_source: Path, rose_meta_c find_source_files(config, source_root=config_dir) -# ============================================================================ -@step -def fparser_workaround_stop_concatenation(config): - """ - fparser can't handle string concat in a stop statement. This step is - a workaround. - - https://github.com/stfc/fparser/issues/330 - - """ - feign_path = None - for file_path in config.artefact_store[ArtefactSet.FORTRAN_BUILD_FILES]: - if file_path.name == 'feign_config_mod.f90': - feign_path = file_path - break - else: - raise RuntimeError("Could not find 'feign_config_mod.f90'.") - - # rename "broken" version - broken_version = feign_path.with_suffix('.broken') - shutil.move(feign_path, broken_version) - - # make fixed version - bad = "_config: '// &\n 'Unable to close temporary file'" - good = "_config: Unable to close temporary file'" - - open(feign_path, 'wt').write( - open(broken_version, 'rt').read().replace(bad, good)) - - # ============================================================================ def get_transformation_script(fpath: Path, config: BuildConfig) -> Optional[Path]: diff --git a/run_configs/lfric/mesh_tools.py b/run_configs/lfric/mesh_tools.py index fde5b793..5d87c961 100755 --- a/run_configs/lfric/mesh_tools.py +++ b/run_configs/lfric/mesh_tools.py @@ -13,7 +13,7 @@ from fab.steps.psyclone import psyclone, preprocess_x90 from fab.tools import ToolBox -from lfric_common import API, configurator, fparser_workaround_stop_concatenation +from lfric_common import API, configurator from grab_lfric import lfric_source_config, gpl_utils_source_config @@ -60,8 +60,6 @@ api=API, ) - fparser_workaround_stop_concatenation(state) - analyse( state, root_symbol=['cubedsphere_mesh_generator', 'planar_mesh_generator', 'summarise_ugrid'], From 0829610f6119f7bf1111d400e54732ff12081eab Mon Sep 17 00:00:00 2001 From: Joerg Henrichs Date: Thu, 27 Feb 2025 13:50:15 +1100 Subject: [PATCH 3/3] Bring develop up to main (#389) * Fixed failing tests. * Improve support for MPI wrappers (#352) * Updated documentation. * Addressed issues raised in review. * Added more info about removing lib info. * Update clang binding module. (#376) * Removed remove_lib_flags function since there seems to be no real use case for it. * Remove support for '-' in nvidia version numbers (which causes problems with compiler wrapper which do not support this). * Fix regex for Cray compiler. * More improvements for Cray version regex. * Cleaner handling of linking external libraries (#374) * Removed likely a merge artefact. * Try to fix pytest failures on github. --------- Co-authored-by: Matthew Hambley --- Documentation/source/advanced_config.rst | 65 +++++++++++++++++-- source/fab/steps/compile_c.py | 2 + source/fab/steps/compile_fortran.py | 4 ++ source/fab/steps/link.py | 6 +- source/fab/tools/compiler.py | 40 ++---------- source/fab/tools/compiler_wrapper.py | 3 + source/fab/tools/linker.py | 10 --- tests/unit_tests/steps/test_link.py | 10 +-- tests/unit_tests/tools/test_compiler.py | 4 +- .../unit_tests/tools/test_compiler_wrapper.py | 22 +++---- tests/unit_tests/tools/test_linker.py | 14 ---- 11 files changed, 96 insertions(+), 84 deletions(-) diff --git a/Documentation/source/advanced_config.rst b/Documentation/source/advanced_config.rst index d2b54ebf..ec19008f 100644 --- a/Documentation/source/advanced_config.rst +++ b/Documentation/source/advanced_config.rst @@ -186,20 +186,75 @@ Linker flags ------------ Probably the most common instance of the need to pass additional arguments is -to specify 3rd party libraries at the link stage. +to specify 3rd party libraries at the link stage. The linker tool allow +for the definition of library-specific flags: for each library, the user can +specify the required linker flags for this library. In the linking step, +only the name of the libraries to be linked is then required. The linker +object will then use the required linking flags. Typically, a site-specific +setup set (see for example https://github.com/MetOffice/lfric-baf) will +specify the right flags for each site, and the application itself only +needs to list the name of the libraries. This way, the application-specific +Fab script is independent from any site-specific settings. Still, an +application-specific script can also overwrite any site-specific setting, +for example if a newer version of a dependency is to be evaluated. + +The settings for a library are defined as follows: .. code-block:: :linenos: - link_exe(state, flags=['-lm', '-lnetcdf']) + tr = ToolRepository() + linker = tr.get_tool(Category.LINKER, "linker-ifort") -Linkers will be pre-configured with flags for common libraries. Where possible, -a library name should be used to include the required flags for linking. + linker.add_lib_flags("yaxt", ["-L/some_path", "-lyaxt", "-lyaxt_c"]) + linker.add_lib_flags("xios", ["-lxios"]) + +This will define two libraries called ``yaxt`` and ``xios``. In the link step, +the application only needs to specify the name of the libraries required, e.g.: + +.. code-block:: + :linenos: + + link_exe(state, libs=["yaxt", "xios"]) + +The linker will then use the specified options. + +A linker object also allows to define options that should always be added, +either as options before any library details, or at the very end. For example: + +.. code-block:: + :linenos: + + linker.add_pre_lib_flags(["-L/my/common/library/path"]) + linker.add_post_lib_flags(["-lstdc++"]) + +The pre_lib_flags can be used to specify library paths that contain +several libraries only once, and this makes it easy to evaluate a different +set of libraries. Additionally, this can also be used to add common +linking options, e.g. Cray's ``-Ktrap=fp``. + +The post_lib_flags can be used for additional common libraries that need +to be linked in. For example, if the application contains a dependency to +C++ but it is using the Fortran compiler for linking, then the C++ libraries +need to be explicitly added. But if there are several libraries depending +on it, you would have to specify this several times (forcing the linker to +re-read the library several times). Instead, you can just add it to the +post flags once. + +The linker step itself can also take optional flags: .. code-block:: :linenos: - link_exe(state, libs=['netcdf']) + link_exe(state, flags=['-Ktrap=fp']) + +These flags will be added to the very end of the the linker options, +i.e. after any other library or post-lib flag. Note that the example above is +not actually recommended to use, since the specified flag is only +valid for certain linker, and a Fab application script should in general +not hard-code flags for a specific linker. Adding the flag to the linker +instance itself, as shown further above, is the better approach. + Path-specific flags ------------------- diff --git a/source/fab/steps/compile_c.py b/source/fab/steps/compile_c.py index 283d9607..c7d014d7 100644 --- a/source/fab/steps/compile_c.py +++ b/source/fab/steps/compile_c.py @@ -127,6 +127,8 @@ def _compile_file(arg: Tuple[AnalysedC, MpCommonArgs]): if compiler.category != Category.C_COMPILER: raise RuntimeError(f"Unexpected tool '{compiler.name}' of category " f"'{compiler.category}' instead of CCompiler") + # Tool box returns a Tool, in order to make mypy happy, we need + # 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, diff --git a/source/fab/steps/compile_fortran.py b/source/fab/steps/compile_fortran.py index fe6f479b..e5767fae 100644 --- a/source/fab/steps/compile_fortran.py +++ b/source/fab/steps/compile_fortran.py @@ -136,6 +136,8 @@ def handle_compiler_args(config: BuildConfig, common_flags=None, if compiler.category != Category.FORTRAN_COMPILER: raise RuntimeError(f"Unexpected tool '{compiler.name}' of category " 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) logger.info( f'Fortran compiler is {compiler} {compiler.get_version_string()}') @@ -268,6 +270,8 @@ def process_file(arg: Tuple[AnalysedFortran, MpCommonArgs]) \ raise RuntimeError(f"Unexpected tool '{compiler.name}' of " 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) flags = Flags(mp_common_args.flags.flags_for_path( path=analysed_file.fpath, config=config)) diff --git a/source/fab/steps/link.py b/source/fab/steps/link.py index e67a8cce..902defd9 100644 --- a/source/fab/steps/link.py +++ b/source/fab/steps/link.py @@ -9,7 +9,7 @@ """ import logging from string import Template -from typing import List, Optional, Union +from typing import List, Optional from fab.artefacts import ArtefactSet from fab.steps import step @@ -34,8 +34,8 @@ def __call__(self, artefact_store): @step def link_exe(config, - libs: Union[List[str], None] = None, - flags: Union[List[str], None] = None, + libs: Optional[List[str]] = None, + flags: Optional[List[str]] = None, source: Optional[ArtefactsGetter] = None): """ Link object files into an executable for every build target. diff --git a/source/fab/tools/compiler.py b/source/fab/tools/compiler.py index 8e04d011..7448411b 100644 --- a/source/fab/tools/compiler.py +++ b/source/fab/tools/compiler.py @@ -69,7 +69,7 @@ def __init__(self, name: str, @property def mpi(self) -> bool: - '''Returns whether this compiler supports MPI or not.''' + ''':returns: whether this compiler supports MPI or not.''' return self._mpi @property @@ -82,7 +82,7 @@ def openmp(self) -> bool: @property def openmp_flag(self) -> str: - '''Returns the flag to enable OpenMP.''' + ''':returns: the flag to enable OpenMP.''' return self._openmp_flag @property @@ -474,21 +474,7 @@ class Nvc(CCompiler): def __init__(self, name: str = "nvc", exec_name: str = "nvc"): super().__init__(name, exec_name, suite="nvidia", openmp_flag="-mp", - version_regex=r"nvc (\d[\d\.-]+\d)") - - def run_version_command( - self, version_command: Optional[str] = '--version') -> str: - '''Run the compiler's command to get its version. This implementation - runs the function in the base class, and changes any '-' into a - '.' to support nvidia version numbers which have dashes, e.g. 23.5-0. - - :param version_command: The compiler argument used to get version info. - - :returns: The output from the version command, with any '-' replaced - with '.' - ''' - version_string = super().run_version_command() - return version_string.replace("-", ".") + version_regex=r"nvc (\d[\d\.]+\d)") # ============================================================================ @@ -506,21 +492,7 @@ def __init__(self, name: str = "nvfortran", exec_name: str = "nvfortran"): module_folder_flag="-module", openmp_flag="-mp", syntax_only_flag="-Msyntax-only", - version_regex=r"nvfortran (\d[\d\.-]+\d)") - - def run_version_command( - self, version_command: Optional[str] = '--version') -> str: - '''Run the compiler's command to get its version. This implementation - runs the function in the base class, and changes any '-' into a - '.' to support nvidia version numbers which have dashes, e.g. 23.5-0. - - :param version_command: The compiler argument used to get version info. - - :returns: The output from the version command, with any '-' replaced - with '.' - ''' - version_string = super().run_version_command() - return version_string.replace("-", ".") + version_regex=r"nvfortran (\d[\d\.]+\d)") # ============================================================================ @@ -545,7 +517,7 @@ class Craycc(CCompiler): def __init__(self, name: str = "craycc-cc", exec_name: str = "cc"): super().__init__(name, exec_name, suite="cray", mpi=True, openmp_flag="-homp", - version_regex=r"Cray [Cc][^\d]* (\d[\d\.]+\d) ") + version_regex=r"Cray [Cc][^\d]* (\d[\d\.]+\d)") # ============================================================================ @@ -564,4 +536,4 @@ def __init__(self, name: str = "crayftn-ftn", exec_name: str = "ftn"): openmp_flag="-homp", syntax_only_flag="-syntax-only", version_regex=(r"Cray Fortran : Version " - r"(\d[\d\.]+\d) ")) + r"(\d[\d\.]+\d)")) diff --git a/source/fab/tools/compiler_wrapper.py b/source/fab/tools/compiler_wrapper.py index 9338f848..ae9089c0 100644 --- a/source/fab/tools/compiler_wrapper.py +++ b/source/fab/tools/compiler_wrapper.py @@ -148,6 +148,9 @@ def compile_file(self, input_file: Path, a syntax check ''' + # TODO #370: replace change_exec_name, and instead provide + # a function that returns the whole command line, which can + # then be modified here. orig_compiler_name = self._compiler.exec_name self._compiler.change_exec_name(self.exec_name) if add_flags is None: diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py index 63a3dd2b..0fb0f61d 100644 --- a/source/fab/tools/linker.py +++ b/source/fab/tools/linker.py @@ -125,16 +125,6 @@ def add_lib_flags(self, lib: str, flags: List[str], # Make a copy to avoid modifying the caller's list self._lib_flags[lib] = flags[:] - def remove_lib_flags(self, lib: str): - '''Remove any flags configured for a standard library - - :param lib: the library name - ''' - try: - del self._lib_flags[lib] - except KeyError: - pass - def add_pre_lib_flags(self, flags: List[str]): '''Add a set of flags to use before any library-specific flags diff --git a/tests/unit_tests/steps/test_link.py b/tests/unit_tests/steps/test_link.py index e9a6750c..8669f4d8 100644 --- a/tests/unit_tests/steps/test_link.py +++ b/tests/unit_tests/steps/test_link.py @@ -14,7 +14,7 @@ from fab.artefacts import ArtefactSet, ArtefactStore from fab.steps.link import link_exe -from fab.tools import FortranCompiler, Linker +from fab.tools import FortranCompiler, Linker, ToolBox import pytest @@ -22,10 +22,11 @@ class TestLinkExe: '''Test class for linking an executable. ''' - def test_run(self, tool_box): + def test_run(self, tool_box: ToolBox): '''Ensure the command is formed correctly, with the flags at the end and that environment variable FFLAGS is picked up. ''' + config = SimpleNamespace( project_workspace=Path('workspace'), artefact_store=ArtefactStore(), @@ -47,14 +48,13 @@ def test_run(self, tool_box): syntax_only_flag=None, compile_flag=None, output_flag=None, openmp_flag=None) - mock_compiler.run = mock.Mock() linker = Linker(compiler=mock_compiler) # Mark the linker as available to it can be added to the tool box linker._is_available = True # Add a custom library to the linker - linker.add_lib_flags('mylib', ['-L/my/lib', '-mylib']) + linker.add_lib_flags('mylib', ['-L/my/lib', '-lmylib']) tool_box.add_tool(linker, silent_replace=True) mock_result = mock.Mock(returncode=0, stdout="abc\ndef".encode()) with mock.patch('fab.tools.tool.subprocess.run', @@ -68,6 +68,6 @@ def test_run(self, tool_box): tool_run.assert_called_with( ['mock_fortran_compiler.exe', '-L/foo1/lib', '-L/foo2/lib', 'bar.o', 'foo.o', - '-L/my/lib', '-mylib', '-fooflag', '-barflag', + '-L/my/lib', '-lmylib', '-fooflag', '-barflag', '-o', 'workspace/foo'], capture_output=True, env=None, cwd=None, check=False) diff --git a/tests/unit_tests/tools/test_compiler.py b/tests/unit_tests/tools/test_compiler.py index 60c18d78..fbbce38f 100644 --- a/tests/unit_tests/tools/test_compiler.py +++ b/tests/unit_tests/tools/test_compiler.py @@ -778,7 +778,7 @@ def test_nvc_get_version_23_5_0(): """) nvc = Nvc() with mock.patch.object(nvc, "run", mock.Mock(return_value=full_output)): - assert nvc.get_version() == (23, 5, 0) + assert nvc.get_version() == (23, 5) def test_nvc_get_version_with_icc_string(): @@ -819,7 +819,7 @@ def test_nvfortran_get_version_23_5_0(): nvfortran = Nvfortran() with mock.patch.object(nvfortran, "run", mock.Mock(return_value=full_output)): - assert nvfortran.get_version() == (23, 5, 0) + assert nvfortran.get_version() == (23, 5) def test_nvfortran_get_version_with_ifort_string(): diff --git a/tests/unit_tests/tools/test_compiler_wrapper.py b/tests/unit_tests/tools/test_compiler_wrapper.py index 11096f0c..78a53249 100644 --- a/tests/unit_tests/tools/test_compiler_wrapper.py +++ b/tests/unit_tests/tools/test_compiler_wrapper.py @@ -7,7 +7,7 @@ '''Tests the compiler wrapper implementation. ''' -from pathlib import Path, PosixPath +from pathlib import Path from unittest import mock import pytest @@ -179,10 +179,10 @@ def test_compiler_wrapper_fortran_with_add_args(): syntax_only=True) # Notice that "-J/b" has been removed mpif90.compiler.run.assert_called_with( - cwd=PosixPath('.'), additional_parameters=['-c', "-O3", - '-fsyntax-only', - '-J', '/module_out', - 'a.f90', '-o', 'a.o']) + cwd=Path('.'), additional_parameters=['-c', "-O3", + '-fsyntax-only', + '-J', '/module_out', + 'a.f90', '-o', 'a.o']) def test_compiler_wrapper_fortran_with_add_args_unnecessary_openmp(): @@ -199,7 +199,7 @@ def test_compiler_wrapper_fortran_with_add_args_unnecessary_openmp(): add_flags=["-fopenmp", "-O3"], openmp=True, syntax_only=True) mpif90.compiler.run.assert_called_with( - cwd=PosixPath('.'), + cwd=Path('.'), additional_parameters=['-c', '-fopenmp', '-fopenmp', '-O3', '-fsyntax-only', '-J', '/module_out', 'a.f90', '-o', 'a.o']) @@ -219,8 +219,8 @@ def test_compiler_wrapper_c_with_add_args(): mpicc.compile_file(Path("a.f90"), "a.o", openmp=False, add_flags=["-O3"]) mpicc.compiler.run.assert_called_with( - cwd=PosixPath('.'), additional_parameters=['-c', "-O3", - 'a.f90', '-o', 'a.o']) + cwd=Path('.'), additional_parameters=['-c', "-O3", 'a.f90', + '-o', 'a.o']) # Invoke C compiler with syntax-only flag (which is only supported # by Fortran compilers), which should raise an exception. with pytest.raises(RuntimeError) as err: @@ -238,7 +238,7 @@ def test_compiler_wrapper_c_with_add_args(): add_flags=["-fopenmp", "-O3"], openmp=True) mpicc.compiler.run.assert_called_with( - cwd=PosixPath('.'), + cwd=Path('.'), additional_parameters=['-c', '-fopenmp', '-fopenmp', '-O3', 'a.f90', '-o', 'a.o']) @@ -293,7 +293,7 @@ def test_compiler_wrapper_flags_with_add_arg(): mpicc.compile_file(Path("a.f90"), "a.o", add_flags=["-f"], openmp=True) mpicc.compiler.run.assert_called_with( - cwd=PosixPath('.'), + cwd=Path('.'), additional_parameters=["-c", "-fopenmp", "-a", "-b", "-d", "-e", "-f", "a.f90", "-o", "a.o"]) @@ -312,7 +312,7 @@ def test_compiler_wrapper_flags_without_add_arg(): # Test if no add_flags are specified: mpicc.compile_file(Path("a.f90"), "a.o", openmp=True) mpicc.compiler.run.assert_called_with( - cwd=PosixPath('.'), + cwd=Path('.'), additional_parameters=["-c", "-fopenmp", "-a", "-b", "-d", "-e", "a.f90", "-o", "a.o"]) diff --git a/tests/unit_tests/tools/test_linker.py b/tests/unit_tests/tools/test_linker.py index cd2d8dc9..18f27bbd 100644 --- a/tests/unit_tests/tools/test_linker.py +++ b/tests/unit_tests/tools/test_linker.py @@ -177,20 +177,6 @@ def test_linker_add_lib_flags_overwrite_silent(mock_linker): assert result == ["-t", "-b"] -def test_linker_remove_lib_flags(mock_linker): - """Linker should provide a way to remove the flags for a library""" - mock_linker.remove_lib_flags("netcdf") - - with pytest.raises(RuntimeError) as err: - mock_linker.get_lib_flags("netcdf") - assert "Unknown library name: 'netcdf'" in str(err.value) - - -def test_linker_remove_lib_flags_unknown(mock_linker): - """Linker should silently allow removing flags for unknown library""" - mock_linker.remove_lib_flags("unknown") - - # ==================== # Linking: # ====================