Skip to content
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

Only upload file to device if remote is different #107

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jdjjm
Copy link
Contributor

@jdjjm jdjjm commented Jun 17, 2020

This PR adds the ability to check if the remote file is different to the local file on upload. If its the same it can skip uploading to save time.
For a single file upload this will increase the time to upload it takes a bit under a second to check if they are different plus the time to upload. However, when working on larger projects with multiple files, if you are only modifying a couple at a time it can speed up the upload process as it will only upload the files that need to be uploaded. It also means you don't need to upload each file individually.

You have to add the --different option to the script call otherwise it will use the default to uploading all files.

Just wanted everyone's opinion on whether they see themselves using this or just prefer the old way.

If it was to be implemented I would recommend the run configuration to have a checkbox on whether it is enabled or not. I would also recommend disabling it for single file uploads and save it for project level or other multi-file uploads.

Gets a hash of the remote file and compares it to a hash of the local file. If they are different it uploads. If not it skips over.
@jdjjm
Copy link
Contributor Author

jdjjm commented Jun 18, 2020

Found a few bugs with this PR so I'll fix them when I have a chance.

@vlasovskikh
Copy link
Contributor

@jdjjm Thanks for working on this! It's an interesting idea. Let's see if it brings the benefits of faster uploading without adding bugs or too much complexity to the code base. I'll mark this PR as draft while you're working on it. Feel free to convert it back and/or ask me to review it.

@vlasovskikh
Copy link
Contributor

For a single file upload this will increase the time to upload it takes a bit under a second to check if they are different plus the time to upload.

@jdjjm You can improve it further. If you check if the files are of different size, then it makes no sense to continue with the hash, since you already know it differs. So even if you upload a single file (that has been edited and changed its size), you can skip calculating the hash.

@jdjjm
Copy link
Contributor Author

jdjjm commented Jun 18, 2020

I hadn't thought of that but that is a good idea. I think os.stat might be implemented in uos so that could be an easy way to get the file size.

Fixed windows path issue when finding the remote path. 
Used ubinascii to get hexdigest as string when finding hash to fix bug when hashing certain hex values.
@jdjjm jdjjm marked this pull request as ready for review June 22, 2020 02:59
Removed print commands used for development
@jdjjm
Copy link
Contributor Author

jdjjm commented Jun 22, 2020

@vlasovskikh PR is ready for testing. Let me know if you have any issues and remember you need to set the --different flag to enable it.

@vlasovskikh vlasovskikh self-assigned this Jun 22, 2020
Removed some debug print statements I forgot to remove.
@marcelocrispim
Copy link

marcelocrispim commented Jul 13, 2020

added support for esp32

seguindo seu conselho, estou tentando aprender kotlin.
thank you very much

Edit by @vlasovskikh: I've removed the link to the *.zip file since this file is not the official version of intellij-micropython.

@jdjjm
Copy link
Contributor Author

jdjjm commented Jul 13, 2020

@marcelocrispim If you have changes you have worked on please create a new pull request for it to be reviewed. This pull request is only for discussing what the title outlines.

@vlasovskikh
Copy link
Contributor

@marcelocrispim Could you please send your changes as a separate PR? Also I would like to kindly ask you not to publish non-official builds of the intellij-micropython plugin with the name of the official plugin and select a different name instead. "intellij-micropython-1.1.2" sends a wrong message that it may be the next official release of intellij-micropython while it isn't.

@marcelocrispim
Copy link

@vlasovskikh sorry, I will make the pull request over the weekend

best Regards

Comment on lines +147 to +157
remote_file_size = files.get_size(remote_path)
if local_file_size == remote_file_size:
remote_file_hash = files.get_hash(remote_path)
if remote_file_hash == local_file_hash:
print("File Identical... Skipping upload", file=sys.stderr, flush=True)
else:
wait_for_board()
files.put(remote_path, raw)
else:
wait_for_board()
files.put(remote_path, raw)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify to something like,

Suggested change
remote_file_size = files.get_size(remote_path)
if local_file_size == remote_file_size:
remote_file_hash = files.get_hash(remote_path)
if remote_file_hash == local_file_hash:
print("File Identical... Skipping upload", file=sys.stderr, flush=True)
else:
wait_for_board()
files.put(remote_path, raw)
else:
wait_for_board()
files.put(remote_path, raw)
if local_file_size == files.get_size(remote_path) and local_file_hash == files.get_hash(remote_path):
print("File Identical... Skipping upload", file=sys.stderr, flush=True)
else:
wait_for_board()
files.put(remote_path, raw)
else:
files.put(remote_path, fd.read())

The interpreter will short-circuit if the size check local_file_size == files.get_size(remote_path) evaluates false, and not calculate the hash too.

@Gor-Ren
Copy link

Gor-Ren commented Jul 19, 2020

Just wanted everyone's opinion on whether they see themselves using this or just prefer the old way.

It sounds like a good enhancement 😄 I have some big SSL certificates that take a while to re-upload, so I find myself cherrypicking only files that have changed to flash. This would automate that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants