-
Notifications
You must be signed in to change notification settings - Fork 38
Fix command line too long on windows. #509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
35b2944
f02e5f9
d2a1078
9caf34c
aa26c3b
aa4cf6b
4fe6514
c66392d
99d005a
f7f19d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
|
@@ -30,6 +33,45 @@ def call_rclone(command: str, pipe_std: bool = False) -> CompletedProcess: | |
| return output | ||
|
|
||
|
|
||
| def call_rclone_through_script(command: str) -> CompletedProcess: | ||
| """ | ||
| 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 | ||
|
|
||
| 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) | ||
|
Comment on lines
+59
to
+60
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks good and I think will work in most cases but there might be some really unique edge cases wherein the I think instead of completely replacing the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @cs7-shrey thanks for this, this is a good point. It doesn't make sense to fail just because permissions can't be set here. I checked a little deeper further and see on macOS the max character limit is 256kb for the command line, which is about 65k characters (assuming ASCII encoding). For a very rough 'quite long' path scenario of say 120 chars this would be 541 paths, which is feasible to have for a larger project. As such it would be good to use this for macOS too (and we might as well apply to Linux too for consistency). To have the best of both worlds, what do you think of moving the So basically, fall back to There is probably too much going on in this function, trying to handle windows and macOS/Linux together, but I'm not sure if its worth the indirection to break it apart yet.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks great, handling permission error and calling_clone directly. On the other hand, I realise that we can omit this altogether so as to know about the behaviour of the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great I agree on balance that makes sense, let's leave it and it people run into the error there might be another more elegant way to deal with it, or it will highlight some use case we have not considered. |
||
|
|
||
| output = subprocess.run( | ||
| [tmp_script_path], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| shell=False, | ||
| ) | ||
|
|
||
| finally: | ||
| os.remove(tmp_script_path) | ||
|
|
||
| return output | ||
|
|
||
|
|
||
| # ----------------------------------------------------------------------------- | ||
| # Setup | ||
| # ----------------------------------------------------------------------------- | ||
|
|
@@ -189,19 +231,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 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.