Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EEXIST error when building kibana a second time #34852

Merged
merged 9 commits into from May 1, 2019

Conversation

Projects
None yet
5 participants
@skaapgif
Copy link
Contributor

commented Apr 10, 2019

Summary

Building Kibana for the second time fails with:

│ERROR Error: EEXIST: file already exists, copyfile '/Users/rudolf/dev/kibana/.node_binaries/10.15.2/node.exe' -> '/Users/rudolf/dev/kibana/.node_binaries/10.15.2/windows-x64/node.exe'

This PR unlinks the destination before attempting to do a copy-on-write to prevent these failures.

This PR adds .node_binaries to kbn cleanup and moves the very specialised fs.copy() function into the calling code.

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 10, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 10, 2019

@jbudz

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Test failures look legit :)

@tylersmalley

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Any steps to reproduce? During the build, we remove the entire target/build directories.

@skaapgif

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

To reproduce:
yarn build && yarn build

The clean task doesn't remove files in .node_binaries/ which is where this problem originates. @tylersmalley would you prefer if we removed that directory too as part of the cleanup task?

@spalger

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

I'm kinda suspicious of this, https://nodejs.org/api/fs.html#fs_fs_copyfile_src_dest_flags_callback implies that this is not the behavior of fs.constants.COPYFILE_FICLONE:

  • fs.constants.COPYFILE_EXCL is a thing with the description "The copy operation will fail if dest already exists"
  • Their example is fs.constants.COPYFILE_EXCL | fs.constants.COPYFILE_FICLONE to clone a file exclusively
@skaapgif

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@spalger I agree, this is not the behaviour that the documentation suggests, I've created nodejs/node#27273

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 17, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 17, 2019

// Delete `destination` since `fs.copyFile` with the copy-on-write reflink/COPYFILE_FICLONE
// flag fails if the destination already exists. https://github.com/nodejs/node/issues/27273
try {
await unlinkAsync(destination);

This comment has been minimized.

Copy link
@tylersmalley

tylersmalley Apr 17, 2019

Member

Is this still necicary after cleaning the .node_binaries directory? My concern is the overhead of an additional delete call before any copy when it's not needed.

This comment has been minimized.

Copy link
@skaapgif

skaapgif Apr 17, 2019

Author Contributor

cleaning node_binaries solves the original problem, so we don't need to add the unlink call.

I kept it there since it felt like a bug in our fs.copy() implementation, because we abstract away how we do the copy, I would expect this to behave like Node's fs.copy and then it weirdly doesn't without it being obvious from the calling code that we're passing in COPYFILE_FICLONE.

Since the whole point of using COPYFILE_FICLONE is for performance it makes sense that we want to prevent unnecessary calls.

I'm thinking perhaps it's best to rename the function to copyOnWrite and add a comment explaining the behaviour, but removing the unlink call.

skaapgif added some commits Apr 17, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 29, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 29, 2019

@skaapgif skaapgif requested a review from tylersmalley Apr 30, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 30, 2019

@tylersmalley

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Looks like the failure is related

03:25:48   28:3  error  copy not found in './fs'  import/named
@elasticmachine

This comment has been minimized.

Copy link

commented May 1, 2019

@tylersmalley

This comment has been minimized.

Copy link
Member

commented May 1, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link

commented May 1, 2019

@tylersmalley
Copy link
Member

left a comment

LGTM, thanks!

@skaapgif skaapgif merged commit eb17289 into elastic:master May 1, 2019

2 checks passed

CLA All commits in pull request signed
Details
kibana-ci Build finished.
Details

@skaapgif skaapgif deleted the skaapgif:fix-re-build-error branch May 1, 2019

@skaapgif skaapgif changed the title Unlink destination before copy-on-write Remove shared fs.copy() function May 1, 2019

skaapgif added a commit to skaapgif/kibana that referenced this pull request May 1, 2019

Unlink destination before copy-on-write (elastic#34852)
* Unlink destination before copy-on-write

* Catch unlink exception if destination file doesn't exist

* Add link to upstream node issue

* Add .node_binaries to kbn cleanup

* Remove unlink call

* Remove fs.copy()

* Fix extract_node_builds_task test

* Remove copy from build/lib exports

@skaapgif skaapgif changed the title Remove shared fs.copy() function Fix EEXIST error when building kibana a second time May 1, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented May 1, 2019

@skaapgif skaapgif referenced this pull request May 2, 2019

Merged

Bind tasks' run function to prevent loosing this context #35933

0 of 7 tasks complete

skaapgif added a commit that referenced this pull request May 2, 2019

Unlink destination before copy-on-write (#34852) (#35884)
* Unlink destination before copy-on-write

* Catch unlink exception if destination file doesn't exist

* Add link to upstream node issue

* Add .node_binaries to kbn cleanup

* Remove unlink call

* Remove fs.copy()

* Fix extract_node_builds_task test

* Remove copy from build/lib exports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.