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

(#747) Switch CryptoHashProvider to filestream for hashing files #2714

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

TheCakeIsNaOH
Copy link
Member

Description Of Changes

This changes the hash_file method to use a filestream which is passed to ComputeHash, which
should allow arbitrary file sizes with better memory usage.

The unit test for this method had to be removed because the mock
filesystem does not allow opening a file stream.

Motivation and Context

Previously, when getting the checksum of a file, this read the file
into a byte array to pass to ComputeHash. However, this was limited to
files of 2gb or less, and was not efficent memory wise.

Testing

Ran integration and units tests.

Validated that the warning/error about the file being too large was resolved this way:

  • Created a .nupkg over 2gb
  • Installed with choco 1.1.0 and validated the the warning/error did show up
  • Installed with debug choco like this: .\choco.exe install davinci-resolve -s . --verbose --debug --skippowershell
  • Validated that the error/warning did NOT show up for debug choco
  • Went into .chocolatey\davinci-resolve\.files and validated that the .nupkg had the correct md5 checksum recorded.

Change Types Made

  • Bug fix (non-breaking change)
  • Feature / Enhancement (non-breaking change)
  • Breaking change (fix or feature that could cause existing functionality to change)

Related Issue

Fixes #747

Change Checklist

  • Requires a change to the documentation
  • Documentation has been updated
  • Tests to cover my changes, have been added
  • All new and existing tests passed.
  • PowerShell v2 compatibility checked.

@TheCakeIsNaOH TheCakeIsNaOH changed the title (#747) Switch CryptoHashProvider filestream for hashing files (#747) Switch CryptoHashProvider to filestream for hashing files May 7, 2022
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the stream-hashes branch 2 times, most recently from 29ba825 to b3a8f3c Compare June 27, 2022 15:33
@coveralls
Copy link

coveralls commented Jun 27, 2022

Coverage Status

Coverage remained the same at 27.311% when pulling 0123c01 on TheCakeIsNaOH:stream-hashes into 3892cfb on chocolatey:develop.

…files

Previously, when getting the checksum of a file, this read the file
into a byte array to pass to ComputeHash. However, this was limited to
files of 2gb or less, and was not efficent memory wise. This changes
the method to using a filestream which is passed to ComputeHash, which
should allow arbitrary file sizes with better memory usage.

The unit test for this method had to be removed because the mock
filesystem does not allow opening a file stream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support streaming hashes
2 participants