Skip to content
This repository has been archived by the owner on Oct 24, 2022. It is now read-only.

Add a file hashing utility to the Utilities class. #234

Merged
merged 3 commits into from
Oct 27, 2016

Conversation

jkinkead
Copy link
Contributor

This makes cache key generation no longer sensitive to file ordering, but adds sensitivity to file naming.

One more utility method for generating file hashes. This will be used a few times in the Docker image generation code.

This makes cache key generation no longer sensitive to file ordering,
but adds sensitivity to file naming.
Copy link
Contributor

@ckarenz ckarenz left a comment

Choose a reason for hiding this comment

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

LGTM! One comment asking for an additional test.

def hashFiles(filesToHash: Seq[File], rootDir: File): String = {
// Resolve the filenames relative to the root directory.
val rootDirPath = rootDir.toPath
val relativizedNames = filesToHash.map(_.toPath).map(rootDirPath.relativize).map(_.toString)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume relativize will create a canonical path? If I feed it "foo/dir/../dir" what happens? Probably add a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, yes.

Test added!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you were asking another thing! I'll check that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah-ha! It does not. Good catch; I'll add a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another test added. :)

I'll merge on green, unless I hear otherwise!

@jkinkead
Copy link
Contributor Author

Actually, there is no semaphore integration here, so I will just merge one I run the tests again locally!

@jkinkead jkinkead merged commit 351df64 into allenai:master Oct 27, 2016
@jkinkead jkinkead deleted the hash_utility branch October 27, 2016 18:36
tempDirectory.getName + File.pathSeparatorChar + "bar.txt")

val firstHash = Utilities.hashFiles(Seq(fooFile, barFile), tempDirectory)
val secondHash = Utilities.hashFiles(Seq(fooFile, barFile), tempDirectory)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the second hash attempt use a different (denormalized) path to barFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah! Indeed. I'll send a follow-up.

jkinkead added a commit to jkinkead/sbt-plugins that referenced this pull request Oct 28, 2016
* Add a file hashing utility to the Utilities class.

This makes cache key generation no longer sensitive to file ordering,
but adds sensitivity to file naming.

* Create a unit test exercising path relativization.

* Add logic to normalize paths before hashing, and a test exercising this.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants