Use UV_FS_COPYFILE_FICLONE flag in fs.copyFile when available #6302

as-com · 2018-08-22T22:12:29Z

Summary

This flag increases performance and saves disk space on COW filesystems that support this flag (APFS, btrfs). On my machine running High Sierra, this decreases yarn install --offline time from 9.24s to 6.89s for a project with a lot of dependencies.

Test plan

yarn run test passes

Gudahtt · 2018-08-27T19:27:21Z

src/util/fs-normalized.js

@@ -38,7 +38,7 @@ export const unlink: (path: string) => Promise<void> = promisify(require('rimraf
 export const copyFile = async function(data: CopyFileAction, cleanup: () => mixed): Promise<void> {
 try {
 await unlink(data.dest);
- await copyFilePoly(data.src, data.dest, 0, data);
+ await copyFilePoly(data.src, data.dest, (fs.constants: any).COPYFILE_FICLONE || 0, data);


Hmm. This is unfortunate.

The flow error you're suppressing here is because Flow's built-in types for Node.js are missing this constant. I have submitted a PR to Flow to fix this, but even if that gets merged and released we can't immediately pull it in here. There were Flow errors introduced with a recent update that still need to be fixed first.

So in the meantime, bypassing this type check in some manner is appropriate I think. Typically this is done with a $FlowFixMe comment though, so that we know that we should come back and fix this at some point. I don't know what the maintainers would prefer here.

The benefit of using $FlowFixMe is that it would mark the line as something to fix later. You could also mention the PR that is required. The downside would be that it would suppress type errors elsewhere in this line.

It's probably worthwhile using the comment instead.

langpavel · 2018-09-06T00:00:47Z

Hooray! 🎉

Great work, I did't expect simple implementation like that really, but let's celebrate 🎈 and thank you!

Where is this used, it will apply on every yarn install, are there cases where files will be duplicated on COW file systems?
I'm asking for known cases that does not use src/util/fs-normalized.js:copyFile

manishmkr45 · 2018-09-26T10:40:12Z

HI
Getting this error -
error An unexpected error occurred: "Cannot read property 'COPYFILE_FICLONE' of undefined".

arcanis · 2018-09-26T10:48:57Z

Can you share your node version + system?

…

On Wed, Sep 26, 2018, 11:40 AM manishmkr45 ***@***.***> wrote: HI Getting this error - error An unexpected error occurred: "Cannot read property 'COPYFILE_FICLONE' of undefined". — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#6302 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_WayfHCjUW6KusCbVzonIjIOrsGHs4ks5ue1mogaJpZM4WIgMg> .

manishmkr45 · 2018-09-26T10:53:44Z

Can you share your node version + system?
…
On Wed, Sep 26, 2018, 11:40 AM manishmkr45 @.***> wrote: HI Getting this error - error An unexpected error occurred: "Cannot read property 'COPYFILE_FICLONE' of undefined". — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#6302 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_WayfHCjUW6KusCbVzonIjIOrsGHs4ks5ue1mogaJpZM4WIgMg .

Node version is 6.14.3
Linux system

craigjbass · 2018-09-26T11:07:17Z

I'm also seeing this problem as of an hour ago.

yarn install v1.10.0
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[91merror An unexpected error occurred: "Cannot read property 'COPYFILE_FICLONE' of undefined".
[0minfo If you think this is a bug, please open a bug report with the information provided in "/usr/src/app/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
[91mrails aborted!

We've also got

Yarn version: 
  1.11.0

Node version: 
  4.8.2

Platform: 
  linux x64

arcanis · 2018-09-26T11:24:13Z

I'm about to take a train so I can't investigate at the moment, but if someone can put up a PR with a fix I'll release a patch release as soon as I can.

…

On Wed, Sep 26, 2018, 12:07 PM Craig J. Bass ***@***.***> wrote: I'm also seeing this problem as of an hour ago. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#6302 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_Wawh4K9M3JNgw_f9S-9DmEJBCLuDPks5ue1_6gaJpZM4WIgMg> .

arcanis · 2018-09-26T11:45:32Z

All hail the train station free wifi: #6432

arcanis · 2018-09-26T15:47:14Z

Fixed in the 1.10.1, now available 🙂

…

On Wed, Sep 26, 2018, 1:45 PM Maël Nison ***@***.***> wrote: All hail the train station free wifi: #6432 <#6432> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6302 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_Wa07YVPHfZVsr86ak-izc8yZJzcMsks5ue2jygaJpZM4WIgMg> .

st-bender

Hi,
This change breaks yarn on some platforms, see for example #7152, #7440 .
The consequence being that also seemingly unrelated projects break: jupyterlab/jupyterlab#6969.
Cheers.

as-com added 2 commits August 22, 2018 17:48

Use UV_FS_COPYFILE_FICLONE flag in fs.copyFile when available

66b0d9e

Satisfy Flow

7390d17

Gudahtt reviewed Aug 27, 2018

View reviewed changes

Update fs-normalized.js

e0afa01

Gudahtt approved these changes Aug 28, 2018

View reviewed changes

arcanis merged commit 3c34587 into yarnpkg:master Sep 5, 2018

langpavel mentioned this pull request Sep 6, 2018

Use fs.copyFile with UV_FS_COPYFILE_FICLONE flag when ready #5456

Closed

2 tasks

arcanis mentioned this pull request Sep 26, 2018

Fixes constants #6432

Merged

craigjbass mentioned this pull request Sep 26, 2018

Lock yarn and nodejs versions alphagov/govwifi-admin#160

Merged

cristianl mentioned this pull request Nov 20, 2018

macOS binaries: UV_FS_COPYFILE_FICLONE not supported nodejs/node#24521

Closed

st-bender mentioned this pull request Sep 19, 2019

jupyter lab build fails with "EINVAL: invalid argument, copyfile..." (v1.0.4) jupyterlab/jupyterlab#6969

Closed

st-bender reviewed Sep 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use UV_FS_COPYFILE_FICLONE flag in fs.copyFile when available #6302

Use UV_FS_COPYFILE_FICLONE flag in fs.copyFile when available #6302

as-com commented Aug 22, 2018

Gudahtt Aug 27, 2018 •

edited

Loading

langpavel commented Sep 6, 2018 •

edited

Loading

manishmkr45 commented Sep 26, 2018

arcanis commented Sep 26, 2018 via email

manishmkr45 commented Sep 26, 2018

craigjbass commented Sep 26, 2018 •

edited

Loading

arcanis commented Sep 26, 2018 via email

arcanis commented Sep 26, 2018

arcanis commented Sep 26, 2018 via email

st-bender left a comment

Use UV_FS_COPYFILE_FICLONE flag in fs.copyFile when available #6302

Use UV_FS_COPYFILE_FICLONE flag in fs.copyFile when available #6302

Conversation

as-com commented Aug 22, 2018

Gudahtt Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

langpavel commented Sep 6, 2018 • edited Loading

manishmkr45 commented Sep 26, 2018

arcanis commented Sep 26, 2018 via email

manishmkr45 commented Sep 26, 2018

craigjbass commented Sep 26, 2018 • edited Loading

arcanis commented Sep 26, 2018 via email

arcanis commented Sep 26, 2018

arcanis commented Sep 26, 2018 via email

st-bender left a comment

Choose a reason for hiding this comment

Gudahtt Aug 27, 2018 •

edited

Loading

langpavel commented Sep 6, 2018 •

edited

Loading

craigjbass commented Sep 26, 2018 •

edited

Loading