Skip to content

Checksums: use blocks to read the files#836

Merged
boegel merged 12 commits intoeasybuilders:developfrom
wpoely86:checksums
Feb 8, 2014
Merged

Checksums: use blocks to read the files#836
boegel merged 12 commits intoeasybuilders:developfrom
wpoely86:checksums

Conversation

@wpoely86
Copy link
Copy Markdown
Member

@wpoely86 wpoely86 commented Feb 5, 2014

We need to read a file in blocks to calculate the checksum because else, the memory blows up if you try to install icc.

This PR fixes it for sha1 and md5 but not yet for addler32 or crc32. I will try to fix them when I find the time.

When calculating the checksums of a large file, we need to read it into
blocks (currently 16MB) to keep memory usage acceptable.
Comment thread easybuild/tools/filetools.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this would be nicer if it was just on the next line...(starting at the (")

Comment thread easybuild/tools/filetools.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"parents", while you are cleaning up stuff.

@itkovian
Copy link
Copy Markdown
Contributor

itkovian commented Feb 5, 2014

Looks OK to me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

split off the hash functions from the dicts/lambda's. too much duplicated code

try:
    import hashlib
    md5_func=hashlib.md5
    sha1_func=hashlib.sha1
except ImportError:
    import md5, sha
    md5_func=md5.md5
    sha1_func=sha.sha
CHECKSUM_FUNCTIONS['md5'] = lambda p: calc_block_checksum(p, md5_func())
CHECKSUM_FUNCTIONS['sha1'] = lambda p: calc_block_checksum(p, sha1_func())

Comment thread easybuild/tools/filetools.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

class names are camelcase

Comment thread easybuild/tools/filetools.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do the try/except a bit higher, and all this in the dict above. this looks odd now.

@stdweird
Copy link
Copy Markdown
Contributor

stdweird commented Feb 6, 2014

@wpoely86 nice!
@boegel ready

Comment thread easybuild/tools/filetools.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

are you sure you need to call md5_func and sha1_func here? It seems like you just want to pass them along.
(did this pass the untit tests?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We call them to have a md5 or sha1 object to work with. It has passed the unit tests (the last one failed because the disk was full, @boegel was going to fix it).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so better to call the md5_class and sha1_class then? (and i wasn't going to make any more remarks...)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fair point, the would be more correct.

@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 8, 2014

Thanks @wpoely86, thanks for the reviewing @itkovian, @stdweird, @JensTimmerman!

boegel added a commit that referenced this pull request Feb 8, 2014
Checksums: use blocks to read the files
@boegel boegel merged commit c76599c into easybuilders:develop Feb 8, 2014
@wpoely86 wpoely86 deleted the checksums branch February 9, 2014 19:38
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.

5 participants