From 5e77dc30f191b0445ea73d64047bd06af9eb349e Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Fri, 29 Jul 2022 11:16:12 +0200 Subject: [PATCH 01/15] Lint for patch validity --- nf_core/modules/lint/patch.py | 128 ++++++++++++++++++++++++++++++ nf_core/modules/modules_differ.py | 5 +- 2 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 nf_core/modules/lint/patch.py diff --git a/nf_core/modules/lint/patch.py b/nf_core/modules/lint/patch.py new file mode 100644 index 0000000000..bf9bb3ca79 --- /dev/null +++ b/nf_core/modules/lint/patch.py @@ -0,0 +1,128 @@ +from pathlib import Path + +from ..modules_differ import ModulesDiffer +from ..nfcore_module import NFCoreModule + + +def patch(module_lint_obj, module: NFCoreModule): + """ + Lint a patch file found in a module + + Checks that the file name is well formed, and that + the patch can be applied in reverse with the correct result. + """ + # Check if there exists a patch file + patch_fn = f"{module.module_name.replace('/', '-')}.diff" + patch_path = Path(module.module_dir, patch_fn) + if not patch_path.exists(): + # Nothing to lint, just return + return + + +def check_patch_valid(module_lint_obj, module: NFCoreModule, patch_path): + with open(patch_path, "r") as fh: + patch_lines = fh.readlines() + + # Check that the file contains a patch for at least one file + # and that the file is in the correct directory + paths_in_patch = [] + passed = True + it = iter(patch_lines) + try: + while True: + line = next(it) + if line.startswith("---"): + frompath = Path(line.split(" ")[1]) + line = next(it) + if not line.startswith("+++"): + module.failed.append( + ( + "patch_valid", + "Patch file invalid. Line starting with '---' should always be follow by line starting with '+++'", + patch_path, + ) + ) + passed = False + topath = Path(line.split(" ")[1]) + if frompath == Path("/dev/null"): + paths_in_patch.append((frompath, ModulesDiffer.DiffEnum.CREATED)) + elif topath == Path("/dev/null"): + paths_in_patch.append((frompath, ModulesDiffer.DiffEnum.REMOVED)) + elif frompath == topath: + paths_in_patch.append((frompath, ModulesDiffer.DiffEnum.CHANGED)) + else: + module.failed.append( + ( + "patch_valid", + "Patch file invaldi. From file '{frompath}' mismatched with to path '{topath}'", + patch_path, + ) + ) + passed = False + # Check that the next line is hunk + line = next(it) + if not line.startswith("@@"): + module.failed.append( + ( + "patch_valid", + "Patch file invalid. File declarations should be followed by hunk", + patch_path, + ) + ) + passed = False + except StopIteration: + pass + + if not passed: + return False + + if len(paths_in_patch) == 0: + module.failed.append(("patch_valid", "Patch file invalid. Found no patches", patch_path)) + return False + + # Go through the files and check that they exist + # Warn about any created or removed files + passed = True + for path, diff_status in paths_in_patch: + if diff_status == ModulesDiffer.DiffEnum.CHANGED: + if not Path(module.base_dir, path).exists(): + module.failed.append( + ( + "patch_valid", + f"Patch file invalid. Path '{path}' does not exist but is reported in patch file.", + patch_path, + ) + ) + passed = False + continue + elif diff_status == ModulesDiffer.DiffEnum.CREATED: + if not Path(module.base_dir, path).exists(): + module.failed.append( + ( + "patch_valid", + f"Patch file invalid. Path '{path}' does not exist but is reported in patch file.", + patch_path, + ) + ) + passed = False + continue + module.warned.append( + ("patch", f"Patch file performs file creation of {path}. This is discouraged."), patch_path + ) + elif diff_status == ModulesDiffer.DiffEnum.REMOVED: + if Path(module.base_dir, path).exists(): + module.failed.append( + ( + "patch_valid", + f"Patch file invalid. Path '{path}' is reported as deleted but exists.", + patch_path, + ) + ) + passed = False + continue + module.warned.append( + ("patch", f"Patch file performs file deletion of {path}. This is discouraged.", patch_path) + ) + if passed: + module.passed(("patch_valid", "Patch file is valid", patch_path)) + return passed diff --git a/nf_core/modules/modules_differ.py b/nf_core/modules/modules_differ.py index 2df04276b8..9c8ff20153 100644 --- a/nf_core/modules/modules_differ.py +++ b/nf_core/modules/modules_differ.py @@ -354,7 +354,7 @@ def get_new_and_old_lines(patch): return old_lines, new_lines @staticmethod - def try_apply_patch(file_path, patch): + def try_apply_patch(file_path, patch, reverse=False): """ Tries to apply a patch to a modified file. Since the line numbers in the patch does not agree if the file is modified, the old and new @@ -365,6 +365,7 @@ def try_apply_patch(file_path, patch): Args: new_fn (str | Path): Path to the modified file patch (str | Path): (Outdated) patch for the file + reverse (bool): Apply the patch in reverse Returns: [str]: The patched lines of the file @@ -374,6 +375,8 @@ def try_apply_patch(file_path, patch): the file. """ org_lines, patch_lines = ModulesDiffer.get_new_and_old_lines(patch) + if reverse: + patch_lines, org_lines = org_lines, patch_lines with open(file_path, "r") as fh: new_lines = fh.readlines() From 90c70c62f2080ca56ec26627da96ebf7392fb46c Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Fri, 29 Jul 2022 11:58:05 +0200 Subject: [PATCH 02/15] Lint for patch reversible --- nf_core/modules/lint/patch.py | 54 ++++++++++++++++++++++++++++++- nf_core/modules/modules_differ.py | 15 ++++----- nf_core/modules/update.py | 4 ++- 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/nf_core/modules/lint/patch.py b/nf_core/modules/lint/patch.py index bf9bb3ca79..8b6f40ca62 100644 --- a/nf_core/modules/lint/patch.py +++ b/nf_core/modules/lint/patch.py @@ -18,8 +18,30 @@ def patch(module_lint_obj, module: NFCoreModule): # Nothing to lint, just return return + if not check_patch_valid(module, patch_path): + # Test failed, just exit + return + + patch_reversible(module, patch_path) + + +def check_patch_valid(module, patch_path): + """ + Checks whether a patch is valid. Looks for lines like + --- + +++ + @@ n,n n,n @@ + and make sure that the come in the right order and that + the reported paths exists. If the patch file performs + file creation or deletion we issue a lint warning. + + Args: + module (NFCoreModule): The module currently being linted + patch_path (Path): The absolute path to the patch file. -def check_patch_valid(module_lint_obj, module: NFCoreModule, patch_path): + Returns: + (bool): False if any test failed, True otherwise + """ with open(patch_path, "r") as fh: patch_lines = fh.readlines() @@ -126,3 +148,33 @@ def check_patch_valid(module_lint_obj, module: NFCoreModule, patch_path): if passed: module.passed(("patch_valid", "Patch file is valid", patch_path)) return passed + + +def patch_reversible(module, patch_path): + """ + Try applying a patch in reverse to see if it is up to date + + Args: + module (NFCoreModule): The module currently being linted + patch_path (Path): The absolute path to the patch file. + + Returns: + (bool): False if any test failed, True otherwise + """ + # Get the patches + patches = ModulesDiffer.per_file_patch(patch_path) + + # Try applying the patches + for file, patch in patches.items(): + try: + file_path = module.base_dir / file + with open(file_path, "r") as fh: + file_lines = fh.readlines() + ModulesDiffer.try_apply_patch(file_lines, patch, reverse=True) + except LookupError as e: + # Patch failed. Save the patch file by moving to the install dir + module.failed.append( + (("patch_reversible", f"Failed to recreate remote version of '{file}' from patch", patch_path)) + ) + return False + module.passed((("patch_reversible", f"Successfully recreated remote file versions", patch_path))) diff --git a/nf_core/modules/modules_differ.py b/nf_core/modules/modules_differ.py index 9c8ff20153..a957c4ec94 100644 --- a/nf_core/modules/modules_differ.py +++ b/nf_core/modules/modules_differ.py @@ -354,7 +354,7 @@ def get_new_and_old_lines(patch): return old_lines, new_lines @staticmethod - def try_apply_patch(file_path, patch, reverse=False): + def try_apply_patch(file_lines, patch, reverse=False): """ Tries to apply a patch to a modified file. Since the line numbers in the patch does not agree if the file is modified, the old and new @@ -378,9 +378,6 @@ def try_apply_patch(file_path, patch, reverse=False): if reverse: patch_lines, org_lines = org_lines, patch_lines - with open(file_path, "r") as fh: - new_lines = fh.readlines() - # The patches are sorted by their order of occurrence in the original # file. Loop through the new file and try to find the new indices of # these lines. We know they are non overlapping, and thus only need to @@ -389,11 +386,11 @@ def try_apply_patch(file_path, patch, reverse=False): patch_indices = [None] * p i = 0 j = 0 - n = len(new_lines) + n = len(file_lines) while i < n and j < p: m = len(org_lines[j]) while i < n: - if org_lines[j] == new_lines[i : i + m]: + if org_lines[j] == file_lines[i : i + m]: patch_indices[j] = (i, i + m) j += 1 break @@ -406,15 +403,15 @@ def try_apply_patch(file_path, patch, reverse=False): # Apply the patch to new lines by substituting # the original lines with the patch lines - patched_new_lines = new_lines[: patch_indices[0][0]] + patched_new_lines = file_lines[: patch_indices[0][0]] for i in range(len(patch_indices) - 1): # Add the patch lines patched_new_lines.extend(patch_lines[i]) # Fill the spaces between the patches - patched_new_lines.extend(new_lines[patch_indices[i][1] : patch_indices[i + 1][0]]) + patched_new_lines.extend(file_lines[patch_indices[i][1] : patch_indices[i + 1][0]]) # Add the remaining part of the new file patched_new_lines.extend(patch_lines[-1]) - patched_new_lines.extend(new_lines[patch_indices[-1][1] :]) + patched_new_lines.extend(file_lines[patch_indices[-1][1] :]) return patched_new_lines diff --git a/nf_core/modules/update.py b/nf_core/modules/update.py index 1d2b06a89c..7bfc800ca6 100644 --- a/nf_core/modules/update.py +++ b/nf_core/modules/update.py @@ -508,7 +508,9 @@ def try_apply_patch(self, module, repo_name, patch_relpath, module_dir, module_i log.debug(f"Applying patch to {Path(module_fullname, file)}") file_relpath = Path(file).relative_to(module_relpath) file_path = temp_module_dir / file_relpath - patched_new_lines = ModulesDiffer.try_apply_patch(file_path, patch) + with open(file_path, "r") as fh: + file_lines = fh.readlines() + patched_new_lines = ModulesDiffer.try_apply_patch(file_lines, patch) new_files[file_relpath] = "".join(patched_new_lines) except LookupError as e: # Patch failed. Save the patch file by moving to the install dir From 1fb719ffaaaae67fade61ab558cf2413f866c99a Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Fri, 29 Jul 2022 14:46:19 +0200 Subject: [PATCH 03/15] Start adding test --- nf_core/modules/patch.py | 4 +- tests/modules/lint.py | 49 +++++++++++++++++++++ tests/modules/patch.py | 94 ++++++++++++++++++++++------------------ tests/test_modules.py | 1 + tests/utils.py | 1 + 5 files changed, 107 insertions(+), 42 deletions(-) diff --git a/nf_core/modules/patch.py b/nf_core/modules/patch.py index aabf95e9d5..d84e58caf8 100644 --- a/nf_core/modules/patch.py +++ b/nf_core/modules/patch.py @@ -50,11 +50,12 @@ def patch(self, module=None): f"The '{module_fullname}' module does not have a valid version in the 'modules.json' file. Cannot compute patch" ) # Set the diff filename based on the module name - patch_filename = f"{'-'.join(module.split('/'))}.diff" + patch_filename = f"{module.replace('/', '-')}.diff" module_relpath = Path("modules", self.modules_repo.fullname, module) patch_relpath = Path(module_relpath, patch_filename) module_dir = Path(self.dir, module_relpath) patch_path = Path(self.dir, patch_relpath) + print(patch_path) if patch_path.exists(): remove = questionary.confirm( @@ -105,4 +106,5 @@ def patch(self, module=None): # Finally move the created patch file to its final location shutil.move(patch_temp_path, patch_path) + print(f"{patch_path} exists? {patch_path.exists()}") log.info(f"Patch file of '{module_fullname}' written to '{patch_path}'") diff --git a/tests/modules/lint.py b/tests/modules/lint.py index 0f60377d5e..d83c63468e 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -1,5 +1,10 @@ +from pathlib import Path + import nf_core.modules +from ..utils import GITLAB_REPO, GITLAB_URL +from .patch import BISMARK_ALIGN, PATCH_BRANCH, setup_patch + def test_modules_lint_trimgalore(self): """Test linting the TrimGalore! module""" @@ -30,3 +35,47 @@ def test_modules_lint_new_modules(self): assert len(module_lint.failed) == 0, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" assert len(module_lint.passed) > 0 assert len(module_lint.warned) >= 0 + + +def test_modules_lint_patch_invalid(self): + """Try linting a module that has a corrupted patch file""" + # Install the trimgalore module and modify it + setup_patch(self.pipeline_dir, True) + + # Create the patch file + nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH).patch(BISMARK_ALIGN) + + module_lint = nf_core.modules.ModuleLint(dir=self.pipeline_dir) + + # Modify the file + patch_path = Path( + self.pipeline_dir, "modules", GITLAB_REPO, BISMARK_ALIGN, f"{BISMARK_ALIGN.replace('/', '-')}.diff" + ) + with open(patch_path, "r") as fh: + org_lines = fh.readlines() + + mod_lines = org_lines.copy() + for i, line in enumerate(mod_lines): + if line.startswith("+++"): + mod_lines.pop(i) + break + with open(patch_path, "w") as fh: + fh.writelines(mod_lines) + + module_lint.lint(print_results=False, module=BISMARK_ALIGN) + assert len(module_lint.failed) == 1, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" + assert len(module_lint.passed) > 0 + assert len(module_lint.warned) >= 0 + + mod_lines = org_lines.copy() + for i, line in enumerate(mod_lines): + if line.startswith("@@"): + mod_lines.pop(i) + break + with open(patch_path, "w") as fh: + fh.writelines(mod_lines) + + module_lint.lint(print_results=False, module=BISMARK_ALIGN) + assert len(module_lint.failed) == 1, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" + assert len(module_lint.passed) > 0 + assert len(module_lint.warned) >= 0 diff --git a/tests/modules/patch.py b/tests/modules/patch.py index dbfc543542..cb87c85d7a 100644 --- a/tests/modules/patch.py +++ b/tests/modules/patch.py @@ -20,7 +20,7 @@ ORG_SHA = "22c7c12dc21e2f633c00862c1291ceda0a3b7066" SUCCEED_SHA = "f7d3a3894f67db2e2f3f8c9ba76f8e33356be8e0" FAIL_SHA = "b4596169055700533865cefb7542108418f53100" -MODULE = "bismark/align" +BISMARK_ALIGN = "bismark/align" REPO_NAME = "nf-core/modules-test" PATCH_BRANCH = "patch-tester" @@ -31,11 +31,11 @@ def setup_patch(pipeline_dir, modify_module): ) # Install the module - install_obj.install(MODULE) + install_obj.install(BISMARK_ALIGN) if modify_module: # Modify the module - module_path = Path(pipeline_dir, "modules", REPO_NAME, MODULE) + module_path = Path(pipeline_dir, "modules", REPO_NAME, BISMARK_ALIGN) modify_main_nf(module_path / "main.nf") @@ -60,16 +60,16 @@ def test_create_patch_no_change(self): # Try creating a patch file patch_obj = nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH) with pytest.raises(UserWarning): - patch_obj.patch(MODULE) + patch_obj.patch(BISMARK_ALIGN) - module_path = Path(self.pipeline_dir, "modules", REPO_NAME, MODULE) + module_path = Path(self.pipeline_dir, "modules", REPO_NAME, BISMARK_ALIGN) # Check that no patch file has been added to the directory assert set(os.listdir(module_path)) == {"main.nf", "meta.yml"} # Check the 'modules.json' contains no patch file for the module modules_json_obj = nf_core.modules.modules_json.ModulesJson(self.pipeline_dir) - assert modules_json_obj.get_patch_fn(MODULE, REPO_NAME) is None + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) is None def test_create_patch_change(self): @@ -78,17 +78,19 @@ def test_create_patch_change(self): # Try creating a patch file patch_obj = nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH) - patch_obj.patch(MODULE) + patch_obj.patch(BISMARK_ALIGN) - module_path = Path(self.pipeline_dir, "modules", REPO_NAME, MODULE) + module_path = Path(self.pipeline_dir, "modules", REPO_NAME, BISMARK_ALIGN) - patch_fn = f"{'-'.join(MODULE.split('/'))}.diff" + patch_fn = f"{'-'.join(BISMARK_ALIGN.split('/'))}.diff" # Check that a patch file with the correct name has been created 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(MODULE, REPO_NAME) == Path("modules", REPO_NAME, MODULE, patch_fn) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) == Path( + "modules", REPO_NAME, BISMARK_ALIGN, patch_fn + ) # Check that the correct lines are in the patch file with open(module_path / patch_fn, "r") as fh: @@ -106,42 +108,46 @@ def test_create_patch_try_apply_successful(self): Test creating a patch file and applying it to a new version of the the files """ setup_patch(self.pipeline_dir, True) - module_relpath = Path("modules", REPO_NAME, MODULE) + module_relpath = Path("modules", REPO_NAME, BISMARK_ALIGN) module_path = Path(self.pipeline_dir, module_relpath) # Try creating a patch file patch_obj = nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH) - patch_obj.patch(MODULE) + patch_obj.patch(BISMARK_ALIGN) - patch_fn = f"{'-'.join(MODULE.split('/'))}.diff" + patch_fn = f"{'-'.join(BISMARK_ALIGN.split('/'))}.diff" # Check that a patch file with the correct name has been created 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(MODULE, REPO_NAME) == Path("modules", REPO_NAME, MODULE, patch_fn) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) == Path( + "modules", REPO_NAME, BISMARK_ALIGN, patch_fn + ) update_obj = nf_core.modules.ModuleUpdate( self.pipeline_dir, sha=SUCCEED_SHA, remote_url=GITLAB_URL, branch=PATCH_BRANCH ) # Install the new files install_dir = Path(tempfile.mkdtemp()) - update_obj.install_module_files(MODULE, SUCCEED_SHA, update_obj.modules_repo, install_dir) + update_obj.install_module_files(BISMARK_ALIGN, SUCCEED_SHA, update_obj.modules_repo, install_dir) # Try applying the patch - module_install_dir = install_dir / MODULE + module_install_dir = install_dir / BISMARK_ALIGN patch_relpath = module_relpath / patch_fn - assert update_obj.try_apply_patch(MODULE, REPO_NAME, patch_relpath, module_path, module_install_dir) is True + assert update_obj.try_apply_patch(BISMARK_ALIGN, REPO_NAME, patch_relpath, module_path, module_install_dir) is True # Move the files from the temporary directory - update_obj.move_files_from_tmp_dir(MODULE, module_path, install_dir, REPO_NAME, SUCCEED_SHA) + update_obj.move_files_from_tmp_dir(BISMARK_ALIGN, module_path, install_dir, REPO_NAME, SUCCEED_SHA) # Check that a patch file with the correct name has been created 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(MODULE, REPO_NAME) == Path("modules", REPO_NAME, MODULE, patch_fn) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) == Path( + "modules", REPO_NAME, BISMARK_ALIGN, patch_fn + ) # Check that the correct lines are in the patch file with open(module_path / patch_fn, "r") as fh: @@ -168,32 +174,34 @@ def test_create_patch_try_apply_failed(self): Test creating a patch file and applying it to a new version of the the files """ setup_patch(self.pipeline_dir, True) - module_relpath = Path("modules", REPO_NAME, MODULE) + module_relpath = Path("modules", REPO_NAME, BISMARK_ALIGN) module_path = Path(self.pipeline_dir, module_relpath) # Try creating a patch file patch_obj = nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH) - patch_obj.patch(MODULE) + patch_obj.patch(BISMARK_ALIGN) - patch_fn = f"{'-'.join(MODULE.split('/'))}.diff" + patch_fn = f"{'-'.join(BISMARK_ALIGN.split('/'))}.diff" # Check that a patch file with the correct name has been created 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(MODULE, REPO_NAME) == Path("modules", REPO_NAME, MODULE, patch_fn) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) == Path( + "modules", REPO_NAME, BISMARK_ALIGN, patch_fn + ) update_obj = nf_core.modules.ModuleUpdate( self.pipeline_dir, sha=FAIL_SHA, remote_url=GITLAB_URL, branch=PATCH_BRANCH ) # Install the new files install_dir = Path(tempfile.mkdtemp()) - update_obj.install_module_files(MODULE, FAIL_SHA, update_obj.modules_repo, install_dir) + update_obj.install_module_files(BISMARK_ALIGN, FAIL_SHA, update_obj.modules_repo, install_dir) # Try applying the patch - module_install_dir = install_dir / MODULE + module_install_dir = install_dir / BISMARK_ALIGN patch_path = module_relpath / patch_fn - assert update_obj.try_apply_patch(MODULE, REPO_NAME, patch_path, module_path, module_install_dir) is False + assert update_obj.try_apply_patch(BISMARK_ALIGN, REPO_NAME, patch_path, module_path, module_install_dir) is False def test_create_patch_update_success(self): @@ -204,34 +212,36 @@ def test_create_patch_update_success(self): but uses higher level api """ setup_patch(self.pipeline_dir, True) - module_path = Path(self.pipeline_dir, "modules", REPO_NAME, MODULE) + module_path = Path(self.pipeline_dir, "modules", REPO_NAME, BISMARK_ALIGN) # Try creating a patch file patch_obj = nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH) - patch_obj.patch(MODULE) + patch_obj.patch(BISMARK_ALIGN) - patch_fn = f"{'-'.join(MODULE.split('/'))}.diff" + patch_fn = f"{'-'.join(BISMARK_ALIGN.split('/'))}.diff" # Check that a patch file with the correct name has been created 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(MODULE, REPO_NAME) == Path("modules", REPO_NAME, MODULE, patch_fn) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) == Path( + "modules", REPO_NAME, BISMARK_ALIGN, patch_fn + ) # Update the module update_obj = nf_core.modules.ModuleUpdate( self.pipeline_dir, sha=SUCCEED_SHA, show_diff=False, remote_url=GITLAB_URL, branch=PATCH_BRANCH ) - update_obj.update(MODULE) + update_obj.update(BISMARK_ALIGN) # Check that a patch file with the correct name has been created 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(MODULE, REPO_NAME) == Path( - "modules", REPO_NAME, MODULE, patch_fn - ), modules_json_obj.get_patch_fn(MODULE, REPO_NAME) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) == Path( + "modules", REPO_NAME, BISMARK_ALIGN, patch_fn + ), modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) # Check that the correct lines are in the patch file with open(module_path / patch_fn, "r") as fh: @@ -258,19 +268,21 @@ def test_create_patch_update_fail(self): Test creating a patch file and updating a module when there is a diff conflict """ setup_patch(self.pipeline_dir, True) - module_path = Path(self.pipeline_dir, "modules", REPO_NAME, MODULE) + module_path = Path(self.pipeline_dir, "modules", REPO_NAME, BISMARK_ALIGN) # Try creating a patch file patch_obj = nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH) - patch_obj.patch(MODULE) + patch_obj.patch(BISMARK_ALIGN) - patch_fn = f"{'-'.join(MODULE.split('/'))}.diff" + patch_fn = f"{'-'.join(BISMARK_ALIGN.split('/'))}.diff" # Check that a patch file with the correct name has been created 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(MODULE, REPO_NAME) == Path("modules", REPO_NAME, MODULE, patch_fn) + assert modules_json_obj.get_patch_fn(BISMARK_ALIGN, REPO_NAME) == Path( + "modules", REPO_NAME, BISMARK_ALIGN, patch_fn + ) # Save the file contents for downstream comparison with open(module_path / patch_fn, "r") as fh: @@ -279,15 +291,15 @@ def test_create_patch_update_fail(self): update_obj = nf_core.modules.ModuleUpdate( self.pipeline_dir, sha=FAIL_SHA, show_diff=False, remote_url=GITLAB_URL, branch=PATCH_BRANCH ) - update_obj.update(MODULE) + update_obj.update(BISMARK_ALIGN) # Check that the installed files have not been affected by the attempted patch temp_dir = Path(tempfile.mkdtemp()) nf_core.modules.modules_command.ModuleCommand(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH).install_module_files( - MODULE, FAIL_SHA, update_obj.modules_repo, temp_dir + BISMARK_ALIGN, FAIL_SHA, update_obj.modules_repo, temp_dir ) - temp_module_dir = temp_dir / MODULE + temp_module_dir = temp_dir / BISMARK_ALIGN for file in os.listdir(temp_module_dir): assert file in os.listdir(module_path) with open(module_path / file, "r") as fh: diff --git a/tests/test_modules.py b/tests/test_modules.py index 786fa4676e..a36ba9781d 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -111,6 +111,7 @@ def test_modulesrepo_class(self): from .modules.lint import ( test_modules_lint_empty, test_modules_lint_new_modules, + test_modules_lint_patch_invalid, test_modules_lint_trimgalore, ) from .modules.list import ( diff --git a/tests/utils.py b/tests/utils.py index 343e5d6a0d..a844d774f8 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -9,6 +9,7 @@ OLD_TRIMGALORE_SHA = "20d8250d9f39ddb05dfb437603aaf99b5c0b2b41" GITLAB_URL = "https://gitlab.com/nf-core/modules-test.git" +GITLAB_REPO = "nf-core/modules-test" def with_temporary_folder(func): From 94603ff05723a28fb7467618cfc3e3e0e84005cb Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Fri, 29 Jul 2022 16:32:36 +0200 Subject: [PATCH 04/15] Move patch application to single function --- nf_core/modules/lint/patch.py | 32 +++++++++++++++---------------- nf_core/modules/modules_differ.py | 32 ++++++++++++++++++++++++++++++- nf_core/modules/update.py | 27 +++++++++----------------- 3 files changed, 55 insertions(+), 36 deletions(-) diff --git a/nf_core/modules/lint/patch.py b/nf_core/modules/lint/patch.py index 8b6f40ca62..d0c09197d5 100644 --- a/nf_core/modules/lint/patch.py +++ b/nf_core/modules/lint/patch.py @@ -161,20 +161,18 @@ def patch_reversible(module, patch_path): Returns: (bool): False if any test failed, True otherwise """ - # Get the patches - patches = ModulesDiffer.per_file_patch(patch_path) - - # Try applying the patches - for file, patch in patches.items(): - try: - file_path = module.base_dir / file - with open(file_path, "r") as fh: - file_lines = fh.readlines() - ModulesDiffer.try_apply_patch(file_lines, patch, reverse=True) - except LookupError as e: - # Patch failed. Save the patch file by moving to the install dir - module.failed.append( - (("patch_reversible", f"Failed to recreate remote version of '{file}' from patch", patch_path)) - ) - return False - module.passed((("patch_reversible", f"Successfully recreated remote file versions", patch_path))) + try: + ModulesDiffer.try_apply_patch( + module.module_name, + "nf-core/modules", + patch_path, + Path(module.module_dir).relative(module.base_dir), + reverse=True, + ) + except LookupError as e: + # Patch failed. Save the patch file by moving to the install dir + module.failed.append((("patch_reversible", "Patch does not agree with module files", patch_path))) + return False + + module.passed((("patch_reversible", "Patch agrees with module files", patch_path))) + return True diff --git a/nf_core/modules/modules_differ.py b/nf_core/modules/modules_differ.py index a957c4ec94..6dce756ba8 100644 --- a/nf_core/modules/modules_differ.py +++ b/nf_core/modules/modules_differ.py @@ -354,7 +354,7 @@ def get_new_and_old_lines(patch): return old_lines, new_lines @staticmethod - def try_apply_patch(file_lines, patch, reverse=False): + def try_apply_single_patch(file_lines, patch, reverse=False): """ Tries to apply a patch to a modified file. Since the line numbers in the patch does not agree if the file is modified, the old and new @@ -415,3 +415,33 @@ def try_apply_patch(file_lines, patch, reverse=False): patched_new_lines.extend(file_lines[patch_indices[-1][1] :]) return patched_new_lines + + @staticmethod + def try_apply_patch(module, repo_name, patch_path, module_dir, reverse=False): + """ + Try applying a full patch file to a module + + Args: + module (str): Name of the module + repo_name (str): Name of the repository where the module resides + patch_path (str): The absolute path to the patch file to be applied + module_dir (Path): The directory containing the module + + Returns: + dict[str, str]: A dictionary with file paths (relative to the pipeline dir) + as keys and the patched file contents as values + + Raises: + LookupError: If the the patch application fails in a file + """ + module_relpath = Path("modules", repo_name, module) + patches = ModulesDiffer.per_file_patch(patch_path) + new_files = {} + for file, patch in patches.items(): + log.debug(f"Applying patch to {file}") + file_relpath = Path(file).relative_to(module_relpath) + file_path = module_dir / file_relpath + with open(file_path, "r") as fh: + file_lines = fh.readlines() + patched_new_lines = ModulesDiffer.try_apply_single_patch(file_lines, patch, reverse=reverse) + new_files[file_relpath] = "".join(patched_new_lines) diff --git a/nf_core/modules/update.py b/nf_core/modules/update.py index 7bfc800ca6..3daa7e81ff 100644 --- a/nf_core/modules/update.py +++ b/nf_core/modules/update.py @@ -501,24 +501,15 @@ def try_apply_patch(self, module, repo_name, patch_relpath, module_dir, module_i temp_module_dir = temp_dir / module shutil.copytree(module_install_dir, temp_module_dir) - patches = ModulesDiffer.per_file_patch(patch_path) - new_files = {} - for file, patch in patches.items(): - try: - log.debug(f"Applying patch to {Path(module_fullname, file)}") - file_relpath = Path(file).relative_to(module_relpath) - file_path = temp_module_dir / file_relpath - with open(file_path, "r") as fh: - file_lines = fh.readlines() - patched_new_lines = ModulesDiffer.try_apply_patch(file_lines, patch) - new_files[file_relpath] = "".join(patched_new_lines) - except LookupError as e: - # Patch failed. Save the patch file by moving to the install dir - shutil.move(patch_path, Path(module_install_dir, patch_path.relative_to(module_dir))) - log.warning( - f"Failed to apply patch for module '{module_fullname}'. You will have to apply the patch manually" - ) - return False + try: + new_files = ModulesDiffer.try_apply_patch(module, repo_name, patch_path, temp_module_dir) + except LookupError as e: + # Patch failed. Save the patch file by moving to the install dir + shutil.move(patch_path, Path(module_install_dir, patch_path.relative_to(module_dir))) + log.warning( + f"Failed to apply patch for module '{module_fullname}'. You will have to apply the patch manually" + ) + return False # Write the patched files to a temporary directory log.debug("Writing patched files") From 595f932523f0872010075bd97c202791828df4b7 Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Fri, 29 Jul 2022 16:45:08 +0200 Subject: [PATCH 05/15] Move patch checking to NFCoreModule --- nf_core/modules/lint/patch.py | 12 +++++------- nf_core/modules/nfcore_module.py | 15 +++++++++++++-- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/nf_core/modules/lint/patch.py b/nf_core/modules/lint/patch.py index d0c09197d5..9331d461ec 100644 --- a/nf_core/modules/lint/patch.py +++ b/nf_core/modules/lint/patch.py @@ -11,18 +11,16 @@ def patch(module_lint_obj, module: NFCoreModule): Checks that the file name is well formed, and that the patch can be applied in reverse with the correct result. """ - # Check if there exists a patch file - patch_fn = f"{module.module_name.replace('/', '-')}.diff" - patch_path = Path(module.module_dir, patch_fn) - if not patch_path.exists(): - # Nothing to lint, just return + # Check if the module is patched + if not module.is_patched: + # Nothing to do return - if not check_patch_valid(module, patch_path): + if not check_patch_valid(module, module.patch_path): # Test failed, just exit return - patch_reversible(module, patch_path) + patch_reversible(module, module.patch_path) def check_patch_valid(module, patch_path): diff --git a/nf_core/modules/nfcore_module.py b/nf_core/modules/nfcore_module.py index f828142fda..5ec871929c 100644 --- a/nf_core/modules/nfcore_module.py +++ b/nf_core/modules/nfcore_module.py @@ -2,6 +2,7 @@ The NFCoreModule class holds information and utility functions for a single module """ import os +from pathlib import Path class NFCoreModule(object): @@ -21,11 +22,13 @@ def __init__(self, module_dir, repo_type, base_dir, nf_core_module=True): self.outputs = [] self.has_meta = False self.git_sha = None + self.is_patched = False + self.is_patched = None if nf_core_module: # Initialize the important files - self.main_nf = os.path.join(self.module_dir, "main.nf") - self.meta_yml = os.path.join(self.module_dir, "meta.yml") + self.main_nf = str(Path(self.module_dir, "main.nf")) + self.meta_yml = str(Path(self.module_dir, "meta.yml")) if self.repo_type == "pipeline": self.module_name = module_dir.split("nf-core/modules" + os.sep)[1] else: @@ -37,3 +40,11 @@ def __init__(self, module_dir, repo_type, base_dir, nf_core_module=True): self.test_dir = os.path.join(self.base_dir, "tests", "modules", self.module_name) self.test_yml = os.path.join(self.test_dir, "test.yml") self.test_main_nf = os.path.join(self.test_dir, "main.nf") + + # Check if we have a patch file + if self.repo_type == "pipeline": + patch_fn = f"{self.module_name.replace('/', '-')}.diff" + patch_path = Path(self.module_dir, patch_fn) + if patch_path.exists(): + self.is_patched = True + self.patch_path = patch_path From cf544cf153000bbfa1bdd95cd7b391dea870a582 Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 09:23:41 +0200 Subject: [PATCH 06/15] Apply diff backwards in module changes --- nf_core/modules/lint/module_changes.py | 24 +++++++++++++++++++++++- nf_core/modules/modules_differ.py | 3 ++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/nf_core/modules/lint/module_changes.py b/nf_core/modules/lint/module_changes.py index 3be9d0a638..6c05902811 100644 --- a/nf_core/modules/lint/module_changes.py +++ b/nf_core/modules/lint/module_changes.py @@ -2,6 +2,11 @@ Check whether the content of a module has changed compared to the original repository """ import os +import shutil +import tempfile +from pathlib import Path + +from nf_core.modules.modules_differ import ModulesDiffer def module_changes(module_lint_object, module): @@ -17,8 +22,25 @@ def module_changes(module_lint_object, module): Only runs when linting a pipeline, not the modules repository """ + if module.is_patched: + # If the module is patched, we need to apply + # the patch in reverse before comparing with the remote + tempdir = Path(tempfile.mkdtemp()) + shutil.copytree(module.module_dir, tempdir, dirs_exist_ok=True) + try: + new_lines = ModulesDiffer.try_apply_patch( + module.module_name, "nf-core/modules", module.patch_path, tempdir, reverse=True + ) + for file, lines in new_lines.items(): + with open(tempdir / file, "w") as fh: + fh.writelines(lines) + except LookupError: + return + else: + tempdir = module.module_dir + for f, same in module_lint_object.modules_repo.module_files_identical( - module.module_name, module.module_dir, module.git_sha + module.module_name, tempdir, module.git_sha ).items(): if same: module.passed.append( diff --git a/nf_core/modules/modules_differ.py b/nf_core/modules/modules_differ.py index 6dce756ba8..07dbb50833 100644 --- a/nf_core/modules/modules_differ.py +++ b/nf_core/modules/modules_differ.py @@ -444,4 +444,5 @@ def try_apply_patch(module, repo_name, patch_path, module_dir, reverse=False): with open(file_path, "r") as fh: file_lines = fh.readlines() patched_new_lines = ModulesDiffer.try_apply_single_patch(file_lines, patch, reverse=reverse) - new_files[file_relpath] = "".join(patched_new_lines) + new_files[file_relpath] = patched_new_lines + return new_files From d467787edf0ca8da6c56e7e10a5a95f01fdcd0dc Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 09:36:58 +0200 Subject: [PATCH 07/15] Rename file and fix issue with get_all_lint_test --- docs/api/make_lint_md.py | 6 +++- nf_core/lint/__init__.py | 6 ++-- nf_core/modules/lint/__init__.py | 28 +++++++++++-------- nf_core/modules/lint/module_changes.py | 11 ++++++-- .../lint/{patch.py => module_patch.py} | 2 +- 5 files changed, 36 insertions(+), 17 deletions(-) rename nf_core/modules/lint/{patch.py => module_patch.py} (99%) diff --git a/docs/api/make_lint_md.py b/docs/api/make_lint_md.py index c3362b7f6e..e0265c707d 100644 --- a/docs/api/make_lint_md.py +++ b/docs/api/make_lint_md.py @@ -44,7 +44,11 @@ def make_docs(docs_basedir, lint_tests, md_template): modules_docs_basedir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "_src", "module_lint_tests") make_docs( modules_docs_basedir, - nf_core.modules.lint.ModuleLint._get_all_lint_tests(), + list( + set(nf_core.modules.lint.ModuleLint.get_all_lint_tests(is_pipeline=True)).union( + nf_core.modules.lint.ModuleLint.get_all_lint_tests(is_pipeline=False) + ) + ), """# {0} ```{{eval-rst}} diff --git a/nf_core/lint/__init__.py b/nf_core/lint/__init__.py index 27619ad73f..4fc13c2f21 100644 --- a/nf_core/lint/__init__.py +++ b/nf_core/lint/__init__.py @@ -55,7 +55,7 @@ def run_linting( # Verify that the requested tests exist if key: all_tests = set(PipelineLint._get_all_lint_tests(release_mode)).union( - set(nf_core.modules.lint.ModuleLint._get_all_lint_tests()) + set(nf_core.modules.lint.ModuleLint.get_all_lint_tests(is_pipeline=True)) ) bad_keys = [k for k in key if k not in all_tests] if len(bad_keys) > 0: @@ -91,7 +91,9 @@ def run_linting( # Run only the tests we want if key: # Select only the module lint tests - module_lint_tests = list(set(key).intersection(set(nf_core.modules.lint.ModuleLint._get_all_lint_tests()))) + module_lint_tests = list( + set(key).intersection(set(nf_core.modules.lint.ModuleLint.get_all_lint_tests(is_pipeline=True))) + ) else: # If no key is supplied, run the default modules tests module_lint_tests = ("module_changes", "module_version") diff --git a/nf_core/modules/lint/__init__.py b/nf_core/modules/lint/__init__.py index a4664ee05a..26ba8940ec 100644 --- a/nf_core/modules/lint/__init__.py +++ b/nf_core/modules/lint/__init__.py @@ -9,31 +9,23 @@ from __future__ import print_function -import json import logging import operator import os -import re -import sys import questionary -import requests import rich -import yaml from rich.markdown import Markdown -from rich.panel import Panel from rich.table import Table import nf_core.modules.module_utils import nf_core.utils -from nf_core.lint.pipeline_todos import pipeline_todos from nf_core.lint_utils import console from nf_core.modules.modules_command import ModuleCommand from nf_core.modules.modules_json import ModulesJson from nf_core.modules.modules_repo import ModulesRepo from nf_core.modules.nfcore_module import NFCoreModule from nf_core.utils import plural_s as _s -from nf_core.utils import rich_force_colors log = logging.getLogger(__name__) @@ -66,6 +58,7 @@ class ModuleLint(ModuleCommand): from .meta_yml import meta_yml from .module_changes import module_changes from .module_deprecations import module_deprecations + from .module_patch import module_patch from .module_tests import module_tests from .module_todos import module_todos from .module_version import module_version @@ -82,7 +75,7 @@ def __init__(self, dir, fail_warned=False, remote_url=None, branch=None, no_pull self.warned = [] self.failed = [] self.modules_repo = ModulesRepo(remote_url, branch, no_pull, base_path) - self.lint_tests = self._get_all_lint_tests() + self.lint_tests = self.get_all_lint_tests() # Get lists of modules install in directory self.all_local_modules, self.all_nfcore_modules = self.get_installed_modules() @@ -96,12 +89,25 @@ def __init__(self, dir, fail_warned=False, remote_url=None, branch=None, no_pull if self.repo_type == "pipeline": # Add as first test to load git_sha before module_changes self.lint_tests.insert(0, "module_version") + # Add as the second test to verify the patch file before module_changes + self.lint_test.insert(1) # Only check if modules have been changed in pipelines self.lint_tests.append("module_changes") @staticmethod - def _get_all_lint_tests(): - return ["main_nf", "meta_yml", "module_todos", "module_deprecations"] + def get_all_lint_tests(is_pipeline=True): + if is_pipeline: + return [ + "module_patch", + "module_version", + "main_nf", + "meta_yml", + "module_todos", + "module_deprecations", + "module_changes", + ] + else: + return ["main_nf", "meta_yml", "module_todos", "module_deprecations", "module_tests"] def lint( self, diff --git a/nf_core/modules/lint/module_changes.py b/nf_core/modules/lint/module_changes.py index 6c05902811..c0e1ada3ff 100644 --- a/nf_core/modules/lint/module_changes.py +++ b/nf_core/modules/lint/module_changes.py @@ -35,6 +35,13 @@ def module_changes(module_lint_object, module): with open(tempdir / file, "w") as fh: fh.writelines(lines) except LookupError: + module.failed.append( + ( + "check_local_copy", + "Local copy of module out of sync with diff file", + f"{module.module_dir}", + ) + ) return else: tempdir = module.module_dir @@ -47,7 +54,7 @@ def module_changes(module_lint_object, module): ( "check_local_copy", "Local copy of module up to date", - f"{os.path.join(module.module_dir, f)}", + f"{Path(module.module_dir, f)}", ) ) else: @@ -55,6 +62,6 @@ def module_changes(module_lint_object, module): ( "check_local_copy", "Local copy of module does not match remote", - f"{os.path.join(module.module_dir, f)}", + f"{Path(module.module_dir, f)}", ) ) diff --git a/nf_core/modules/lint/patch.py b/nf_core/modules/lint/module_patch.py similarity index 99% rename from nf_core/modules/lint/patch.py rename to nf_core/modules/lint/module_patch.py index 9331d461ec..e8ce3cbce3 100644 --- a/nf_core/modules/lint/patch.py +++ b/nf_core/modules/lint/module_patch.py @@ -4,7 +4,7 @@ from ..nfcore_module import NFCoreModule -def patch(module_lint_obj, module: NFCoreModule): +def module_patch(module_lint_obj, module: NFCoreModule): """ Lint a patch file found in a module From 3cbb38f3555493cd827ff3fab00db2bdb586ca9b Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 10:00:27 +0200 Subject: [PATCH 08/15] Bug fix in module_patch --- nf_core/modules/lint/__init__.py | 16 ++-------------- nf_core/modules/lint/module_changes.py | 8 +------- nf_core/modules/lint/module_patch.py | 17 ++++++++++------- nf_core/modules/modules_differ.py | 2 ++ 4 files changed, 15 insertions(+), 28 deletions(-) diff --git a/nf_core/modules/lint/__init__.py b/nf_core/modules/lint/__init__.py index 26ba8940ec..8cce9d5bba 100644 --- a/nf_core/modules/lint/__init__.py +++ b/nf_core/modules/lint/__init__.py @@ -75,27 +75,15 @@ def __init__(self, dir, fail_warned=False, remote_url=None, branch=None, no_pull self.warned = [] self.failed = [] self.modules_repo = ModulesRepo(remote_url, branch, no_pull, base_path) - self.lint_tests = self.get_all_lint_tests() + self.lint_tests = self.get_all_lint_tests(self.repo_type == "pipeline") # Get lists of modules install in directory self.all_local_modules, self.all_nfcore_modules = self.get_installed_modules() self.lint_config = None self.modules_json = None - # Add tests specific to nf-core/modules or pipelines - if self.repo_type == "modules": - self.lint_tests.append("module_tests") - - if self.repo_type == "pipeline": - # Add as first test to load git_sha before module_changes - self.lint_tests.insert(0, "module_version") - # Add as the second test to verify the patch file before module_changes - self.lint_test.insert(1) - # Only check if modules have been changed in pipelines - self.lint_tests.append("module_changes") - @staticmethod - def get_all_lint_tests(is_pipeline=True): + def get_all_lint_tests(is_pipeline): if is_pipeline: return [ "module_patch", diff --git a/nf_core/modules/lint/module_changes.py b/nf_core/modules/lint/module_changes.py index c0e1ada3ff..8addb9a7bf 100644 --- a/nf_core/modules/lint/module_changes.py +++ b/nf_core/modules/lint/module_changes.py @@ -35,13 +35,7 @@ def module_changes(module_lint_object, module): with open(tempdir / file, "w") as fh: fh.writelines(lines) except LookupError: - module.failed.append( - ( - "check_local_copy", - "Local copy of module out of sync with diff file", - f"{module.module_dir}", - ) - ) + # This error is already reported by module_patch, so just return return else: tempdir = module.module_dir diff --git a/nf_core/modules/lint/module_patch.py b/nf_core/modules/lint/module_patch.py index e8ce3cbce3..13702ab5e4 100644 --- a/nf_core/modules/lint/module_patch.py +++ b/nf_core/modules/lint/module_patch.py @@ -1,3 +1,5 @@ +import os +import shutil from pathlib import Path from ..modules_differ import ModulesDiffer @@ -52,18 +54,19 @@ def check_patch_valid(module, patch_path): while True: line = next(it) if line.startswith("---"): - frompath = Path(line.split(" ")[1]) + frompath = Path(line.split(" ")[1].strip("\n")) line = next(it) if not line.startswith("+++"): module.failed.append( ( "patch_valid", - "Patch file invalid. Line starting with '---' should always be follow by line starting with '+++'", + "Patch file invalid. Line starting with '---' should always be followed by line starting with '+++'", patch_path, ) ) passed = False - topath = Path(line.split(" ")[1]) + continue + topath = Path(line.split(" ")[1].strip("\n")) if frompath == Path("/dev/null"): paths_in_patch.append((frompath, ModulesDiffer.DiffEnum.CREATED)) elif topath == Path("/dev/null"): @@ -144,7 +147,7 @@ def check_patch_valid(module, patch_path): ("patch", f"Patch file performs file deletion of {path}. This is discouraged.", patch_path) ) if passed: - module.passed(("patch_valid", "Patch file is valid", patch_path)) + module.passed.append(("patch_valid", "Patch file is valid", patch_path)) return passed @@ -164,13 +167,13 @@ def patch_reversible(module, patch_path): module.module_name, "nf-core/modules", patch_path, - Path(module.module_dir).relative(module.base_dir), + Path(module.module_dir).relative_to(module.base_dir), reverse=True, ) except LookupError as e: # Patch failed. Save the patch file by moving to the install dir - module.failed.append((("patch_reversible", "Patch does not agree with module files", patch_path))) + module.failed.append((("patch_reversible", "Patch file is outdated or edited", patch_path))) return False - module.passed((("patch_reversible", "Patch agrees with module files", patch_path))) + module.passed.append((("patch_reversible", "Patch agrees with module files", patch_path))) return True diff --git a/nf_core/modules/modules_differ.py b/nf_core/modules/modules_differ.py index 07dbb50833..8aaaded083 100644 --- a/nf_core/modules/modules_differ.py +++ b/nf_core/modules/modules_differ.py @@ -293,6 +293,8 @@ def per_file_patch(patch_fn): patch_lines = [line] i += 1 line = lines[i] + if not line.startswith("+++ "): + raise LookupError("Missing to-file in patch file") _, topath = line.split(" ") topath = topath.strip() patch_lines.append(line) From 9583f82e7cb1fc11387dd30b498224a0a388bb4e Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 10:44:21 +0200 Subject: [PATCH 09/15] Reverse diff in main.nf and meta.yml lints --- nf_core/modules/lint/main_nf.py | 31 ++++++++++++++++++++-------- nf_core/modules/lint/meta_yml.py | 34 +++++++++++++++++++++++-------- nf_core/modules/modules_differ.py | 8 ++++---- nf_core/modules/update.py | 2 +- 4 files changed, 54 insertions(+), 21 deletions(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index 4f9dc2ad3b..00459b793a 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -5,11 +5,14 @@ import logging import re +import sys +from pathlib import Path import requests import nf_core import nf_core.modules.module_utils +from nf_core.modules.modules_differ import ModulesDiffer log = logging.getLogger(__name__) @@ -37,14 +40,26 @@ def main_nf(module_lint_object, module, fix_version, progress_bar): inputs = [] outputs = [] - # Check whether file exists and load it - try: - with open(module.main_nf, "r") as fh: - lines = fh.readlines() - module.passed.append(("main_nf_exists", "Module file exists", module.main_nf)) - except FileNotFoundError as e: - module.failed.append(("main_nf_exists", "Module file does not exist", module.main_nf)) - return + # Check if we have a patch file affecting the 'main.nf' file + # otherwise read the lines directly from the module + lines = None + if module.is_patched: + lines = ModulesDiffer.try_apply_patch( + module.module_name, + "nf-core/modules", + module.patch_path, + Path(module.module_dir).relative_to(module.base_dir), + reverse=True, + ).get("main.nf") + if lines is None: + try: + # Check whether file exists and load it + with open(module.main_nf, "r") as fh: + lines = fh.readlines() + module.passed.append(("main_nf_exists", "Module file exists", module.main_nf)) + except FileNotFoundError as e: + module.failed.append(("main_nf_exists", "Module file does not exist", module.main_nf)) + return deprecated_i = ["initOptions", "saveFiles", "getSoftwareName", "getProcessName", "publishDir"] lines_j = "\n".join(lines) diff --git a/nf_core/modules/lint/meta_yml.py b/nf_core/modules/lint/meta_yml.py index 620d6c8982..b874e00994 100644 --- a/nf_core/modules/lint/meta_yml.py +++ b/nf_core/modules/lint/meta_yml.py @@ -1,7 +1,11 @@ #!/usr/bin/env python +from pathlib import Path + import yaml +from nf_core.modules.modules_differ import ModulesDiffer + def meta_yml(module_lint_object, module): """ @@ -19,19 +23,33 @@ def meta_yml(module_lint_object, module): """ required_keys = ["name", "output"] required_keys_lists = ["input", "output"] - try: - with open(module.meta_yml, "r") as fh: - meta_yaml = yaml.safe_load(fh) - module.passed.append(("meta_yml_exists", "Module `meta.yml` exists", module.meta_yml)) - except FileNotFoundError: - module.failed.append(("meta_yml_exists", "Module `meta.yml` does not exist", module.meta_yml)) - return + # Check if we have a patch file, get original file in that case + meta_yaml = None + if module.is_patched: + lines = ModulesDiffer.try_apply_patch( + module.module_name, + "nf-core/modules", + module.patch_path, + Path(module.module_dir).relative_to(module.base_dir), + reverse=True, + ).get("meta.yml") + if lines is not None: + meta_yaml = yaml.safe_load("".join(lines)) + if meta_yaml is None: + try: + with open(module.meta_yml, "r") as fh: + meta_yaml = yaml.safe_load(fh) + module.passed.append(("meta_yml_exists", "Module `meta.yml` exists", module.meta_yml)) + except FileNotFoundError: + module.failed.append(("meta_yml_exists", "Module `meta.yml` does not exist", module.meta_yml)) + return # Confirm that all required keys are given contains_required_keys = True all_list_children = True for rk in required_keys: - if not rk in meta_yaml.keys(): + print(rk) + if rk not in meta_yaml.keys(): module.failed.append(("meta_required_keys", f"`{rk}` not specified in YAML", module.meta_yml)) contains_required_keys = False elif rk in meta_yaml.keys() and not isinstance(meta_yaml[rk], list) and rk in required_keys_lists: diff --git a/nf_core/modules/modules_differ.py b/nf_core/modules/modules_differ.py index 8aaaded083..088c32da9c 100644 --- a/nf_core/modules/modules_differ.py +++ b/nf_core/modules/modules_differ.py @@ -441,10 +441,10 @@ def try_apply_patch(module, repo_name, patch_path, module_dir, reverse=False): new_files = {} for file, patch in patches.items(): log.debug(f"Applying patch to {file}") - file_relpath = Path(file).relative_to(module_relpath) - file_path = module_dir / file_relpath + fn = Path(file).relative_to(module_relpath) + file_path = module_dir / fn with open(file_path, "r") as fh: file_lines = fh.readlines() patched_new_lines = ModulesDiffer.try_apply_single_patch(file_lines, patch, reverse=reverse) - new_files[file_relpath] = patched_new_lines - return new_files + new_files[str(fn)] = patched_new_lines + return new_files diff --git a/nf_core/modules/update.py b/nf_core/modules/update.py index 3daa7e81ff..085d41bbfe 100644 --- a/nf_core/modules/update.py +++ b/nf_core/modules/update.py @@ -516,7 +516,7 @@ def try_apply_patch(self, module, repo_name, patch_relpath, module_dir, module_i for file, new_content in new_files.items(): fn = temp_module_dir / file with open(fn, "w") as fh: - fh.write(new_content) + fh.writelines(new_content) # Create the new patch file log.debug("Regenerating patch file") From c7079e86fa6080d37c7fe1fb1d07e117b3bb59ec Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 10:54:40 +0200 Subject: [PATCH 10/15] Remove test --- tests/modules/lint.py | 49 ------------------------------------------- tests/test_modules.py | 1 - 2 files changed, 50 deletions(-) diff --git a/tests/modules/lint.py b/tests/modules/lint.py index d83c63468e..0f60377d5e 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -1,10 +1,5 @@ -from pathlib import Path - import nf_core.modules -from ..utils import GITLAB_REPO, GITLAB_URL -from .patch import BISMARK_ALIGN, PATCH_BRANCH, setup_patch - def test_modules_lint_trimgalore(self): """Test linting the TrimGalore! module""" @@ -35,47 +30,3 @@ def test_modules_lint_new_modules(self): assert len(module_lint.failed) == 0, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" assert len(module_lint.passed) > 0 assert len(module_lint.warned) >= 0 - - -def test_modules_lint_patch_invalid(self): - """Try linting a module that has a corrupted patch file""" - # Install the trimgalore module and modify it - setup_patch(self.pipeline_dir, True) - - # Create the patch file - nf_core.modules.ModulePatch(self.pipeline_dir, GITLAB_URL, PATCH_BRANCH).patch(BISMARK_ALIGN) - - module_lint = nf_core.modules.ModuleLint(dir=self.pipeline_dir) - - # Modify the file - patch_path = Path( - self.pipeline_dir, "modules", GITLAB_REPO, BISMARK_ALIGN, f"{BISMARK_ALIGN.replace('/', '-')}.diff" - ) - with open(patch_path, "r") as fh: - org_lines = fh.readlines() - - mod_lines = org_lines.copy() - for i, line in enumerate(mod_lines): - if line.startswith("+++"): - mod_lines.pop(i) - break - with open(patch_path, "w") as fh: - fh.writelines(mod_lines) - - module_lint.lint(print_results=False, module=BISMARK_ALIGN) - assert len(module_lint.failed) == 1, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" - assert len(module_lint.passed) > 0 - assert len(module_lint.warned) >= 0 - - mod_lines = org_lines.copy() - for i, line in enumerate(mod_lines): - if line.startswith("@@"): - mod_lines.pop(i) - break - with open(patch_path, "w") as fh: - fh.writelines(mod_lines) - - module_lint.lint(print_results=False, module=BISMARK_ALIGN) - assert len(module_lint.failed) == 1, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" - assert len(module_lint.passed) > 0 - assert len(module_lint.warned) >= 0 diff --git a/tests/test_modules.py b/tests/test_modules.py index a36ba9781d..786fa4676e 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -111,7 +111,6 @@ def test_modulesrepo_class(self): from .modules.lint import ( test_modules_lint_empty, test_modules_lint_new_modules, - test_modules_lint_patch_invalid, test_modules_lint_trimgalore, ) from .modules.list import ( From 7706a3fd0a6154ba7927e1083bcf3cef409343a6 Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 11:11:02 +0200 Subject: [PATCH 11/15] Update changelog and docs --- CHANGELOG.md | 1 + README.md | 2 ++ 2 files changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e49127146..cb64dc7508 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ - Add short option for `--no-pull` option in `nf-core modules` - Add `nf-core modules patch` command ([#1312](https://github.com/nf-core/tools/issues/1312)) - Add support for patch in `nf-core modules update` command ([#1312](https://github.com/nf-core/tools/issues/1312)) +- Add support for patch in `nf-core modules lint` command ([#1312](https://github.com/nf-core/tools/issues/1312)) ## [v2.4.1 - Cobolt Koala Patch](https://github.com/nf-core/tools/releases/tag/2.4) - [2022-05-16] diff --git a/README.md b/README.md index d2a852f216..f4e3f02870 100644 --- a/README.md +++ b/README.md @@ -1219,6 +1219,8 @@ The generated patches work with `nf-core modules update`: when you install a new the patch automatically. The patch application fails if the new version of the module modifies the same lines as the patch. In this case, the patch new version is installed but the old patch file is preversed. +When linting a patched module, the patch is applied in reverse to recover the original files and then the module is linted as usual. + ### Create a new module This command creates a new nf-core module from the nf-core module template. From 1aab434c21f7a08207383f96dee7f5720e97131e Mon Sep 17 00:00:00 2001 From: Erik Danielsson <53212377+ErikDanielsson@users.noreply.github.com> Date: Mon, 1 Aug 2022 16:14:41 +0200 Subject: [PATCH 12/15] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: JĂșlia Mir Pedrol --- nf_core/modules/lint/module_patch.py | 2 +- nf_core/modules/modules_differ.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nf_core/modules/lint/module_patch.py b/nf_core/modules/lint/module_patch.py index 13702ab5e4..cbdd85869b 100644 --- a/nf_core/modules/lint/module_patch.py +++ b/nf_core/modules/lint/module_patch.py @@ -77,7 +77,7 @@ def check_patch_valid(module, patch_path): module.failed.append( ( "patch_valid", - "Patch file invaldi. From file '{frompath}' mismatched with to path '{topath}'", + f"Patch file invaldi. From file '{frompath}' mismatched with to path '{topath}'", patch_path, ) ) diff --git a/nf_core/modules/modules_differ.py b/nf_core/modules/modules_differ.py index 088c32da9c..8a2c4b7f11 100644 --- a/nf_core/modules/modules_differ.py +++ b/nf_core/modules/modules_differ.py @@ -434,7 +434,7 @@ def try_apply_patch(module, repo_name, patch_path, module_dir, reverse=False): as keys and the patched file contents as values Raises: - LookupError: If the the patch application fails in a file + LookupError: If the patch application fails in a file """ module_relpath = Path("modules", repo_name, module) patches = ModulesDiffer.per_file_patch(patch_path) From 0c2cf8469bb509b2a570c5f7d49c46082ac42806 Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 16:31:04 +0200 Subject: [PATCH 13/15] Apply suggestions from @mirpedrol --- nf_core/modules/modules_differ.py | 25 +++++++++++++++---------- nf_core/modules/patch.py | 1 - nf_core/modules/update.py | 9 ++++++++- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/nf_core/modules/modules_differ.py b/nf_core/modules/modules_differ.py index 8a2c4b7f11..66d0c7eba2 100644 --- a/nf_core/modules/modules_differ.py +++ b/nf_core/modules/modules_differ.py @@ -53,10 +53,6 @@ def get_module_diffs(from_dir, to_dir, for_git=True, dsp_from_dir=None, dsp_to_d dict[str, (ModulesDiffer.DiffEnum, str)]: A dictionary containing the diff type and the diff string (empty if no diff) """ - if dsp_from_dir is None: - dsp_from_dir = from_dir - if dsp_to_dir is None: - dsp_to_dir = to_dir if for_git: dsp_from_dir = Path("a", dsp_from_dir) dsp_to_dir = Path("b", dsp_to_dir) @@ -157,9 +153,13 @@ def write_diff_file( for_git (bool): indicates whether the diff file is to be compatible with `git apply`. If true it adds a/ and b/ prefixes to the file paths - dsp_from_dir (str | Path): The from directory to display in the diff - dsp_to_dir (str | Path): The to directory to display in the diff + dsp_from_dir (str | Path): The 'from' directory displayed in the diff + dsp_to_dir (str | Path): The 'to' directory displayed in the diff """ + if dsp_from_dir is None: + dsp_from_dir = from_dir + if dsp_to_dir is None: + dsp_to_dir = to_dir diffs = ModulesDiffer.get_module_diffs(from_dir, to_dir, for_git, dsp_from_dir, dsp_to_dir) if all(diff_status == ModulesDiffer.DiffEnum.UNCHANGED for _, (diff_status, _) in diffs.items()): @@ -228,14 +228,19 @@ def print_diff( Args: module (str): The module name repo_name (str): The name of the repo where the module resides - diffs (dict[str, (ModulesDiffer.DiffEnum, str)]): A dictionary containing - the type of change and the diff (if any) - from_dir (str): The directory containing the old module files - to_dir (str): The directory containing the new module files + from_dir (str | Path): The directory containing the old module files + to_dir (str | Path): The directory containing the new module files module_dir (str): The path to the current installation of the module current_version (str): The installed version of the module new_version (str): The version of the module the diff is computed against + dsp_from_dir (str | Path): The 'from' directory displayed in the diff + dsp_to_dir (str | Path): The 'to' directory displayed in the diff """ + if dsp_from_dir is None: + dsp_from_dir = from_dir + if dsp_to_dir is None: + dsp_to_dir = to_dir + diffs = ModulesDiffer.get_module_diffs( from_dir, to_dir, for_git=False, dsp_from_dir=dsp_from_dir, dsp_to_dir=dsp_to_dir ) diff --git a/nf_core/modules/patch.py b/nf_core/modules/patch.py index d84e58caf8..cba7c96d61 100644 --- a/nf_core/modules/patch.py +++ b/nf_core/modules/patch.py @@ -55,7 +55,6 @@ def patch(self, module=None): patch_relpath = Path(module_relpath, patch_filename) module_dir = Path(self.dir, module_relpath) patch_path = Path(self.dir, patch_relpath) - print(patch_path) if patch_path.exists(): remove = questionary.confirm( diff --git a/nf_core/modules/update.py b/nf_core/modules/update.py index 085d41bbfe..a6204c85f2 100644 --- a/nf_core/modules/update.py +++ b/nf_core/modules/update.py @@ -215,7 +215,14 @@ def update(self, module=None): elif self.show_diff: ModulesDiffer.print_diff( - module, modules_repo.fullname, module_dir, module_install_dir, current_version, version + module, + modules_repo.fullname, + module_dir, + module_install_dir, + current_version, + version, + dsp_from_dir=module_dir, + dsp_to_dir=module_dir, ) # Ask the user if they want to install the module From f63a171a95828dc8c4beecf7a8d5e4b51382be97 Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 18:00:43 +0200 Subject: [PATCH 14/15] Remove print stmt --- nf_core/modules/patch.py | 1 - 1 file changed, 1 deletion(-) diff --git a/nf_core/modules/patch.py b/nf_core/modules/patch.py index cba7c96d61..c276d4e146 100644 --- a/nf_core/modules/patch.py +++ b/nf_core/modules/patch.py @@ -105,5 +105,4 @@ def patch(self, module=None): # Finally move the created patch file to its final location shutil.move(patch_temp_path, patch_path) - print(f"{patch_path} exists? {patch_path.exists()}") log.info(f"Patch file of '{module_fullname}' written to '{patch_path}'") From ead9a690fa7c8dfb58e6acc248c2da7288b431df Mon Sep 17 00:00:00 2001 From: Erik Danielsson Date: Mon, 1 Aug 2022 18:06:31 +0200 Subject: [PATCH 15/15] Remove another print stmt... --- nf_core/modules/lint/meta_yml.py | 1 - 1 file changed, 1 deletion(-) diff --git a/nf_core/modules/lint/meta_yml.py b/nf_core/modules/lint/meta_yml.py index b874e00994..42f5ac21a1 100644 --- a/nf_core/modules/lint/meta_yml.py +++ b/nf_core/modules/lint/meta_yml.py @@ -48,7 +48,6 @@ def meta_yml(module_lint_object, module): contains_required_keys = True all_list_children = True for rk in required_keys: - print(rk) if rk not in meta_yaml.keys(): module.failed.append(("meta_required_keys", f"`{rk}` not specified in YAML", module.meta_yml)) contains_required_keys = False