Fix command line too long on windows.#509
Conversation
20fb741 to
aa26c3b
Compare
…atics-unit/datashuttle into fix_windows_command_too_long
cs7-shrey
left a comment
There was a problem hiding this comment.
Hey @JoeZiminski, all looks good. I have added a comment on one edge case that we might encounter.
| if system != "Windows": | ||
| os.chmod(tmp_script_path, 0o700) |
There was a problem hiding this comment.
This looks good and I think will work in most cases but there might be some really unique edge cases wherein the chmod might fail. I'm not sure how or when would this happen as logically the program created the script file and should be able to chmod on it but it might fail in some unique system configuration.
I think instead of completely replacing the call_rclone function with call_rclone_through_script, we can have a function like call_rclone_safe that can check if the platform is windows and then execute call_rclone_through_script. Since the call_rclone works fine on unix/linux, I think we can still keep using it unless in future we need to call_rclone_through_script on these systems as well.
There was a problem hiding this comment.
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 chmod command outside of the current try/except and doing:
if system != "Windows":
try:
os.chmod(tmp_script_path, 0o700)
except PermissionError:
return call_rclone(command, ....)
So basically, fall back to call_rclone if permissions can't be set (we would need to change the variable names for the overwritten command). This would still fail if the user had a very long command and did not have permission to write, but this should be a very rare case.
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.
There was a problem hiding this comment.
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 chmod command here when it breaks anything. It won't be a good idea to over engineer things at this point so I think we could keep it as it is and tend to it when the need be.
There was a problem hiding this comment.
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.
On windows, the command line arguments are limited to 32,768 characters. For rclone we are building huge command line commands which can quickly surpass this limit. This PR creates a new function that calls rclone by writes the command to temporary file (
.baton windows,.shon macOS / linux) before running.This new function is only used for transfer, a) to avoid overhead on all other rclone calls which are short b) it changes the
stdoutfrom the function call, making it harder to pass and breaking existing functionality that reads rclone outputs.