Do not canonicalize path of persistent cache #29201
Conversation
This is consolidate how the location of the directory is handled between a PersistentCache and a SingleDepthFileAccessTracker.
|
@bot-gradle test QF please. |
|
I've triggered the following builds for you. Click here to see all build failures. |
| throw new UncheckedIOException(e); | ||
| } | ||
| DirCacheReference dirCacheReference = dirCaches.get(canonicalDir); | ||
| DirCacheReference dirCacheReference = dirCaches.get(cacheDir); |
There was a problem hiding this comment.
❓ Woudn't that make it a bit more fragile depending on how File path is constructed?
Since for example:
new File("./cache").equals(new File("cache")); // returns false
new File("./cache").getCanonicalFile().equals(new File("cache").getCanonicalFile()); // returns true
So dirCaches.get(canonicalDir) could return different results.
There was a problem hiding this comment.
Not sure if that is a problem in practice though for this part of code
There was a problem hiding this comment.
The canincalization was added in 2009, I don't think it matters. User code does not create these caches, so we control all creation. So if tests pass, I think we should be okay.
For the directory build cache there isn't any change either: we've been canonicalizing the directory build cache's location already, that's why it didn't cause any trouble (Maven didn't, that's how we found the discrepancy).
In theory, we could canonicalize everywhere where we resolve a location, but that'd be defensive coding.
|
Tests pass so I triggered the merge. Let me know if you think we should reconsider. |
This is consolidate how the location of the directory is handled between a
PersistentCacheand aSingleDepthFileAccessTracker. Before the change only the cache factory would canonicalize the location, and this could lead to the access tracker trying to track files under a different path. Now they both use the same path, and callers can decide whether they want to canonicalize or not.Reviewing cheatsheet
Before merging the PR, comments starting with