From 956ad8143fd599994903f53904e3920b8af36cb6 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 24 Jan 2022 11:10:59 +0100 Subject: [PATCH 1/5] Fix adding new files via tarball upload The API of `repo.dirstate.write` has changed and takes a `tr` parameter now. I've removed the whole fallback since it fails and uses internals of mercurial that we should probably not be using anyway. Instead we're going to delete untracked files in hg_util.remove_file() --- lib/tool_shed/util/commit_util.py | 27 ++++--------------- lib/tool_shed/util/hg_util.py | 20 +++++++++----- .../webapp/controllers/repository.py | 26 +++--------------- 3 files changed, 23 insertions(+), 50 deletions(-) diff --git a/lib/tool_shed/util/commit_util.py b/lib/tool_shed/util/commit_util.py index 845d52c441d..799d5b862ab 100644 --- a/lib/tool_shed/util/commit_util.py +++ b/lib/tool_shed/util/commit_util.py @@ -17,7 +17,7 @@ from tool_shed.util import basic_util, hg_util, shed_util_common as suc log = logging.getLogger(__name__) -UNDESIRABLE_DIRS = ['.hg', '.svn', '.git', '.cvs'] +UNDESIRABLE_DIRS = ['.hg', '.svn', '.git', '.cvs', '.idea'] UNDESIRABLE_FILES = ['.hg_archival.txt', 'hgrc', '.DS_Store', 'tool_test_output.html', 'tool_test_output.json'] @@ -155,7 +155,6 @@ def handle_directory_changes(app, host, username, repository, full_path, filenam content_alert_str = '' files_to_remove = [] filenames_in_archive = [os.path.join(full_path, name) for name in filenames_in_archive] - repo = repository.hg_repo if remove_repo_files_not_in_tar and not repository.is_new(): # We have a repository that is not new (it contains files), so discover those files that are in the # repository, but not in the uploaded archive. @@ -177,27 +176,11 @@ def handle_directory_changes(app, host, username, repository, full_path, filenam # Remove files in the repository (relative to the upload point) that are not in # the uploaded archive. try: - hg_util.remove_file(repo_path, repo_file, force=True) + hg_util.remove_file(repo_path, repo_file) except Exception as e: - log.debug(f"Error removing files using the mercurial API, so trying a different approach, the error was: {str(e)}") - relative_selected_file = repo_file.split('repo_%d' % repository.id)[1].lstrip('/') - repo.dirstate.remove(relative_selected_file) - repo.dirstate.write() - absolute_selected_file = os.path.abspath(repo_file) - if os.path.isdir(absolute_selected_file): - try: - os.rmdir(absolute_selected_file) - except OSError: - # The directory is not empty. - pass - elif os.path.isfile(absolute_selected_file): - os.remove(absolute_selected_file) - dir = os.path.split(absolute_selected_file)[0] - try: - os.rmdir(dir) - except OSError: - # The directory is not empty. - pass + error_message = (f"Error removing file {repo_file} in mercurial repo:\n{e}") + log.debug(error_message) + return 'error', error_message, files_to_remove, content_alert_str, 0, 0 # See if any admin users have chosen to receive email alerts when a repository is updated. # If so, check every uploaded file to ensure content is appropriate. check_contents = check_file_contents_for_email_alerts(app) diff --git a/lib/tool_shed/util/hg_util.py b/lib/tool_shed/util/hg_util.py index f7de9b28884..bbba757c1e4 100644 --- a/lib/tool_shed/util/hg_util.py +++ b/lib/tool_shed/util/hg_util.py @@ -1,5 +1,6 @@ import logging import os +import shutil import subprocess import tempfile from datetime import datetime @@ -209,17 +210,24 @@ def get_rev_label_from_changeset_revision(repo, changeset_revision, include_date return rev, label -def remove_file(repo_path, selected_file, force=True): - cmd = ['hg', 'remove'] - if force: - cmd.append('--force') - cmd.append(selected_file) +def remove_file(repo_path, selected_file): + cmd = ['hg', 'remove', '--force', selected_file] try: subprocess.check_output(cmd, stderr=subprocess.STDOUT, cwd=repo_path) except Exception as e: error_message = f"Error removing file '{selected_file}': {unicodify(e)}" if isinstance(e, subprocess.CalledProcessError): - error_message += f"\nOutput was:\n{unicodify(e.output)}" + output = unicodify(e.output) + if 'is untracked' in output: + # That's ok, happens if we add a new file or directory via tarball upload, + # just delete the file or dir on disk + selected_file_path = os.path.join(repo_path, selected_file) + if os.path.isdir(selected_file_path): + shutil.rmtree(selected_file_path) + else: + os.remove(selected_file_path) + return + error_message += f"\nOutput was:\n{output}" raise Exception(error_message) diff --git a/lib/tool_shed/webapp/controllers/repository.py b/lib/tool_shed/webapp/controllers/repository.py index b0eb5c21372..54ca9a2be78 100644 --- a/lib/tool_shed/webapp/controllers/repository.py +++ b/lib/tool_shed/webapp/controllers/repository.py @@ -2156,29 +2156,11 @@ class RepositoryController(BaseUIController, ratings_util.ItemRatings): tip = repository.tip() for selected_file in selected_files_to_delete: try: - hg_util.remove_file(repo_dir, selected_file, force=True) + hg_util.remove_file(repo_dir, selected_file) except Exception as e: - log.debug("Error removing the following file using the mercurial API:\n %s", selected_file) - log.debug("The error was: %s", util.unicodify(e)) - log.debug("Attempting to remove the file using a different approach.") - relative_selected_file = selected_file.split('repo_%d' % repository.id)[1].lstrip('/') - repo.dirstate.remove(relative_selected_file) - repo.dirstate.write() - absolute_selected_file = os.path.abspath(selected_file) - if os.path.isdir(absolute_selected_file): - try: - os.rmdir(absolute_selected_file) - except OSError: - # The directory is not empty - pass - elif os.path.isfile(absolute_selected_file): - os.remove(absolute_selected_file) - dir = os.path.split(absolute_selected_file)[0] - try: - os.rmdir(dir) - except OSError: - # The directory is not empty - pass + status = 'error' + message = f"Error removing file {selected_file} in mercurial repo:\n{e}" + log.debug(message) # Commit the change set. if not commit_message: commit_message = 'Deleted selected files' From 5c39169a036e09d8faf81ed5edaa11b205f63da5 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 24 Jan 2022 12:25:14 +0100 Subject: [PATCH 2/5] Partial unit tests for hgutil.py --- lib/tool_shed/util/hg_util.py | 6 +- .../webapp/controllers/repository.py | 2 +- test/unit/shed_unit/test_hg_util.py | 57 +++++++++++++++++++ 3 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 test/unit/shed_unit/test_hg_util.py diff --git a/lib/tool_shed/util/hg_util.py b/lib/tool_shed/util/hg_util.py index bbba757c1e4..82c8feb2bd4 100644 --- a/lib/tool_shed/util/hg_util.py +++ b/lib/tool_shed/util/hg_util.py @@ -210,12 +210,12 @@ def get_rev_label_from_changeset_revision(repo, changeset_revision, include_date return rev, label -def remove_file(repo_path, selected_file): +def remove_path(repo_path, selected_file): cmd = ['hg', 'remove', '--force', selected_file] try: subprocess.check_output(cmd, stderr=subprocess.STDOUT, cwd=repo_path) except Exception as e: - error_message = f"Error removing file '{selected_file}': {unicodify(e)}" + error_message = f"Error removing path '{selected_file}': {unicodify(e)}" if isinstance(e, subprocess.CalledProcessError): output = unicodify(e.output) if 'is untracked' in output: @@ -278,7 +278,7 @@ __all__ = ( 'get_revision_label_from_ctx', 'get_rev_label_from_changeset_revision', 'pull_repository', - 'remove_file', + 'remove_path', 'reversed_lower_upper_bounded_changelog', 'reversed_upper_bounded_changelog', 'update_repository', diff --git a/lib/tool_shed/webapp/controllers/repository.py b/lib/tool_shed/webapp/controllers/repository.py index 54ca9a2be78..c44aab686a6 100644 --- a/lib/tool_shed/webapp/controllers/repository.py +++ b/lib/tool_shed/webapp/controllers/repository.py @@ -2156,7 +2156,7 @@ class RepositoryController(BaseUIController, ratings_util.ItemRatings): tip = repository.tip() for selected_file in selected_files_to_delete: try: - hg_util.remove_file(repo_dir, selected_file) + hg_util.remove_path(repo_dir, selected_file) except Exception as e: status = 'error' message = f"Error removing file {selected_file} in mercurial repo:\n{e}" diff --git a/test/unit/shed_unit/test_hg_util.py b/test/unit/shed_unit/test_hg_util.py new file mode 100644 index 00000000000..b5d2f88b165 --- /dev/null +++ b/test/unit/shed_unit/test_hg_util.py @@ -0,0 +1,57 @@ +import pytest + +from tool_shed.util import hg_util + + +def test_init_repository(tmpdir): + hg_util.init_repository(str(tmpdir)) + assert (tmpdir / '.hg').exists() + + +def test_init_repository_fails(tmpdir): + test_init_repository(tmpdir) + with pytest.raises(Exception) as exc_info: + test_init_repository(tmpdir) + assert 'Error initializing repository' in str(exc_info.value) + + +def test_add_file_and_commmit_changeset(tmpdir): + test_init_repository(tmpdir) + path_to_add = (tmpdir / 'test.txt') + path_to_add.write('test') + hg_util.add_changeset(str(tmpdir), str(path_to_add)) + hg_util.commit_changeset(str(tmpdir), str(path_to_add), 'testuser', 'testcommit') + return path_to_add + + +def test_add_dir_and_commit_changeset(tmpdir): + test_init_repository(tmpdir) + path_to_add = tmpdir.mkdir('abc') + (path_to_add / 'test.txt').write('bla') + hg_util.add_changeset(str(tmpdir), str(path_to_add)) + hg_util.commit_changeset(str(tmpdir), str(path_to_add), 'testuser', 'testcommit') + return path_to_add + + +def test_remove_tracked_file(tmpdir): + path_to_remove = test_add_file_and_commmit_changeset(tmpdir) + hg_util.remove_path(str(tmpdir), path_to_remove) + + +def test_remove_untracked_file(tmpdir): + test_init_repository(tmpdir) + untracked_path = tmpdir / 'untracked.txt' + untracked_path.write('bla') + hg_util.remove_path(str(tmpdir), str(untracked_path)) + + +def test_remove_tracked_dir(tmpdir): + path_to_remove = test_add_dir_and_commit_changeset(tmpdir) + hg_util.remove_path(str(tmpdir), path_to_remove) + + +def test_remove_nonexistant_file_fails(tmpdir): + test_init_repository(tmpdir) + with pytest.raises(Exception) as exc_info: + hg_util.remove_path(str(tmpdir), str(tmpdir / 'some path')) + assert 'Error removing path' in str(exc_info.value) From 381d218180be16a113ce4ee497ad32f893b4a9c4 Mon Sep 17 00:00:00 2001 From: Simon Bray Date: Mon, 24 Jan 2022 13:37:59 +0100 Subject: [PATCH 3/5] ensure tool_id/content_id is updated when refactoring workflows --- lib/galaxy/workflow/refactor/execute.py | 4 ++++ lib/galaxy_test/base/workflow_fixtures.py | 4 ++++ test/integration/test_workflow_refactoring.py | 11 +++++++++-- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/workflow/refactor/execute.py b/lib/galaxy/workflow/refactor/execute.py index 3a6a76a99ea..a6c274ca0b7 100644 --- a/lib/galaxy/workflow/refactor/execute.py +++ b/lib/galaxy/workflow/refactor/execute.py @@ -391,6 +391,10 @@ class WorkflowRefactorExecutor: self._inject_for_updated_step(step, execution) step_def["tool_version"] = tool_version step_def["tool_state"] = step.module.get_tool_state() + if step_def.get("tool_id"): + step_def["tool_id"] = step.module.get_content_id() + if step_def.get("content_id"): + step_def["content_id"] = step.module.get_content_id() self._patch_step(execution, step, step_def) def _apply_upgrade_all_steps(self, action: UpgradeAllStepsAction, execution: RefactorActionExecution): diff --git a/lib/galaxy_test/base/workflow_fixtures.py b/lib/galaxy_test/base/workflow_fixtures.py index 5b28ee8a6d1..e6187e1d99c 100644 --- a/lib/galaxy_test/base/workflow_fixtures.py +++ b/lib/galaxy_test/base/workflow_fixtures.py @@ -423,6 +423,10 @@ steps: in: input1: nested_workflow/workflow_output queries_0|input2: nested_workflow/workflow_output + compose_text_param: + tool_id: compose_text_param + tool_version: 0.1.0 + label: compose_text_param """ WORKFLOW_WITH_OUTPUT_ACTIONS = """ diff --git a/test/integration/test_workflow_refactoring.py b/test/integration/test_workflow_refactoring.py index 17ba3819c60..719d7f87f0b 100644 --- a/test/integration/test_workflow_refactoring.py +++ b/test/integration/test_workflow_refactoring.py @@ -19,6 +19,9 @@ from galaxy.workflow.refactor.schema import ( from galaxy_test.base.populators import ( WorkflowPopulator, ) +from galaxy_test.base.uses_shed import ( + UsesShed +) from galaxy_test.base.workflow_fixtures import ( WORKFLOW_NESTED_RUNTIME_PARAMETER, WORKFLOW_NESTED_SIMPLE, @@ -42,7 +45,7 @@ steps: """ -class WorkflowRefactoringIntegrationTestCase(integration_util.IntegrationTestCase): +class WorkflowRefactoringIntegrationTestCase(integration_util.IntegrationTestCase, UsesShed): framework_tool_and_types = True @@ -693,6 +696,8 @@ steps: assert message.output_label == "outer_output" def test_upgrade_all_steps(self): + self.install_repository("iuc", "compose_text_param", "feb3acba1e0a") # 0.1.0 + self.install_repository("iuc", "compose_text_param", "e188c9826e0f") # 0.1.1 self.workflow_populator.upload_yaml_workflow(WORKFLOW_NESTED_WITH_MULTIPLE_VERSIONS_TOOL) nested_stored_workflow = self._recent_stored_workflow(2) assert self._latest_workflow.step_by_label("tool_update_step").tool_version == "0.1" @@ -716,13 +721,15 @@ steps: nested_stored_workflow = self._recent_stored_workflow(2) updated_nested_step = nested_stored_workflow.latest_workflow.step_by_label("random_lines") assert updated_nested_step.tool_inputs["num_lines"] == "2" + assert self._latest_workflow.step_by_label("compose_text_param").tool_version == '0.1.1' + assert self._latest_workflow.step_by_label("compose_text_param").tool_id == 'toolshed.g2.bx.psu.edu/repos/iuc/compose_text_param/compose_text_param/0.1.1' assert len(action_executions) == 1 messages = action_executions[0].messages assert len(messages) == 1 message = messages[0] assert message.message_type == RefactorActionExecutionMessageTypeEnum.connection_drop_forced - assert message.order_index == 1 + assert message.order_index == 2 assert message.step_label == "tool_update_step" assert message.output_name == "output" From 2d70fa546686ae268a1d29afd54d241298f567d4 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 26 Jan 2022 16:54:42 +0100 Subject: [PATCH 4/5] Fix one usage of hg_util.remove_file I missed this one in https://github.com/galaxyproject/galaxy/pull/13230 --- lib/tool_shed/util/commit_util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tool_shed/util/commit_util.py b/lib/tool_shed/util/commit_util.py index 799d5b862ab..fcc9e988def 100644 --- a/lib/tool_shed/util/commit_util.py +++ b/lib/tool_shed/util/commit_util.py @@ -176,7 +176,7 @@ def handle_directory_changes(app, host, username, repository, full_path, filenam # Remove files in the repository (relative to the upload point) that are not in # the uploaded archive. try: - hg_util.remove_file(repo_path, repo_file) + hg_util.remove_path(repo_path, repo_file) except Exception as e: error_message = (f"Error removing file {repo_file} in mercurial repo:\n{e}") log.debug(error_message) From 6cb59a51fe32ad25eb4644ecd538725c6862f8de Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 27 Jan 2022 14:24:54 +0100 Subject: [PATCH 5/5] Fix imported subworkflows cannot be edited There may be no StoredWorkflow associated with the imported subworkflow yet, so trying to edit the subworkflow without it will fail. xref https://github.com/galaxyproject/galaxy/pull/10160 --- lib/galaxy/webapps/galaxy/controllers/workflow.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/controllers/workflow.py b/lib/galaxy/webapps/galaxy/controllers/workflow.py index e181a082456..20e7506ea17 100644 --- a/lib/galaxy/webapps/galaxy/controllers/workflow.py +++ b/lib/galaxy/webapps/galaxy/controllers/workflow.py @@ -655,8 +655,7 @@ class WorkflowController(BaseUIController, SharableMixin, UsesStoredWorkflowMixi """ if not id: if workflow_id: - workflow = trans.sa_session.query(model.Workflow).get(trans.security.decode_id(workflow_id)) - stored_workflow = workflow.stored_workflow + stored_workflow = self.app.workflow_manager.get_stored_workflow(trans, workflow_id, by_stored_id=False) self.security_check(trans, stored_workflow, True, False) stored_workflow_id = trans.security.encode_id(stored_workflow.id) return trans.response.send_redirect(f'{url_for("/")}workflow/editor?id={stored_workflow_id}')