diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 04815c18..9476d6e8 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -10,10 +10,11 @@ To indicate your agreement, add your details to the the following table. If you are not submitting contributions on behalf of an organisation please use "n/a" for your affiliation. -| GitHub Username | Real Name | Affiliation | -|-----------------|-----------------|-------------| -| MatthewHambley | Matthew Hambley | Met Office | -| yaswant | Yaswant Pradhan | Met Office | +| GitHub Username | Real Name | Affiliation | +|-----------------|-----------------|----------------------------------| +| MatthewHambley | Matthew Hambley | Met Office | +| yaswant | Yaswant Pradhan | Met Office | +| hiker | Joerg Henrichs | Bureau of Meteorology, Australia | --- diff --git a/source/fab/api.py b/source/fab/api.py index 57bf2688..5d31c79f 100644 --- a/source/fab/api.py +++ b/source/fab/api.py @@ -21,6 +21,7 @@ from fab.steps.compile_fortran import compile_fortran from fab.steps.find_source_files import Exclude, find_source_files, Include from fab.steps.grab.fcm import fcm_export +from fab.steps.grab.files import grab_files from fab.steps.grab.folder import grab_folder from fab.steps.grab.git import git_checkout from fab.steps.grab.prebuild import grab_pre_build @@ -67,6 +68,7 @@ "FlagList", "get_fab_workspace", "git_checkout", + "grab_files", "grab_folder", "grab_pre_build", "find_source_files", diff --git a/source/fab/fab_base/fab_base.py b/source/fab/fab_base/fab_base.py index d8799350..70d17c93 100755 --- a/source/fab/fab_base/fab_base.py +++ b/source/fab/fab_base/fab_base.py @@ -28,7 +28,7 @@ from fab.steps.compile_c import compile_c from fab.steps.compile_fortran import compile_fortran from fab.steps.find_source_files import find_source_files, Exclude, Include -from fab.steps.grab.folder import grab_folder +from fab.steps.grab.files import grab_files from fab.steps.link import link_exe, link_shared_object from fab.steps.preprocess import preprocess_c, preprocess_fortran from fab.tools.category import Category @@ -656,7 +656,7 @@ def grab_files_step(self) -> None: if type(self).__name__ == "FabBase": # Do a simple build based on files in "." if FabBase is # started by itself (and not inherited): - grab_folder(self.config, src=".") + grab_files(self.config, src=".") def find_source_files_step( self, diff --git a/source/fab/steps/grab/files.py b/source/fab/steps/grab/files.py new file mode 100644 index 00000000..03ffdbf7 --- /dev/null +++ b/source/fab/steps/grab/files.py @@ -0,0 +1,38 @@ +# ############################################################################## +# (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 grab_files function. +""" + +from pathlib import Path +from typing import Union + +from fab.steps import step +from fab.tools.category import Category + + +@step +def grab_files(config, src: Union[Path, str], dst_label: str = ''): + """ + Copy a source file or folder to the project workspace. + + :param config: + The :class:`fab.build_config.BuildConfig` object where we can read + settings such as the project workspace folder or the multiprocessing + flag. + :param src: + The source location to grab. Either a directory or a file. + :param dst_label: + The name of a sub folder, in the project workspace, in which to put + the source. If not specified, the code is copied into the root of the + source folder. + + """ + dst = config.source_root / dst_label + dst.mkdir(parents=True, exist_ok=True) + rsync = config.tool_box.get_tool(Category.RSYNC) + rsync.execute(src=Path(src), dst=dst) diff --git a/source/fab/steps/grab/folder.py b/source/fab/steps/grab/folder.py index 03bfabc6..ae8553a2 100644 --- a/source/fab/steps/grab/folder.py +++ b/source/fab/steps/grab/folder.py @@ -3,29 +3,33 @@ # For further details please refer to the file COPYRIGHT # which you should have received as part of this distribution # ############################################################################## + +""" +This file contains the grab_folder function. +""" +import logging from pathlib import Path from typing import Union -from fab.steps import step -from fab.tools.category import Category +from fab.steps.grab.files import grab_files + +logger = logging.getLogger(__name__) -@step def grab_folder(config, src: Union[Path, str], dst_label: str = ''): """ - Copy a source folder to the project workspace. + Copy a source folder to the project workspace. This function is + deprecated, use `grab_files` instead. :param config: The :class:`fab.build_config.BuildConfig` object where we can read settings such as the project workspace folder or the multiprocessing flag. :param src: - The source location to grab. The nature of this parameter is depends on the subclass. + The source directory or file to grab. :param dst_label: The name of a sub folder, in the project workspace, in which to put the source. If not specified, the code is copied into the root of the source folder. """ - _dst = config.source_root / dst_label - _dst.mkdir(parents=True, exist_ok=True) - rsync = config.tool_box.get_tool(Category.RSYNC) - rsync.execute(src=src, dst=_dst) + logger.warning("Using deprecated `grab_folder`. Use `grab_files` instead.") + grab_files(config, src, dst_label) diff --git a/source/fab/tools/rsync.py b/source/fab/tools/rsync.py index 160d7ee5..a2c12be3 100644 --- a/source/fab/tools/rsync.py +++ b/source/fab/tools/rsync.py @@ -7,7 +7,6 @@ """This file contains the Rsync class for synchronising file trees. """ -import os from pathlib import Path from typing import Union @@ -25,18 +24,22 @@ def __init__(self): super().__init__("rsync", "rsync", Category.RSYNC) def execute(self, src: Path, - dst: Path): + dst: Path) -> str: '''Execute an rsync command from src to dst. It supports ~ expansion for src, and makes sure that `src` end with a `/` - so that rsync does not create a sub-directory. + if src is a directory so that rsync does not create a sub-directory. :param src: the input path. :param dst: destination path. ''' - src_str = os.path.expanduser(str(src)) - if not src_str.endswith('/'): - src_str += '/' - + src_abs = src.expanduser().resolve() + if src_abs.is_dir(): + # Ensure that a directory name ends with a '/' + src_str = f"{src_abs}/" + else: + src_str = str(src_abs) + + # Note that run will change Path to str internally parameters: list[Union[str, Path]] = [ '--times', '--links', '--stats', '-ru', src_str, dst] return self.run(additional_parameters=parameters) diff --git a/tests/unit_tests/fab_base/test_fab_base.py b/tests/unit_tests/fab_base/test_fab_base.py index 160d8c93..7d36f333 100644 --- a/tests/unit_tests/fab_base/test_fab_base.py +++ b/tests/unit_tests/fab_base/test_fab_base.py @@ -390,7 +390,7 @@ def test_build_binary(monkeypatch) -> None: # We need to patch a lot of Fab functions (to avoid dependencies # on the runtime environment): mocks = {} - for function_name in ["grab_folder", "find_source_files", + for function_name in ["grab_files", "find_source_files", "preprocess_c", "preprocess_fortran", "compile_fortran", "compile_c", "analyse"]: patcher = mock.patch(f"fab.fab_base.fab_base.{function_name}") @@ -400,8 +400,8 @@ def test_build_binary(monkeypatch) -> None: fab_base.build() assert "No target objects defined, linking aborted" in str(err.value) - mocks["grab_folder"][0].stop() - mocks["grab_folder"][1].assert_called_once_with( + mocks["grab_files"][0].stop() + mocks["grab_files"][1].assert_called_once_with( fab_base.config, src=".") mocks["find_source_files"][0].stop() @@ -443,7 +443,7 @@ def test_build_static_lib(monkeypatch) -> None: # We need to patch a lot of Fab functions (to avoid dependencies # on the runtime environment): mocks = {} - for function_name in ["grab_folder", "find_source_files", "preprocess_c", + for function_name in ["grab_files", "find_source_files", "preprocess_c", "preprocess_fortran", "compile_fortran", "compile_c", "analyse", "archive_objects"]: patcher = mock.patch(f"fab.fab_base.fab_base.{function_name}") @@ -451,8 +451,8 @@ def test_build_static_lib(monkeypatch) -> None: fab_base.build() - mocks["grab_folder"][0].stop() - mocks["grab_folder"][1].assert_called_once_with( + mocks["grab_files"][0].stop() + mocks["grab_files"][1].assert_called_once_with( fab_base.config, src=".") mocks["find_source_files"][0].stop() @@ -495,7 +495,7 @@ def test_build_shared_lib(monkeypatch) -> None: # We need to patch a lot of Fab functions (to avoid dependencies # on the runtime environment): mocks = {} - for function_name in ["grab_folder", "find_source_files", "preprocess_c", + for function_name in ["grab_files", "find_source_files", "preprocess_c", "preprocess_fortran", "compile_fortran", "compile_c", "analyse", "link_shared_object"]: patcher = mock.patch(f"fab.fab_base.fab_base.{function_name}") @@ -503,8 +503,8 @@ def test_build_shared_lib(monkeypatch) -> None: fab_base.build() - mocks["grab_folder"][0].stop() - mocks["grab_folder"][1].assert_called_once_with( + mocks["grab_files"][0].stop() + mocks["grab_files"][1].assert_called_once_with( fab_base.config, src=".") mocks["find_source_files"][0].stop() diff --git a/tests/unit_tests/parse/fortran/test_contained_subroutine.py b/tests/unit_tests/parse/fortran/test_contained_subroutine.py index 6c79844d..c8e187d5 100644 --- a/tests/unit_tests/parse/fortran/test_contained_subroutine.py +++ b/tests/unit_tests/parse/fortran/test_contained_subroutine.py @@ -16,7 +16,7 @@ from fab.steps.analyse import analyse from fab.steps.compile_fortran import compile_fortran from fab.steps.find_source_files import find_source_files -from fab.steps.grab.folder import grab_folder +from fab.steps.grab.files import grab_files from fab.steps.link import link_exe from fab.tools.category import Category from fab.tools.tool_box import ToolBox @@ -47,7 +47,7 @@ def test_contained_subroutine(tmp_path): with BuildConfig(fab_workspace=tmp_path, tool_box=tb, project_label='contained_subroutine', multiprocessing=False) as config: - grab_folder(config, PROJECT_SOURCE) + grab_files(config, PROJECT_SOURCE) find_source_files(config) analyse(config, root_symbols='main') build_tree = config.artefact_store[ArtefactSet.BUILD_TREES]["main"] diff --git a/tests/unit_tests/steps/test_grab.py b/tests/unit_tests/steps/test_grab.py index 6d10b8cb..e5b1fba4 100644 --- a/tests/unit_tests/steps/test_grab.py +++ b/tests/unit_tests/steps/test_grab.py @@ -6,6 +6,7 @@ """ Validate methods to obtain source. """ +import logging from pathlib import Path from pyfakefs.fake_filesystem import FakeFilesystem @@ -14,12 +15,13 @@ from fab.build_config import BuildConfig from fab.steps.grab.fcm import fcm_export +from fab.steps.grab.files import grab_files from fab.steps.grab.folder import grab_folder from fab.tools.tool_box import ToolBox from fab.tools.tool_repository import ToolRepository -class TestGrabFolder: +class TestGrabFiles: """ Tests file directory grabbing. """ @@ -30,15 +32,49 @@ class TestGrabFolder: ['/grab/source', '/grab/source/'] ] ) - def test_source_path(self, + def test_grab_files(self, + source: str, + expected: str, + stub_tool_repository: ToolRepository, + fs: FakeFilesystem, + fake_process: FakeProcess) -> None: + """ + Tests file directory grabbery. + """ + fs.create_dir("/grab/source") + version_command = ['rsync', '--version'] + fake_process.register(version_command, stdout='1.2.3') + grab_command = ['rsync', '--times', '--links', '--stats', + '-ru', expected, '/fab/project/source/bar'] + fake_process.register(grab_command) + + config = BuildConfig('project', ToolBox(), + mpi=False, openmp=False, multiprocessing=False, + fab_workspace=Path('/fab')) + + with warns(UserWarning, + match="_metric_send_conn not set, cannot send metrics"): + grab_files(config, src=source, dst_label='bar') + assert fake_process.call_count(grab_command) == 1 + + @mark.parametrize( + ['source', 'expected'], + [ + ['/grab/source/', '/grab/source/'], + ['/grab/source', '/grab/source/'] + ] + ) + def test_grab_folder(self, source: str, expected: str, stub_tool_repository: ToolRepository, fs: FakeFilesystem, - fake_process: FakeProcess) -> None: + fake_process: FakeProcess, + caplog) -> None: """ Tests file directory grabbery. """ + fs.create_dir("/grab/source") version_command = ['rsync', '--version'] fake_process.register(version_command, stdout='1.2.3') grab_command = ['rsync', '--times', '--links', '--stats', @@ -51,7 +87,10 @@ def test_source_path(self, with warns(UserWarning, match="_metric_send_conn not set, cannot send metrics"): - grab_folder(config, src=source, dst_label='bar') + with caplog.at_level(logging.WARNING): + grab_folder(config, src=source, dst_label='bar') + assert ("Using deprecated `grab_folder`. Use `grab_files` instead." + in caplog.text) assert fake_process.call_count(grab_command) == 1 diff --git a/tests/unit_tests/test_api.py b/tests/unit_tests/test_api.py index 384dc180..d214653b 100644 --- a/tests/unit_tests/test_api.py +++ b/tests/unit_tests/test_api.py @@ -38,6 +38,8 @@ def test_import_from_api() -> None: "file_checksum", "get_fab_workspace", "git_checkout", + "grab_files", + "grab_folder", "grab_folder", "grab_pre_build", "find_source_files", diff --git a/tests/unit_tests/tools/test_rsync.py b/tests/unit_tests/tools/test_rsync.py index bc2bd4ec..3815464b 100644 --- a/tests/unit_tests/tools/test_rsync.py +++ b/tests/unit_tests/tools/test_rsync.py @@ -45,23 +45,36 @@ def test_check_available(fake_process: FakeProcess) -> None: ] -def test_rsync_create(fake_process: FakeProcess) -> None: +def test_rsync_create(fake_process: FakeProcess, + change_into_tmpdir: Path) -> None: """ Tests performing a sync. Ensure source always ends with a '/'. """ - with_command = ['rsync', '--times', '--links', '--stats', '-ru', '/src/', '/dst'] - fake_process.register(with_command) - without_command = ['rsync', '--times', '--links', '--stats', '-ru', '/src/', '/dst'] - fake_process.register(without_command) + tmp_dir = change_into_tmpdir + # Create a directory" + directory = tmp_dir / "directory" + directory.mkdir() + file = tmp_dir / "file" + file.write_text("A file\n") rsync = Rsync() - # Test 1: src with / - rsync.execute(src=Path("/src/"), dst=Path("/dst")) + # Test 1: Directory must have a '/' at the end: + dir_command = ['rsync', '--times', '--links', '--stats', '-ru', + f'{directory}/', '/dst'] + fake_process.register(dir_command) + rsync.execute(src=directory, dst=Path("/dst")) - # Test 2: src without / - rsync.execute(src=Path("/src"), dst=Path("/dst")) + # Test 2: a file should not have a '/' at the end. First ensure + # that file does indeed not have a '/' at the end (Path should discard + # trailing / ... but just in case:) + assert str(file)[-1] != "/" + file_command = ['rsync', '--times', '--links', '--stats', '-ru', + f'{file}', '/dst'] + fake_process.register(file_command) + + rsync.execute(src=file, dst=Path("/dst")) assert call_list(fake_process) == [ - with_command, without_command + dir_command, file_command ]