Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modules remove patch #2216

Merged
merged 10 commits into from
Apr 25, 2023
8 changes: 6 additions & 2 deletions nf_core/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)
Expand Down
10 changes: 10 additions & 0 deletions nf_core/modules/modules_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
77 changes: 77 additions & 0 deletions nf_core/modules/patch.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,80 @@ 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)
patch_path = Path(self.dir, patch_relpath)
module_path = Path(self.dir, module_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_path
)
try:
for file in Path(temp_module_dir).glob("*"):
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}")

log.info(f"Patch for {module} reverted!")
# Remove patch file if we could revert the patch
patch_path.unlink()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would catch possible errors here before removing it from modules.json

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, error handling needs to be added for sure!

# 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_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}' "
)
32 changes: 32 additions & 0 deletions tests/modules/patch.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import os
import tempfile
from pathlib import Path
from unittest import mock

import pytest

Expand Down Expand Up @@ -326,3 +327,34 @@ 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
)

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"}

# 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
1 change: 1 addition & 0 deletions tests/test_modules.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,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,
Expand Down