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

Closed
3 tasks done
oliversalzburg opened this issue Mar 5, 2019 · 35 comments
Closed
3 tasks done

Random build failures due to fs-extra defect #121

oliversalzburg opened this issue Mar 5, 2019 · 35 comments
Labels
bug Something isn't working

Comments

@oliversalzburg
Copy link

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
Copy link

Domvel 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
Copy link

Domvel 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
Copy link
Author

@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
Copy link

Domvel 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
Copy link
Author

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
Copy link

f5cs commented May 7, 2019

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

@oliversalzburg
Copy link
Author

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

@dimitriscsd
Copy link

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
Copy link
Author

@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
Copy link

dimitriscsd 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
Copy link
Author

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
Copy link
Member

dpogue 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
Copy link

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
Copy link

Domvel 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
Copy link

Why is node 6 required?

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

@janpio
Copy link
Member

janpio 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
Copy link

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

@Domvel
Copy link

Domvel 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
Copy link
Author

@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
Copy link

Domvel 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
@brodybits
Copy link

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 Something isn't working label Jun 7, 2019
@ath0mas
Copy link

ath0mas 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
Copy link

(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

@brodybits
Copy link

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
Copy link

l3ender 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!

@theseyan
Copy link

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 :-)

@Perh0rd
Copy link

Perh0rd commented Sep 9, 2019

I want to add that if problem persist for Windows 10 users, just follow these steps :

Set-ExecutionPolicy Unrestricted -Scope CurrentUser -Force
npm install -g npm-windows-upgrade
npm-windows-upgrade

This worked for me.

@vienom
Copy link

vienom commented Sep 16, 2019

Hi!

I'm facing the same issue and I'm building on a mac.
cordova-common: 3.2.0
fs-extra: 8.1.0

Android (8.0.0) and iOS (5.0.1) builds fail.

@brodybits
Copy link

I'm facing the same issue and I'm building on a mac.

Which version of Node.js are you using?

I would highly recommend using the latest patch on Node.js version 10.

@vienom
Copy link

vienom commented Sep 17, 2019

My node version is 10.15.3. I pulled my project from my git repository again to have a clean start. Still the same error. Previous Android and iOS versions work fine.

@jamesdixon
Copy link

This issue continuously kills me and does so at random...

@gquagliano
Copy link

This may be caused by symlinks, at least that was my case. The solution, other than upgrading as stated before, is to replace symbolic links with hard links.

@I-Connect
Copy link

@gquagliano thx for this trigger, this solved it (Source and destination must not be the same.) for me when adding a new platform.

I use a virtualbox for development and I have disks mapped as network share from my host where the projects are located.

Moving the cordova project to the C drive fixed it for me.

@Stredsto
Copy link

Stredsto commented Oct 5, 2021

is this issue still open ?

@breautek
Copy link
Contributor

Closing this issue as obsolete. If you still have this issue, I'd recommend trying to reinstalling cordova and/or clearing your package-lock & node_modules to force npm to fetch the latest satisfactory dependencies, which should include the fixes from fs-extra upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests