Add rclone install check only if call fails.

[JoeZiminski]

closes #560. Previously a check for rclone install was performed on datashuttle startup if the project did not exist. The assumption was the first time the program is used, if rclone is not installed the install can be reminded. However, this doesn't make much sense because new environments are often created, but this doesn't mean a new project will be made.

The problem with the rclone install check is that it spawns a subprocess to call rclone which is slow, so it cannot be arbitrarily called. The reason it is not always called at program start up is because it creates an annoying lag when opening a project.

The solution here is to only check the rclone install if the rclone call has actually failed. This means we can avoid the overhead of checking up-front, and only incur it if the rclone call has failed (in which case, a short delay is not an issue).

No docs are required for this, it would be ideal to test it but it would be quite complex in the CI (having to uninstall rclone in the test environment) and is not easily mocked because of the subprocess call. In this case no tests are added, it is not critical and will come up in general day-to-day test usage if it fails.

@cs7-shrey the only problem with this is it might cause issues for the Popen method of calling rclone? Maybe it is not possible to take this approach in that function?

[JoeZiminski]

This change just swaps the order of the functions so that the called function is under the calling function

[cs7-shrey]

Hey @JoeZiminski, this looks good. It is completely extendable to the Popen method (in the function that awaits the Popen call). However, I'm not sure of the performance benefits. When I ran it locally, the difference wasn't very noticeable. So, we might as well just keep the check that runs on DataShuttle __init__ because it seems handy in some cases for ex. when creating a new project (assuming the user just installed datashuttle and is making their first project).

Checking rclone installed on failing commands is really good in general. I'm all in for that. However, #560 (the part where it returns sub-001 in case rclone is not installed), reflects a deeper problem and the "rclone not installed" is just one manifestation. So, checking if rclone installed or not would be more of a quick fix. Another way this problem manifests is, if you setup a project with SSH connection method and don't setup the connection (so that remote rclone calls fail), and then if you try to use suggest next sub or ses with include_central, it works just fine, essentially ignoring the remote.

Here's why I it happens,
the search_local_or_remote function that was recently added for search remote using rclone lsjson, doesn't raise error on non zero returncode and returns empty list for files and folders. If we can raise error on a non zero return code it would solve this issue. But if it breaks things elsewhere, we can just leave it as it is.

[JoeZiminski]

Thanks @cs7-shrey! There was an error in the previous implementation, the checking function called call_rclone again which led to infinite recursion, I wonder if this resulted in the error not being triggered when you tested. Would you mind trying again? I tested in the scenarios you tried (nice idea BTW to set up SSH but don't connect, I will use that in future) and it raised the error where expected. (for the first subject, it creates sub-001 as default, then afterwards attempts to search using rclone). I think in search_local_or_remoteit _should_ raise the error withincall_rcloneso execution should be stopped beforeoutput.returncode` is checked in the function body.

On Windows, spawning a new process is much slower than UNIX, in part because UNIX systems use a 'fork' method by default in which the new process shares the memory space of the old process unless it writes to it, in which case it is copied ('copy-on-write'). On windows, the method spawns an entirely new process with dedicated memory etc. which is slower. On my slightly older laptop, unfortunately this lag is quite noticeable and makes the start-up process of datashuttle a little jarring. It's a shame because it creates all kinds of problems, including having to think about #571. I wish the world used UNIX systems!

[cs7-shrey]

Hey @JoeZiminski, I might have missed out on trying that part somehow. I tried it now and it works fine. I agree on the unix/windows part, sorry i was a bit biased with my operating system. Also, what do you think about raising error in the search_local_or_remote in case call_rclone returns a non zero returncode or do you think it'd be better to handle in another PR?

[JoeZiminski]

Hey @cs7-shrey sorry for the late reply, no worries I wouldn't have noticed if I wasn't on Windows! For search_local_or_remote I tried throwing an error if no files or folders were returned, but it led to test failures, I think it is okay if certain directories are searched but contain no files / folders that match the search criteria. In terms of return codes, these can be handled in another PR.

This is mainly related to transfers, return codes, at the moment the working mode is for the transfer to run and the user checks the logs to see whether transfer was successful. This is not ideal and should be handled better (#432, #536) but explicitly handling error codes can be left until these issues. We can think about this suggest_next_sub_or_ses case at the same time as handling this