From 46595026dfd95ef3fe5df8c2dec7717835e4c311 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Tue, 28 Mar 2023 11:36:31 +0100 Subject: [PATCH 1/5] Initial commit of one method for patch removal --- nf_core/__main__.py | 8 +++- nf_core/modules/modules_json.py | 10 +++++ nf_core/modules/patch.py | 68 +++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/nf_core/__main__.py b/nf_core/__main__.py index 841cb9f7e7..0b29f1769c 100644 --- a/nf_core/__main__.py +++ b/nf_core/__main__.py @@ -618,7 +618,8 @@ def update(ctx, tool, dir, force, prompt, sha, all, preview, save_diff, update_d default=".", help=r"Pipeline directory. [dim]\[default: current working directory][/]", ) -def patch(ctx, tool, dir): +@click.option("-r", "--remove", is_flag=True, default=False) +def patch(ctx, tool, dir, remove): """ Create a patch file for minor changes in a module @@ -632,7 +633,10 @@ def patch(ctx, tool, dir): ctx.obj["modules_repo_branch"], ctx.obj["modules_repo_no_pull"], ) - module_patch.patch(tool) + if remove: + module_patch.remove(tool) + else: + module_patch.patch(tool) except (UserWarning, LookupError) as e: log.error(e) sys.exit(1) diff --git a/nf_core/modules/modules_json.py b/nf_core/modules/modules_json.py index 431ad1d657..13e6d4e32b 100644 --- a/nf_core/modules/modules_json.py +++ b/nf_core/modules/modules_json.py @@ -745,6 +745,16 @@ def add_patch_entry(self, module_name, repo_url, install_dir, patch_filename, wr if write_file: self.dump() + def remove_patch_entry(self, module_name, repo_url, install_dir, write_file=True): + if self.modules_json is None: + self.load() + try: + del self.modules_json["repos"][repo_url]["modules"][install_dir][module_name]["patch"] + except KeyError: + log.warning("No patch entry in 'modules.json' to remove") + if write_file: + self.dump() + def get_patch_fn(self, module_name, repo_url, install_dir): """ Get the patch filename of a module diff --git a/nf_core/modules/patch.py b/nf_core/modules/patch.py index fa51640c06..8b8068bafb 100644 --- a/nf_core/modules/patch.py +++ b/nf_core/modules/patch.py @@ -130,3 +130,71 @@ def patch(self, module=None): # Finally move the created patch file to its final location shutil.move(patch_temp_path, patch_path) log.info(f"Patch file of '{module_fullname}' written to '{patch_path}'") + + def remove(self, module): + # Check modules directory structure + self.check_modules_structure() + + self.modules_json.check_up_to_date() + self.param_check(module) + modules = self.modules_json.get_all_components(self.component_type)[self.modules_repo.remote_url] + + if module is None: + choices = [ + module if directory == self.modules_repo.repo_path else f"{directory}/{module}" + for directory, module in modules + ] + module = questionary.autocomplete( + "Tool:", + choices, + style=nf_core.utils.nfcore_question_style, + ).unsafe_ask() + module_dir = [dir for dir, m in modules if m == module][0] + module_fullname = str(Path("modules", module_dir, module)) + + # Verify that the module has an entry in the modules.json file + if not self.modules_json.module_present(module, self.modules_repo.remote_url, module_dir): + raise UserWarning( + f"The '{module_fullname}' module does not have an entry in the 'modules.json' file. Cannot compute patch" + ) + + module_version = self.modules_json.get_module_version(module, self.modules_repo.remote_url, module_dir) + if module_version is None: + raise UserWarning( + f"The '{module_fullname}' module does not have a valid version in the 'modules.json' file. Cannot compute patch" + ) + # Get the module branch and reset it in the ModulesRepo object + module_branch = self.modules_json.get_component_branch( + self.component_type, module, self.modules_repo.remote_url, module_dir + ) + if module_branch != self.modules_repo.branch: + self.modules_repo.setup_branch(module_branch) + + # Set the diff filename based on the module name + patch_filename = f"{module.replace('/', '-')}.diff" + module_relpath = Path("modules", module_dir, module) + patch_relpath = Path(module_relpath, patch_filename) + module_current_dir = Path(self.dir, module_relpath) + patch_path = Path(self.dir, patch_relpath) + + if patch_path.exists(): + remove = questionary.confirm( + f"Patch exists for module '{module_fullname}'. Are you sure you want to remove?", + style=nf_core.utils.nfcore_question_style, + ).unsafe_ask() + if not remove: + return + + # Try to apply the patch in reverse and move resulting files to module dir + temp_module_dir = self.modules_json.try_apply_patch_reverse( + module, self.modules_repo.repo_path, patch_relpath, module_relpath + ) + for file in Path(temp_module_dir).glob("*"): + file.rename(module_relpath.joinpath(file.name)) + os.rmdir(temp_module_dir) + + # Remove patch file + patch_path.unlink() + # Write changes to modules.json + self.modules_json.remove_patch_entry(module, self.modules_repo.remote_url, module_dir) + log.info(f"Patch for {module} reverted!") From 62c87314c4f28eb1ddc5a155593ca6f1442078d4 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Wed, 29 Mar 2023 11:53:00 +0100 Subject: [PATCH 2/5] Extra error handling --- nf_core/modules/patch.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/nf_core/modules/patch.py b/nf_core/modules/patch.py index 8b8068bafb..4dd4ab12e3 100644 --- a/nf_core/modules/patch.py +++ b/nf_core/modules/patch.py @@ -174,7 +174,6 @@ def remove(self, module): patch_filename = f"{module.replace('/', '-')}.diff" module_relpath = Path("modules", module_dir, module) patch_relpath = Path(module_relpath, patch_filename) - module_current_dir = Path(self.dir, module_relpath) patch_path = Path(self.dir, patch_relpath) if patch_path.exists(): @@ -189,9 +188,12 @@ def remove(self, module): temp_module_dir = self.modules_json.try_apply_patch_reverse( module, self.modules_repo.repo_path, patch_relpath, module_relpath ) - for file in Path(temp_module_dir).glob("*"): - file.rename(module_relpath.joinpath(file.name)) - os.rmdir(temp_module_dir) + try: + for file in Path(temp_module_dir).glob("*"): + file.rename(module_relpath.joinpath(file.name)) + os.rmdir(temp_module_dir) + except Exception as err: + raise UserWarning(f"There was a problem reverting the patched file: {err}") # Remove patch file patch_path.unlink() From 800e1d2003d3df8fdbcc014da60058e9c9c4cc5a Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Wed, 29 Mar 2023 12:35:22 +0100 Subject: [PATCH 3/5] Check if the reverted module code matches the known sha --- nf_core/modules/patch.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/nf_core/modules/patch.py b/nf_core/modules/patch.py index 4dd4ab12e3..8c3f0cf382 100644 --- a/nf_core/modules/patch.py +++ b/nf_core/modules/patch.py @@ -195,8 +195,14 @@ def remove(self, module): except Exception as err: raise UserWarning(f"There was a problem reverting the patched file: {err}") - # Remove patch file + log.info(f"Patch for {module} reverted!") + # Remove patch file if we could revert the patch patch_path.unlink() - # Write changes to modules.json + # Write changes to module.json self.modules_json.remove_patch_entry(module, self.modules_repo.remote_url, module_dir) - log.info(f"Patch for {module} reverted!") + + if not all(self.modules_repo.module_files_identical(module, module_relpath, module_version).values()): + log.error( + f"Module files do not appear to match the remote for the commit sha in the 'module.json': {module_version}\n" + f"Recommend reinstalling with 'nf-core modules install --force --sha {module_version} {module}' " + ) From b8fcbd2fe69d7a3f9864a4a1bd394c3433c83f93 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Tue, 4 Apr 2023 13:25:22 +0100 Subject: [PATCH 4/5] Add a test for patch removal --- tests/modules/patch.py | 29 +++++++++++++++++++++++++++++ tests/test_modules.py | 1 + 2 files changed, 30 insertions(+) diff --git a/tests/modules/patch.py b/tests/modules/patch.py index 09b892e2c8..360bb99120 100644 --- a/tests/modules/patch.py +++ b/tests/modules/patch.py @@ -322,3 +322,32 @@ def test_create_patch_update_fail(self): with open(module_path / patch_fn, "r") as fh: new_patch_contents = fh.read() assert patch_contents == new_patch_contents + + +def test_remove_patch(self): + """Test creating a patch when there is no change to the module""" + setup_patch(self.pipeline_dir, True) + + # Try creating a patch file + patch_obj = nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH) + patch_obj.patch(BISMARK_ALIGN) + + module_path = Path(self.pipeline_dir, "modules", REPO_NAME, BISMARK_ALIGN) + + # Check that a patch file with the correct name has been created + patch_fn = f"{'-'.join(BISMARK_ALIGN.split('/'))}.diff" + assert set(os.listdir(module_path)) == {"main.nf", "meta.yml", patch_fn} + + # Check the 'modules.json' contains a patch file for the module + modules_json_obj = nf_core.modules.modules_json.ModulesJson(self.pipeline_dir) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_URL, REPO_NAME) == Path( + "modules", REPO_NAME, BISMARK_ALIGN, patch_fn + ) + + patch_obj.remove(BISMARK_ALIGN) + # Check that the diff file has been removed + assert set(os.listdir(module_path)) == {"main.nf", "meta.yml"} + + # Check that the 'modules.json' entry has been removed + modules_json_obj = nf_core.modules.modules_json.ModulesJson(self.pipeline_dir) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_URL, REPO_NAME) is None diff --git a/tests/test_modules.py b/tests/test_modules.py index 21c003112e..df2ab901ac 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -203,6 +203,7 @@ def test_modulesrepo_class(self): test_create_patch_try_apply_successful, test_create_patch_update_fail, test_create_patch_update_success, + test_remove_patch, ) from .modules.remove import ( test_modules_remove_multiqc_from_gitlab, From 23c4a2f79fb532ed2c10eb3a02b175e743c53247 Mon Sep 17 00:00:00 2001 From: Arthur Gymer <24782660+awgymer@users.noreply.github.com> Date: Tue, 4 Apr 2023 15:43:26 +0100 Subject: [PATCH 5/5] Fix issue with using relpaths instead of paths with the pipeline_dir. Mock confirmation during tests --- nf_core/modules/patch.py | 7 ++++--- tests/modules/patch.py | 5 ++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/nf_core/modules/patch.py b/nf_core/modules/patch.py index 8c3f0cf382..4890345052 100644 --- a/nf_core/modules/patch.py +++ b/nf_core/modules/patch.py @@ -175,6 +175,7 @@ def remove(self, module): module_relpath = Path("modules", module_dir, module) patch_relpath = Path(module_relpath, patch_filename) patch_path = Path(self.dir, patch_relpath) + module_path = Path(self.dir, module_relpath) if patch_path.exists(): remove = questionary.confirm( @@ -186,11 +187,11 @@ def remove(self, module): # Try to apply the patch in reverse and move resulting files to module dir temp_module_dir = self.modules_json.try_apply_patch_reverse( - module, self.modules_repo.repo_path, patch_relpath, module_relpath + module, self.modules_repo.repo_path, patch_relpath, module_path ) try: for file in Path(temp_module_dir).glob("*"): - file.rename(module_relpath.joinpath(file.name)) + file.rename(module_path.joinpath(file.name)) os.rmdir(temp_module_dir) except Exception as err: raise UserWarning(f"There was a problem reverting the patched file: {err}") @@ -201,7 +202,7 @@ def remove(self, module): # Write changes to module.json self.modules_json.remove_patch_entry(module, self.modules_repo.remote_url, module_dir) - if not all(self.modules_repo.module_files_identical(module, module_relpath, module_version).values()): + if not all(self.modules_repo.module_files_identical(module, module_path, module_version).values()): log.error( f"Module files do not appear to match the remote for the commit sha in the 'module.json': {module_version}\n" f"Recommend reinstalling with 'nf-core modules install --force --sha {module_version} {module}' " diff --git a/tests/modules/patch.py b/tests/modules/patch.py index 360bb99120..72a4bf1788 100644 --- a/tests/modules/patch.py +++ b/tests/modules/patch.py @@ -1,6 +1,7 @@ import os import tempfile from pathlib import Path +from unittest import mock import pytest @@ -344,7 +345,9 @@ def test_remove_patch(self): "modules", REPO_NAME, BISMARK_ALIGN, patch_fn ) - patch_obj.remove(BISMARK_ALIGN) + with mock.patch.object(nf_core.create.questionary, "confirm") as mock_questionary: + mock_questionary.unsafe_ask.return_value = True + patch_obj.remove(BISMARK_ALIGN) # Check that the diff file has been removed assert set(os.listdir(module_path)) == {"main.nf", "meta.yml"}