From 35b29449b0f90638de63ae8aea042ba08c4acb75 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 9 Apr 2025 15:58:46 +0100 Subject: [PATCH 1/9] Fix command line too long on windows. --- datashuttle/utils/rclone.py | 40 ++++++++++++++++++++--- tests/tests_integration/test_datatypes.py | 6 ++-- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/datashuttle/utils/rclone.py b/datashuttle/utils/rclone.py index 9644c103c..c52ef3816 100644 --- a/datashuttle/utils/rclone.py +++ b/datashuttle/utils/rclone.py @@ -1,4 +1,7 @@ +import os +import platform import subprocess +import tempfile from pathlib import Path from subprocess import CompletedProcess from typing import Dict, List, Literal @@ -20,12 +23,39 @@ def call_rclone(command: str, pipe_std: bool = False) -> CompletedProcess: pipe_std: if True, do not output anything to stdout. """ command = "rclone " + command - if pipe_std: - output = subprocess.run( - command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True - ) + + if platform.system() == "Windows": + # Write command to a temporary .bat script + with tempfile.NamedTemporaryFile( + mode="w", suffix=".bat", delete=False + ) as tmp_script: + tmp_script.write(command) + tmp_script_path = tmp_script.name + + try: + if pipe_std: + output = subprocess.run( + [tmp_script_path], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + shell=True, + ) + else: + output = subprocess.run([tmp_script_path], shell=True) + finally: + os.remove(tmp_script_path) # Clean up temp script + else: - output = subprocess.run(command, shell=True) + # On non-Windows, just run it normally + if pipe_std: + output = subprocess.run( + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + shell=True, + ) + else: + output = subprocess.run(command, shell=True) return output diff --git a/tests/tests_integration/test_datatypes.py b/tests/tests_integration/test_datatypes.py index 1aec45480..93288ec97 100644 --- a/tests/tests_integration/test_datatypes.py +++ b/tests/tests_integration/test_datatypes.py @@ -63,9 +63,9 @@ def test_transfer_datatypes( # Unfortunately on Windows we are encountering 'The command line is too long' # and so cannot test against all datatypes here. - some_narrow_datatypes = canonical_configs.quick_get_narrow_datatypes()[ - :10 - ] + some_narrow_datatypes = ( + canonical_configs.quick_get_narrow_datatypes() + ) # [:10] datatypes_used = self.get_narrow_only_datatypes_used(used=False) for key in some_narrow_datatypes: From f02e5f92eeb6d0e91dfc6b16652bb25a403cbb59 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 9 Apr 2025 15:58:46 +0100 Subject: [PATCH 2/9] Fix command line too long on windows. --- datashuttle/utils/rclone.py | 40 ++++++++++++++++++++--- tests/tests_integration/test_datatypes.py | 6 ++-- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/datashuttle/utils/rclone.py b/datashuttle/utils/rclone.py index 9644c103c..c52ef3816 100644 --- a/datashuttle/utils/rclone.py +++ b/datashuttle/utils/rclone.py @@ -1,4 +1,7 @@ +import os +import platform import subprocess +import tempfile from pathlib import Path from subprocess import CompletedProcess from typing import Dict, List, Literal @@ -20,12 +23,39 @@ def call_rclone(command: str, pipe_std: bool = False) -> CompletedProcess: pipe_std: if True, do not output anything to stdout. """ command = "rclone " + command - if pipe_std: - output = subprocess.run( - command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True - ) + + if platform.system() == "Windows": + # Write command to a temporary .bat script + with tempfile.NamedTemporaryFile( + mode="w", suffix=".bat", delete=False + ) as tmp_script: + tmp_script.write(command) + tmp_script_path = tmp_script.name + + try: + if pipe_std: + output = subprocess.run( + [tmp_script_path], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + shell=True, + ) + else: + output = subprocess.run([tmp_script_path], shell=True) + finally: + os.remove(tmp_script_path) # Clean up temp script + else: - output = subprocess.run(command, shell=True) + # On non-Windows, just run it normally + if pipe_std: + output = subprocess.run( + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + shell=True, + ) + else: + output = subprocess.run(command, shell=True) return output diff --git a/tests/tests_integration/test_datatypes.py b/tests/tests_integration/test_datatypes.py index 1aec45480..93288ec97 100644 --- a/tests/tests_integration/test_datatypes.py +++ b/tests/tests_integration/test_datatypes.py @@ -63,9 +63,9 @@ def test_transfer_datatypes( # Unfortunately on Windows we are encountering 'The command line is too long' # and so cannot test against all datatypes here. - some_narrow_datatypes = canonical_configs.quick_get_narrow_datatypes()[ - :10 - ] + some_narrow_datatypes = ( + canonical_configs.quick_get_narrow_datatypes() + ) # [:10] datatypes_used = self.get_narrow_only_datatypes_used(used=False) for key in some_narrow_datatypes: From d2a1078d8f8705c35bd5f0822cd85053ea1b5dfb Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 9 Apr 2025 15:01:40 +0000 Subject: [PATCH 3/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- datashuttle/tui/shared/validate_content.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datashuttle/tui/shared/validate_content.py b/datashuttle/tui/shared/validate_content.py index 949e8494c..ede87a31d 100644 --- a/datashuttle/tui/shared/validate_content.py +++ b/datashuttle/tui/shared/validate_content.py @@ -155,7 +155,7 @@ def on_button_pressed(self, event: Button.Pressed) -> None: ) if not success: self.parent_class.mainwindow.show_modal_error_dialog( - cast(str, output) # noqa + cast("str", output) ) else: self.write_results_to_richlog(output) From 9caf34c0cfda6cc1a246dc5144b025d258ccdf3c Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 14 May 2025 16:24:08 +0100 Subject: [PATCH 4/9] Add linux / macos case. --- datashuttle/utils/rclone.py | 45 +++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/datashuttle/utils/rclone.py b/datashuttle/utils/rclone.py index c52ef3816..d5365b1f2 100644 --- a/datashuttle/utils/rclone.py +++ b/datashuttle/utils/rclone.py @@ -24,38 +24,35 @@ def call_rclone(command: str, pipe_std: bool = False) -> CompletedProcess: """ command = "rclone " + command - if platform.system() == "Windows": - # Write command to a temporary .bat script - with tempfile.NamedTemporaryFile( - mode="w", suffix=".bat", delete=False - ) as tmp_script: - tmp_script.write(command) - tmp_script_path = tmp_script.name - - try: - if pipe_std: - output = subprocess.run( - [tmp_script_path], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - shell=True, - ) - else: - output = subprocess.run([tmp_script_path], shell=True) - finally: - os.remove(tmp_script_path) # Clean up temp script + system = platform.system() + if system == "Windows": + suffix = ".bat" else: - # On non-Windows, just run it normally + suffix = ".sh" + + # Write the command to a temporary script file + with tempfile.NamedTemporaryFile( + mode="w", suffix=suffix, delete=False + ) as tmp_script: + tmp_script.write(command) + tmp_script_path = tmp_script.name + + try: + if system != "Windows": + os.chmod(tmp_script_path, 0o700) + if pipe_std: output = subprocess.run( - command, + [tmp_script_path], stdout=subprocess.PIPE, stderr=subprocess.PIPE, - shell=True, + shell=False, ) else: - output = subprocess.run(command, shell=True) + output = subprocess.run([tmp_script_path], shell=False) + finally: + os.remove(tmp_script_path) return output From aa26c3bae727a9691c09bac5d117d5609e2cf2f3 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 14 May 2025 16:46:50 +0100 Subject: [PATCH 5/9] Split functions, legacy and now through script. --- datashuttle/utils/rclone.py | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/datashuttle/utils/rclone.py b/datashuttle/utils/rclone.py index d5365b1f2..66fd475f7 100644 --- a/datashuttle/utils/rclone.py +++ b/datashuttle/utils/rclone.py @@ -23,6 +23,19 @@ def call_rclone(command: str, pipe_std: bool = False) -> CompletedProcess: pipe_std: if True, do not output anything to stdout. """ command = "rclone " + command + if pipe_std: + output = subprocess.run( + command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True + ) + else: + output = subprocess.run(command, shell=True) + + return output + + +def call_rclone_through_script(command: str) -> CompletedProcess: + """ """ + command = "rclone " + command system = platform.system() @@ -42,15 +55,13 @@ def call_rclone(command: str, pipe_std: bool = False) -> CompletedProcess: if system != "Windows": os.chmod(tmp_script_path, 0o700) - if pipe_std: - output = subprocess.run( - [tmp_script_path], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - shell=False, - ) - else: - output = subprocess.run([tmp_script_path], shell=False) + output = subprocess.run( + [tmp_script_path], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + shell=False, + ) + finally: os.remove(tmp_script_path) @@ -216,19 +227,17 @@ def transfer_data( extra_arguments = handle_rclone_arguments(rclone_options, include_list) if upload_or_download == "upload": - output = call_rclone( + output = call_rclone_through_script( f"{rclone_args('copy')} " f'"{local_filepath}" "{cfg.get_rclone_config_name()}:' f'{central_filepath}" {extra_arguments}', - pipe_std=True, ) elif upload_or_download == "download": - output = call_rclone( + output = call_rclone_through_script( f"{rclone_args('copy')} " f'"{cfg.get_rclone_config_name()}:' f'{central_filepath}" "{local_filepath}" {extra_arguments}', - pipe_std=True, ) return output @@ -299,6 +308,7 @@ def assert_rclone_check_output_is_as_expected(result, symbol, convert_symbols): case is untested and a test case is required. Once the test case is obtained this should most likely be moved to tests. """ + breakpoint() assert result[1] == " ", ( "`rclone check` output does not contain a " "space as the second character`." From aa4cf6bbb55a3aa38aff3bd4b3a74bc2eab57866 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 14 May 2025 20:15:38 +0100 Subject: [PATCH 6/9] Fix transfer on linux / macos. --- datashuttle/utils/rclone.py | 1 + 1 file changed, 1 insertion(+) diff --git a/datashuttle/utils/rclone.py b/datashuttle/utils/rclone.py index 66fd475f7..59a2278c0 100644 --- a/datashuttle/utils/rclone.py +++ b/datashuttle/utils/rclone.py @@ -43,6 +43,7 @@ def call_rclone_through_script(command: str) -> CompletedProcess: suffix = ".bat" else: suffix = ".sh" + command = "#!/bin/bash\n" + command # Write the command to a temporary script file with tempfile.NamedTemporaryFile( From 4fe6514b97ac02bdcf8519f180c5702d938b07d3 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Wed, 14 May 2025 20:28:08 +0100 Subject: [PATCH 7/9] Remove breakpoint. --- datashuttle/utils/rclone.py | 1 - 1 file changed, 1 deletion(-) diff --git a/datashuttle/utils/rclone.py b/datashuttle/utils/rclone.py index 59a2278c0..8827538cc 100644 --- a/datashuttle/utils/rclone.py +++ b/datashuttle/utils/rclone.py @@ -309,7 +309,6 @@ def assert_rclone_check_output_is_as_expected(result, symbol, convert_symbols): case is untested and a test case is required. Once the test case is obtained this should most likely be moved to tests. """ - breakpoint() assert result[1] == " ", ( "`rclone check` output does not contain a " "space as the second character`." From 99d005ac9013d4b956061a985fb53c789c1752f4 Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Thu, 22 May 2025 00:11:43 +0100 Subject: [PATCH 8/9] Fix tests. --- tests/tests_integration/test_datatypes.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/tests_integration/test_datatypes.py b/tests/tests_integration/test_datatypes.py index 93288ec97..1e1776e41 100644 --- a/tests/tests_integration/test_datatypes.py +++ b/tests/tests_integration/test_datatypes.py @@ -63,12 +63,10 @@ def test_transfer_datatypes( # Unfortunately on Windows we are encountering 'The command line is too long' # and so cannot test against all datatypes here. - some_narrow_datatypes = ( - canonical_configs.quick_get_narrow_datatypes() - ) # [:10] + narrow_datatypes = canonical_configs.quick_get_narrow_datatypes() datatypes_used = self.get_narrow_only_datatypes_used(used=False) - for key in some_narrow_datatypes: + for key in narrow_datatypes: datatypes_used[key] = True test_utils.make_and_check_local_project_folders( @@ -76,7 +74,7 @@ def test_transfer_datatypes( "rawdata", subs, sessions, - some_narrow_datatypes, + narrow_datatypes, datatypes_used, ) @@ -87,7 +85,7 @@ def test_transfer_datatypes( project, upload_or_download, "custom", "rawdata" ) - transfer_function("rawdata", "all", "all", some_narrow_datatypes) + transfer_function("rawdata", "all", "all", narrow_datatypes) test_utils.check_folder_tree_is_correct( os.path.join(base_path_to_check, "rawdata"), From f7f19d5a6157609d70f62643d8b5153d10bd452c Mon Sep 17 00:00:00 2001 From: JoeZiminski Date: Thu, 22 May 2025 00:24:40 +0100 Subject: [PATCH 9/9] Tidy up. --- datashuttle/utils/rclone.py | 48 ++++++++++--------------------------- 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/datashuttle/utils/rclone.py b/datashuttle/utils/rclone.py index 26c9b5196..49d7da826 100644 --- a/datashuttle/utils/rclone.py +++ b/datashuttle/utils/rclone.py @@ -23,56 +23,32 @@ def call_rclone(command: str, pipe_std: bool = False) -> CompletedProcess: pipe_std: if True, do not output anything to stdout. """ command = "rclone " + command - - if platform.system() == "Windows": - # Write command to a temporary .bat script - with tempfile.NamedTemporaryFile( - mode="w", suffix=".bat", delete=False - ) as tmp_script: - tmp_script.write(command) - tmp_script_path = tmp_script.name - - try: - if pipe_std: - output = subprocess.run( - [tmp_script_path], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - shell=True, - ) - else: - output = subprocess.run([tmp_script_path], shell=True) - finally: - os.remove(tmp_script_path) # Clean up temp script - + if pipe_std: + output = subprocess.run( + command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True + ) else: - # On non-Windows, just run it normally - if pipe_std: - output = subprocess.run( - command, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - shell=True, - ) - else: - output = subprocess.run(command, shell=True) + output = subprocess.run(command, shell=True) return output def call_rclone_through_script(command: str) -> CompletedProcess: - """ """ - command = "rclone " + command - + """ + Call rclone through a script, to avoid limits on command-line calls + (in particular on Windows). Used for transfers due to generation of + large call strings. + """ system = platform.system() + command = "rclone " + command + if system == "Windows": suffix = ".bat" else: suffix = ".sh" command = "#!/bin/bash\n" + command - # Write the command to a temporary script file with tempfile.NamedTemporaryFile( mode="w", suffix=suffix, delete=False ) as tmp_script: