Fixes package linking when a filename casing changes #3843
Conversation
|
Damn, the tests work fine on OSX, but apparently not on Linux. I'll need to check this at home. |
src/package-linker.js
Outdated
| @@ -296,7 +296,17 @@ export default class PackageLinker { | |||
| // remove all extraneous files that weren't in the tree | |||
| for (const loc of possibleExtraneous) { | |||
| this.reporter.verbose(this.reporter.lang('verboseFileRemoveExtraneous', loc)); | |||
| await fs.unlink(loc); | |||
|
|
|||
There was a problem hiding this comment.
Can we just ignore case in fs.buildActionsForCopy instead?
fs.js: 153
for (const loc of possibleExtraneous) {
if (files.has(loc)) {
possibleExtraneous.delete(loc);
}
}
Just call toLocaleLowerCase() when dealing with files Set.
Same thing should be applied to buildActionsForHardlink function
There was a problem hiding this comment.
Hmm maybe ... do you think the following would work?
files.add(loc);to
files.add(loc);
files.add(loc.toLowerCase());There was a problem hiding this comment.
This way, the .has() checks wouldn't have to change. As far as I see, the files set doesn't look used apart from those checks, so maybe it's enough?
There was a problem hiding this comment.
files set is used just in 4 places.
So yeah, I think doing files.add(loc.toLowerCase()); and file.has(loc.toLowerCase()) should work
There was a problem hiding this comment.
I don't think you need to add loc twice, that would cause confusion
src/package-linker.js
Outdated
| @@ -296,7 +296,17 @@ export default class PackageLinker { | |||
| // remove all extraneous files that weren't in the tree | |||
| for (const loc of possibleExtraneous) { | |||
| this.reporter.verbose(this.reporter.lang('verboseFileRemoveExtraneous', loc)); | |||
| await fs.unlink(loc); | |||
|
|
|||
| const base = path.basename(loc); | |||
src/package-linker.js
Outdated
| // we need to do this because of case-insensitive file systems, where deleting a.js will delete A.js | ||
| for (let file of await fs.readdir(dir)) { | ||
| if (base === file) { | ||
| await fs.unlink(path.join(dir, file)); |
There was a problem hiding this comment.
Do we need to do this sequentially? If not we can probably do Promise.all() and remove all in parallel.
src/util/fs.js
Outdated
| @@ -532,7 +532,11 @@ export async function copyBulk( | |||
| const cleanup = () => delete currentlyWriting[data.dest]; | |||
| reporter.verbose(reporter.lang('verboseFileCopy', data.src, data.dest)); | |||
| return (currentlyWriting[data.dest] = readFileBuffer(data.src) | |||
| .then(d => { | |||
| .then(async (d) => { | |||
There was a problem hiding this comment.
async in a then is a bit weird isn't it? May be we can refactor this piece to only use async/await in a future iteration?
There was a problem hiding this comment.
Yep, we don't seem to do anything concurrent here, so it could be refactored to use await
src/package-linker.js
Outdated
| // we iterate once again on every directory of our extraneous files to find out if they really exist | ||
| // we need to do this because of case-insensitive file systems, where deleting a.js will delete A.js | ||
| for (const file of await fs.readdir(dir)) { | ||
| if (base === file) { |
There was a problem hiding this comment.
Wouldn't this be a single file? If so why can't we use fs.exists directly?
| .then(async d => { | ||
| // we need to do this because of case-insensitive filesystems, which wouldn't properly | ||
| // change the file name in case of a file being renamed | ||
| await unlink(data.dest); |
There was a problem hiding this comment.
Great work, @arcanis.
I am a bit worried about the extra IO operation here.
What impact would it cause for normal installation?
Summary
Fixes an issue where files were being removed when their name casing changed (
State.js=>state.js). Only occured on case-insensitive filesystems (OSX, Windows).Test plan
Tests should pass, + a new test to make sure it doesn't happen again