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

Random build failures due to fs-extra defect #121

Open
oliversalzburg opened this issue Mar 5, 2019 · 26 comments

Comments

@oliversalzburg
Copy link

commented Mar 5, 2019

Bug Report

Problem

What is expected to happen?

You are able to build your Cordova project reliably.

What does actually happen?

Builds randomly fail with the error Source and destination must not be the same.

Information

The random failures are caused by a defect in fs-extra, which neglects to respect an inode-related issue in Node itself.

Command or Code

Every command that copies files with fs-extra is affected. cordova prepare is most likely to cause this.

Environment, Platform, Device

Windows 10

Version information

Cordova 8.1.2 (cordova-lib@8.1.1)

Checklist

  • I searched for existing GitHub issues
  • I updated all Cordova tooling to most recent version
  • I included all the necessary information above
@Domvel

This comment has been minimized.

Copy link

commented Mar 12, 2019

Same here:

Error: Source and destination must not be the same.
    at checkPaths (C:\Jenkins\workspace\%PROJECTNAME%\node_modules\fs-extra\lib\copy-sync\copy-sync.js:185:11)
    at Object.copySync (C:\Jenkins\workspace\%PROJECTNAME%\node_modules\fs-extra\lib\copy-sync\copy-sync.js:25:20)
    at updatePathWithStats (C:\Jenkins\workspace\%PROJECTNAME%\node_modules\cordova-common\src\FileUpdater.js:103:24)
    at C:\Jenkins\workspace\%PROJECTNAME%\node_modules\cordova-common\src\FileUpdater.js:298:19
    at Array.forEach (<anonymous>)
    at Object.mergeAndUpdateDir (C:\Jenkins\workspace\%PROJECTNAME%\node_modules\cordova-common\src\FileUpdater.js:296:33)
    at updateWww (C:\Jenkins\workspace\%PROJECTNAME%\platforms\android\cordova\lib\prepare.js:157:17)
    at Api.module.exports.prepare (C:\Jenkins\workspace\%PROJECTNAME%\platforms\android\cordova\lib\prepare.js:56:19)
    at Api.prepare (C:\Jenkins\workspace\%PROJECTNAME%\platforms\android\cordova\Api.js:177:45)
    at C:\Jenkins\workspace\%PROJECTNAME%\node_modules\cordova\node_modules\cordova-lib\src\cordova\prepare.js:105:36

Cordova-Android 8.0.0

Ionic:
  ionic (cli):        4.10.1
  ionic framework:    ionic-angular 3.9.2
  @ionic/app-scripts: 3.1.11

Cordova:
  cordova (cli):      8.1.2 (cordova-lib@8.1.1)
  platforms:          "cordova-android": "8.0.0"

System:
  Android SDK Tools:  26.1.1
  NodeJS:             10.15.3
  npm:                6.4.1
  OS:                 Windows 10

I only get this error on my jenkins server. Not on my local pc. But I don't use network paths.

@Domvel

This comment has been minimized.

Copy link

commented Mar 18, 2019

Any news / workaround / status? This problem is not 100% reproducible, sometimes it works. But "random" is the wrong word. It's about 80-99.9% that the build fails. This means: It's a critical must-fix bug and should get the highest priority.

@oliversalzburg

This comment has been minimized.

Copy link
Author

commented Mar 18, 2019

@Domvel Other than completely removing fs-extra, there is no way to fix this in Cordova AFAIK. This is unlikely to happen, because there has just been a move to introduce fs-extra into Cordova.

This would need to be fixed upstream, but given the activity in jprichardson/node-fs-extra#657, it appears that this won't happen any time soon.

This is just a huge effin mess and I agree that the builds fail more often than they pass. It is a major defect, but I don't see how to resolve it and it appears to not affect enough people to get enough attention.

I urge you to join the discussion in the upstream ticket, as that seems to be the most promising place to find a solution for this issue.

@Domvel

This comment has been minimized.

Copy link

commented Mar 18, 2019

@oliversalzburg Thanks for the info. Is fs-extra new in Cordova-Android 8.0.0 or is fs-extra updated? Maybe a downgrade is a good idea? If there is no solution I have to go back to Cordova-Android 7.x.
https://github.com/apache/cordova-common/blob/master/RELEASENOTES.md
CB-14140 Replace shelljs calls with fs-extra & which

I don't quite understand the reason that fs-extra breaks the build. What does the message mean: "Source and destination must not be the same." I see over 2 occurrences in node-fs-extra sources in relation of copy a object. Is this a pseudo-error and could be removed? (copy.js and copy-sync.js in function "checkPaths")

@oliversalzburg

This comment has been minimized.

Copy link
Author

commented Mar 18, 2019

fs-extra was newly introduced and you can read up about all the details of this error in the upstream tickets. There is no need to repeat all the details here.

@f5cs

This comment has been minimized.

Copy link

commented May 7, 2019

Have an other solutions to replace the node-fs-extra?

@oliversalzburg

This comment has been minimized.

Copy link
Author

commented May 7, 2019

@f5cs We've been using a postinstall hook to monkey-patch the fs-extra dependency after installation on Windows.

@dimitriscsd

This comment has been minimized.

Copy link

commented May 8, 2019

fs-extra are about to release version 8 which apparently has a workaround for this issue.

Will you guys use that once it's out?

@oliversalzburg

This comment has been minimized.

Copy link
Author

commented May 8, 2019

@dimitriscsd With "we", I didn't mean Cordova, I meant our company :D But I'm sure Cordova will be updated to use the latest version of fs-extra. Feel free to open a PR when the update is available to help speed up the process.

@dimitriscsd

This comment has been minimized.

Copy link

commented May 8, 2019

@oliversalzburg understood. My "you guys" was more towards cordova devs than you specifically :)

Thanks for your hook workaround suggestion though. I might have to give that a shot if this takes too long.

Do you mind sharing more info on it?

@oliversalzburg

This comment has been minimized.

Copy link
Author

commented May 8, 2019

This is the hook file itself:

#!/usr/bin/env node

"use strict";

const fs   = require( "fs" );
const path = require( "path" );
const os   = require( "os" );

if( os.platform() !== "win32" ) {
	console.log( "fs-extra doesn't need to be patched on non-Windows OS. Skipping." );
	process.exit( 0 );
}

function copyFile( source, target, cb ) {
	let cbCalled = false;

	const reader = fs.createReadStream( source );
	const writer = fs.createWriteStream( target );

	reader.on( "error", error => done( error ) );
	writer.on( "error", error => done( error ) );
	writer.on( "close", () => done() );

	reader.pipe( writer );

	function done( error ) {
		if( !cbCalled ) {
			cb( error );
			cbCalled = true;
		}
	}
}

const copyFrom     = path.resolve( __dirname, "fs-extra-patch/copy.js" );
const copyTo       = path.resolve( __dirname, "../node_modules/fs-extra/lib/", "copy/copy.js" );
const copySyncFrom = path.resolve( __dirname, "fs-extra-patch/copy-sync.js" );
const copySyncTo   = path.resolve( __dirname, "../node_modules/fs-extra/lib/", "copy-sync/copy-sync.js" );

console.log( `Copying '${copyFrom}' to '${copyTo}'.` );
copyFile( copyFrom, copyTo, Function.prototype );
console.log( `Copying '${copySyncFrom}' to '${copySyncTo}'.` );
copyFile( copySyncFrom, copySyncTo, Function.prototype );

Relative to it, there is a folder named fs-extra-patch, which contains the two patched files. You're going to want to change the checkPaths method to remove the broken check. For copy-sync, it should look like:

function checkPaths( src, dest ) {
	try {
		return fs.statSync( dest );
	} catch( err ) {
		if( err.code === "ENOENT" ) {
			return notExist;
		}
		throw err;
	}
}

Hope it helps.

@dpogue

This comment has been minimized.

Copy link
Member

commented May 8, 2019

fs-extra are about to release version 8 which apparently has a workaround for this issue.

Will you guys use that once it's out?

If/when there is a version released with a fix, we'll see if we can pull it in without it being a breaking change (which pretty much just means, it needs to support node 6).

@brodybits

This comment has been minimized.

Copy link

commented May 8, 2019

fs-extra are about to release version 8 which apparently has a workaround for this issue.

jprichardson/node-fs-extra#667

[...]
it needs to support node 6

jprichardson/node-fs-extra#667 seems to keep Node.js 6 support in package.json: [1]

and tested green so far on AppVeyor: [2]

I would personally favor incorporating this update in a minor release due to what I think are some non-trivial changes in jprichardson/node-fs-extra#667

[1] https://github.com/jprichardson/node-fs-extra/blob/bb30e7f1758c0388a8e4cddb48752e83bcb85e8b/package.json#L5-L7

[2] https://github.com/jprichardson/node-fs-extra/blob/bb30e7f1758c0388a8e4cddb48752e83bcb85e8b/appveyor.yml#L5

@Domvel

This comment has been minimized.

Copy link

commented May 10, 2019

@dpogue Why is node 6 required? Why not just update and use the latest. e.g. 10.x (LTS). I just wondering.
But if I understand it correctly, there is a fallback. It uses node 10 features like biginteger, but falls back to number if not node 10.x. (correct me if I'm wrong.)

@brodybits

This comment has been minimized.

Copy link

commented May 10, 2019

Why is node 6 required?

According to semver rules, we cannot drop Node.js 6 support without making another major release.

@janpio

This comment has been minimized.

Copy link
Member

commented May 10, 2019

For context:

Why not just update and use the latest. e.g. 10.x (LTS)

Currently ~45% of Cordova users are using node <10.

@dimitriscsd

This comment has been minimized.

Copy link

commented May 11, 2019

fs-extra v8 has been released. Just a heads up.

@Domvel

This comment has been minimized.

Copy link

commented May 13, 2019

@janpio

Currently ~45% of Cordova users are using node <10.

But this is like the Internet Explorer phenomenon. It's more a question of laziness and ignorance. I see it more as a task to bring the people in it. Using the latest versions. Especially for developers, it is important to always be up to date and not to stick to proven versions. For a long time that is not good.

However, I think it's because these people have not yet updated. But there would be nothing wrong with that. Or are there known issues with Node 10+?

@oliversalzburg

This comment has been minimized.

Copy link
Author

commented May 13, 2019

@Domvel Not updating NodeJS isn't always linked to being lazy. Think about companies where you have hundreds of developers. Maybe dependencies you use no longer work in newer versions of Node. There are very good reasons not to update NodeJS constantly.

@Domvel

This comment has been minimized.

Copy link

commented May 13, 2019

@oliversalzburg This was not exactly what I meant. It's fine. Do not blindly update the productive environments. The question is, when do these people finally update? I mean only the people and companies that still use Internet Explorer. It's just wrong in every way. No discussion please. 😄
In my case, I persuaded people to stop supporting IE. This should be the mission of every web developer. ... But I digress ... I just refer this. ...

@brodybits brodybits transferred this issue from apache/cordova-common Jun 7, 2019

@dpogue dpogue referenced this issue Jun 7, 2019
3 of 3 tasks complete
@brodybits

This comment has been minimized.

Copy link

commented Jun 7, 2019

I just transferred this issue from cordova-common to cordova and closed #83 as a duplicate. Also discussed in the following places:

I hope to get the bugfix on cordova-common released next week, then I think we should update some other packages including cordova-lib, cordova-android, cordova-windows, and cordova-cli.

Please feel free to contact us via email to dev@cordova.apache.org in case of any further delays.

@janpio janpio added the bug label Jun 7, 2019

@ath0mas

This comment has been minimized.

Copy link

commented Jun 12, 2019

cordova-common@3.2.0 is now released with apache/cordova-common#70 merged (Thx! @brodybits)
Will it be available in Npm soon? With updates of the other packages you mentioned too?

@brodybits

This comment has been minimized.

Copy link

commented Jun 12, 2019

(Thx! @brodybits)

You are welcome.

Will it be available in Npm soon?

I am still waiting for one more vote from a committer, will publish on npm on Friday if we get the vote in time.

With updates of the other packages you mentioned too?

I only mentioned some other issues and a PR where a similar problem was reported.

I think the update in cordova-common should resolve the issue for cordova-android and cordova-windows platforms since they do not use any other Cordova dependencies.

I would also like to apply the same update to some other Cordova packages, cannot promise when I will have time to do this. Here are the packages I think should be updated when someone gets a chance:

  • cordova-electron
  • cordova-create
  • cordova-fetch
  • cordova-lib
  • cordova-windows - update should be done in devDependencies, which should only affect end-to-end test script and not any end users
@erisu erisu referenced this issue Jun 13, 2019
1 of 1 task complete
@brodybits

This comment has been minimized.

Copy link

commented Jun 14, 2019

I just published cordova-common@3.2.0 on npm, blog post should show up pretty soon. I expect that this should resolve the reported issue for most if not all users. Here is the recommended procedure to update both the Cordova tooling and Cordova project:

  • remove existing installation of Cordova by the following command: npm uninstall -g cordova, or use a manager such as nvm or the cross-platform jasongin/nvs to start with a fresh new installation of Node.js
  • install Cordova tooling again by the following command: npm install -g cordova
  • in the Cordova app project:
    • do the following command for each platform: cordova platform rm <platform name>
    • completely remove the following artifacts: package-lock.json, node_modules, and platforms directory
    • it is optional but recommended to completely remove the plugins directory as well
    • double-check that there is no remaining configuration of cordova-common or any of the Cordova platforms in config.xml or package.json
    • try adding and building for each desired platform, as documented

This may be extra pedantic, but I would recommend this procedure to hopefully ensure a zero chance of using an old version of cordova-common.

I would like to update the other packages (cordova-create, cordova-fetch, cordova-lib, cordova-electron, and cordova-windows) to use fs-extra@8. Unfortunately I cannot promise when I will get a chance to work on these.

I took about 2-3 days out of an extremely busy schedule to make this update, in response to the demand from the user community.

@l3ender

This comment has been minimized.

Copy link

commented Jun 14, 2019

Thanks much, @brodybits!

I would like to update the other packages (cordova-create, cordova-fetch, cordova-lib, cordova-electron, and cordova-windows) to use fs-extra@8. Unfortunately I cannot promise when I will get a chance to work on these.

Is there any way I could help out in updating other packages? Would you like me to submit PRs targeting the updated packages?

I see that the above packages have dependencies to both cordova-common and fs-extra. How come fs-extra is explicitly mentioned instead of being pulled in as a transitive dependency of cordova-common?

Again, thanks for your efforts!

@sayanjyotidas

This comment has been minimized.

Copy link

commented Aug 16, 2019

I have fixed the problem (for my Cordova build not working) by a much easier way guys. Please follow the below steps to update your cordova-common library without having to re-install Cordova or cordova-android:

  • Clone the latest (bug-fixed) cordova-common repository to your machine by runing git clone https://github.com/apache/cordova-common.git

  • cd to the cloned directory and run npm install to download the required dependencies.

  • After you have completely installed the library along with dependencies, cut (or copy) the entire cordova-common-master folder (the directory which you just downloaded) and paste in into <your cordova project>/node_modules folder, be sure to replace the previous outdated cordova-common directory.

  • Restart your CLI and the cordova build should work fine :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.