copyDependencyDir's cp '-RfL' needs to change 'f' to 'u' to support sloppy second copies #3360
Conversation
|
Can one of the admins verify this patch? |
|
run ci |
1 similar comment
|
run ci |
|
Hi @paulpv As far as the code is concerned I have run into some issues while testing your proposed changes.
I have managed to reproduce the issue on all three platforms without your proposed changes, however with them I still get I would like to ask you for details about your environment in order to shed some further light on the matter. |
|
Hey @paulpv , |
Let me start off by saying that today is Thursday and I just started using NativeScript on Monday, so I am most definitely a {N}ewb to {N} and this issue and my below comments might be making a lot of wrong assumptions.
Keep that in mind, and please kindly educate me where I am wrong.
(FWIW, I am a >25 year programmer competent in JavaScript, Java, C#, C/C++, Python... so I'm not a total newb)
I feel compelled, especially as a {N}ewb, to make this one soap-box statement: I have zero idea how this issue hasn't affected a non-trivial percentage of nativescript plugin developers out there. If you develop a nativescript plugin and keep it under [git] source control [who doesn't?] and add the repo'd plugin as a local dependency to an app, I don't see any possible way that the build could work. This issue [below] has been around for over 4 years, so I've got ta ask: How come this has not been fixed?
The issue is "simple", but I am having a very hard time articulating it simply or describing the "right" solution (which isn't obvious).
Here is the executive summary
What is happening is that during the build process the dependencies are actually being copied twice to
platforms/android/app/src/main/assets/app/tns_modules.The first copy of a dependency under source control is successful.
Any copied plugin that is under source control also has it's .git folder copied to
platforms/android/app/src/main/assets/app/tns_modules/some-plugin/.git..git folders are well known to contain working/temp read-only files (ex:
.git/objects/pack/pack-a98ffe92fb5a19a105f39a02942047edfc956aad.idx).Those read-only .git files are now copied to
tns_modules.The second copy fails for the now obvious quite simple reason that
cpcannot overwrite a read-only file.The shelljs command attempts to use the
-RfLflags, but the >4 year old shelljs/shelljs#98 issue says that shelljs cannot handle this.So, the second copy fails, and the shelljs' error handling swallows the real error and generically report's the error as
common.error("cannot create directory '" + dest + "': No such file or directory");.https://github.com/shelljs/shelljs/blob/master/src/cp.js#L261
More Detail
This is directly related to #3028, and specifically the excellent detail in comment #3028 (comment).
That issue was reported 2017/08/02 and has had no resolution.
Issue #3028 refers to shelljs/shelljs#98.
That shelljs issue was reported 2013/12/16...over 4 years ago and has had no resolution!
Issue Symptom:
tns build android(orios) fails with misleading error message:NOTE: Sometimes I have also seen the following error message instead:
I'm ignoring this second error for now [my theory is that fixing the first should prevent the second from ever happening].
The actual first error is...
https://github.com/shelljs/shelljs/blob/master/src/cp.js#L66
...caused by
lib/tools/node-modules-dest-copy.js::copyDependencyDir(...)'s call toshelljs.cp("-RfL", dependency.directory, destinationPath);https://github.com/NativeScript/nativescript-cli/blob/master/lib/tools/node-modules/node-modules-dest-copy.ts#L45
Repro
Repro # 1:
Expected: Success
Result (includes some of my added logging; the
outdated templateis not relevant):Repro # 2 (not 100% related, but semi-related to another issue w/ shelljs):
Solution
The non-obvious "right" solution is that the nativescript build process should probably not copy every file/folder to
tns_modules.It should probably ignore hidden/system files such as .git, .gitignore, etc.
The problem with this is, how to do it to be SCM agnostic?
My first instinct it to plagerize how
npm installappears to ignore .git folder and copy only certain files/folders.This change is beyond my current 4-day old experience w/ {N} to handle.
Another non-obvious solution may be to avoid the sloppy second copies.
I don't think this is relevant, because in the case of a legitimate livesync, multiple copy commands are inevitable.
This PR proposes a short term unblocker to replace the cp's
force switch with auupdate switch.This way no unchanged files will be copied.
The problem will reproduce if the read-only file is modified and attempted to be copied a second time.
I don't foresee read-only files changing very often, so this weakness should hopefully not come up very often.
Thus the above "right" solution is the way things really need to be done.