v8 release

[manidlou]

I think it is time to release v8. @jprichardson @RyanZim @JPeer264 please let me know if you think differently!

graceful-fs has not been updated yet to support bigint option. I opened an issue there, but they haven't been updated their code yet! So, switched to built-in fs for copy*() and move*() for now, so that we can use bigint option. We will switch back to graceful-fs once the upstream is updated

• removed standard-markdown as it started giving us more headaches than benefits!

copy*()

• use fs.stat() with bigint option for node versions that support it >= 10.5.0 and use old fs.stat() without bigint for older node versions.
• only throw Source and destination must not be the same error if src and dest have the same ino and exist on the same device.

move*()

• refactored to use the new internal stat functions.
• added test for prevent moving identical files and directories.

Fixes #608, #613, #614, #657.

[manidlou]

I will rebase this!

[Domvel]

@manidlou Thanks for your changes. checkPathsBeforeCopying is by default true, right? And overwrite is by default true.
Usually the overwrite option is by default false. We should set checkPathsBeforeCopying by default to false. Especially during the #657 bug exists. (Note: The #657 bug is about Inode big integer AND copy between Volumes. This means, the bug is not fixed with your changes. Only by the option to disable the check.)

Recommended default values:

{
  "overwrite": false,
  "checkPathsBeforeCopying": false
}

[RyanZim]

@manidlou Looks like somehow you managed to duplicate most of the commits in this branch. Please fix.

[manidlou]

@RyanZim yeah i know! I will fix it for sure!

[manidlou]

fixed the commits!

[FlorinSerban]

Hi,
Can someone tell me when we will have this fix in the master branch?
I can't use Cordova 9.0.0 until this issue will be fixed.

[Domvel]

This issue does not fix the issue directly. It's a option to disable the checkPaths-method. This is more a workaround. The Inode-Issue still exists with the option enabled. I think that was not quite understood. But please correct me if I am wrong. 🙂

This finally means, Cordova has to update fs-extra and then use this option. I'm sure, this takes a while. But anyway, this "fix" should be published as fast as possible.

[dimitriscsd]

How is this coming along? Cordova will need to make a new release with your version 8 to fix major issues in windows build servers.

[f5cs]

It's a very ugly issue, and because it hasn't been solved for so long, i think this project is not a reliable solution.

Can the Cordova project team consider alternative solutions to node-fs-extra

[RyanZim]

Sorry for the long wait. I've been very busy, plus suffering from a bit of burnout.

[RyanZim]

@manidlou Do you want me to help with releasing v8, or do you want to do it yourself? I'm good with it either way.

[Domvel]

@RyanZim I dream of a vast green landscape where I spend my remaining life. No stress. No technology. Just sit by a lake and look up at the sky and sleep under the stars. You too? #burnout 😄 Shape the world.

[manidlou]

@Domvel we are very well aware of the issue. As I mentioned in the description, we use graceful-fs as a dependency and unfortunately it still doesn't support bigint option! Although I opened an issue there but they have been unresponsive so far!

Honestly I've been thinking about this a lot for awhile and I personally like to fix this right and the only reason that I went down the road of adding the checkPathsBeforeCopying option as a workaround, was that graceful-fs doesn't support the bigint yet! I personally believe copy should be able to handle this cases internally without any extra options. The way that I prefer to fix this is by using the bigint option for node versions that support it and fallback to the legacy approach.

So @jprichardson @RyanZim @JPeer264 although I myself worked on this workaround, this is how I suggest to handle this:

• disable graceful-fs for now and switch to the built-in fs so that we have the fs.stat() with bigint option available in node versions >= 10.5.0. We will switch back to the graceful-fs once the upstream update their code.
• use fs.stat() with bigint for node versions that support it and use old fs.stat() for older versions.

Also @Domvel copy will continue work across devices since I will implement the fix in a way that we only throw source and destination are the same error if src and dest have the same inode and they exist on the same device.

[manidlou]

Alright it should be ready now! updated the description as well! Once it is approved, @RyanZim will you please take care of the changelog and I will do the rest of it? 😃

[manidlou]

@jprichardson @RyanZim @JPeer264 please let me know if you have any concerns about this 😃

[JPeer264]

Sorry for the long wait. I've been very busy, plus suffering from a bit of burnout.

@RyanZim best wishes and hope you will get better :)

---

I'm okay with those changes. It's good that you marked the graceful-fs as a todo so we won't forget ;)
Also sorry for not being that active around here.

[RyanZim]

Items from this PR that should be in the release notes:

---

• Use renameSync() under the hood in moveSync()
• Fix bug with bind-mounted directories in copy*() (#613, #618)
• Fix bug in move() with case-insensitive file systems
• Use fs.stat()'s bigint option in copy*() & move*() where possible (#657)

---

@manidlou Did I miss anything?

We should also drop Node 6. No need to make code changes for it, just drop it in the release notes.

[dimitriscsd]

If you drop node 6 Cordova will not be able to use your new version to fix the bug as that would be a breaking change. You have to support node 6 Please read this: https://github.com/apache/cordova-common/issues/64#issuecomment-490502763

[brodybits]

We should also drop Node 6. No need to make code changes for it, just drop it in the release notes.

I would favor a statement that Node.js version 6 is deprecated and will be dropped in the near future. I would also favor a similar statement for Node.js version 8, considering that its EOL is coming up in December.

As @dimitriscsd said, Cordova still needs to support Node.js 6 until we make another major release.

[jprichardson]

Let's put out a statement that we'll drop Node v6 in the future as it's deprecated. I think this version of fs-extra should support Node v6.

[manidlou]

@RyanZim the items from this PR for release notes are correct. Thanks for taking care of that! Also i just saw your comment in the pile of comments! Hope you feel better!

[RyanZim]

@manidlou I went ahead and published v8.0.0.

[manidlou]

@RyanZim you did the right thing! Thanks for taking care of that!

[mbargiel]

So @jprichardson @RyanZim @JPeer264 although I myself worked on this workaround, this is how I suggest to handle this:

• disable graceful-fs for now and switch to the built-in fs so that we have the fs.stat() with bigint option available in node versions >= 10.5.0. We will switch back to the graceful-fs once the upstream update their code.
• use fs.stat() with bigint for node versions that support it and use old fs.stat() for older versions.

Also @Domvel copy will continue work across devices since I will implement the fix in a way that we only throw source and destination are the same error if src and dest have the same inode and they exist on the same device.

Sorry, a bit late to the party... Does this mean that on node < 10.5, there is still a random chance of failure due to the bigint inode problem? In other words, that this is fixed only for node 10.5 and above? (That's both what I understand from the source code, and what I seemed to have experienced myself.)

[mbargiel]

Note: I understand there's no magical solution here and we just can't use 'bigint' with fs in Node < 10.5, but I was wondering whether there could be tighter heuristics for the legacy case.

I initially thought the suggestion from #657 (comment) could have merit, but I think it wouldn't work with hardlinks and would leak to corrupted files. Instead, relying also on other file stats could help detect different files with incorrectly identical inode values (due to the Number representation). E.g. in legacy form, if the inodes match, but either one of the timestamps or the size don't, then allow the copy because we know it's not the same file, otherwise reject it because it may be the same file.

I could open a PR for that.

[jprichardson]

@mbargiel @RyanZim maybe we embed graceful-fs and fix it locally? We did this for all other libraries that were slow to respond.

[mbargiel]

@jprichardson Sure. Though my problem isn't with the fact that graceful-fs is temporarily not used, but that the fix for #657 is only valid for Node 10.5+ (so in practice, for the OP of that issue, the bug they reported is still present, because they stated they were running on Node v8.11.3).

[mbargiel]

We can move that conversation there, it would make much more sense than here on the v8.0.0 release PR :)