reverted Nested executables #1210 to fix #1823

[bestander]

Fixes #1823.
Looks like nested dependencies binaries can conflict with each other and I reverted #1210 for now.

In a fix we need to expect tests that prevent duplicate bins overwriting

[bestander]

Self merging

[onemen]

With the patch in #1210 all dependencies bins are linked to node_modules/.bin
For example, when installing node-sass yarn add --dev node-sass, the bin for mkdirp version 0.5.1 is linked three times, each time by a different dependency:
    fstream@^1.0.0, fstream@^1.0.2 depend on mkdirp ">=0.5 0"
    node-gyp@^3.3.1 depend on mkdirp ">=0.5 0"
    node-sass@^3.13.0 depend on mkdirp "^0.5.1"
Yarn package-linker call cmdShim with the same 'src' and 'dest' arguments three times.
cmdShim is asynchronous and the calls from Yarn are parallel.
Sometimes a call to cmdShim with the same arguments can happens before the last call to the function completely resolves.

The cause of the error "ENOENT: no such file or directory, chmod 'PATH_TO_BIN'" in windows is the 'cmd-shim' module.
cmd-shim creates 2 file in node_modules/.bin for each bin, sh file and cmd file.
The process of creating these files divided in cmd-shim into four asynchronous steps:
    1. fs.writeFile crate the *.cmd file from the bin
    2. In parallel to 1, fs.writeFile crate the sh file from the bin
After 1 and 2 resolved without an error
    3. fs.chmod set the *.cmd file mode to 0755
    4. In parallel to 3, fs.chmod set the sh file mode to 0755

The error is throwing by step 3 or 4 when 2nd call to 'cmd-shim' with the same arguments is in step 1 or 2 while the 1st call is in step 2 or 4.

There are more than one why to fix this bug, here you can find two

Patch 1 – Queued the call to cmdShim

diff --git a/src/package-linker.js b/src/package-linker.js
--- a/src/package-linker.js
+++ b/src/package-linker.js
@@ -48,15 +48,16 @@ export default class PackageLinker {
   async linkSelfDependencies(pkg: Manifest, pkgLoc: string, targetBinLoc: string): Promise<void> {
     targetBinLoc = await fs.realpath(targetBinLoc);
     pkgLoc = await fs.realpath(pkgLoc);
-    for (const [scriptName, scriptCmd] of entries(pkg.bin)) {
+    const promises = entries(pkg.bin).map(async ([scriptName, scriptCmd]): Promise<void> => {
       const dest = path.join(targetBinLoc, scriptName);
       const src = path.join(pkgLoc, scriptCmd);
       if (!await fs.exists(src)) {
         // TODO maybe throw an error
-        continue;
+        return null;
       }
-      await linkBin(src, dest);
-    }
+      return linkBin(src, dest);
+    });
+    await Promise.all(promises);
   }

   async linkBinDependencies(pkg: Manifest, dir: string): Promise<void> {
@@ -103,9 +104,9 @@ export default class PackageLinker {
     await fs.mkdirp(binLoc);

     // write the executables
-    for (const {dep, loc} of deps) {
-      await this.linkSelfDependencies(dep, loc, binLoc);
-    }
+    await promise.queue(deps, async ({dep, loc}): Promise<void> => {
+      this.linkSelfDependencies(dep, loc, binLoc);
+    });
   }

   getFlatHoistedTree(patterns: Array<string>): Promise<HoistManifestTuples> {
@@ -190,7 +191,7 @@ export default class PackageLinker {
         const binLoc = path.join(this.config.cwd, this.config.getFolder(pkg));
         await this.linkBinDependencies(pkg, binLoc);
         tickBin(dest);
-      }, 4);
+      });
     }
   }

Patch 2 – Call cmdShim with same 'dest' after the previous call resolved

diff --git a/src/package-linker.js b/src/package-linker.js
--- a/src/package-linker.js
+++ b/src/package-linker.js
@@ -22,9 +22,13 @@ type DependencyPairs = Array<{
   loc: string
 }>;

+const allBins: Map<string, Promise<void>> = new Map();
+
 export async function linkBin(src: string, dest: string): Promise<void> {
   if (process.platform === 'win32') {
-    await cmdShim(src, dest);
+    const next = () => cmdShim(src, dest);
+    const promise = allBins.has(dest) ? allBins.get(dest).then(next) : next();
+    allBins.set(dest, promise);
   } else {
     await fs.mkdirp(path.dirname(dest));
     await fs.symlink(src, dest);

[bestander]

Go ahead, send a new PR.
Tests for the thing that broke are vital

On Wed, 16 Nov 2016 at 11:14, onemen notifications@github.com wrote:

With the patch in #1210 #1210 all
dependencies bins are linked to node_modules/.bin
For example, when installing node-sass yarn add --dev node-sass, the bin
for mkdirp version 0.5.1 is linked three times, each time by a different
dependency:
fstream@^1.0.0, fstream@^1.0.2 depend on mkdirp ">=0.5 0"
node-gyp@^3.3.1 depend on mkdirp ">=0.5 0"
node-sass@^3.13.0 depend on mkdirp "^0.5.1"
Yarn package-linker call cmdShim with the same 'src' and 'dest' arguments
three times.
cmdShim is asynchronous and the calls from Yarn are parallel.
Sometimes a call to cmdShim with the same arguments can happens before the
last call to the function completely resolves.

The cause of the error "ENOENT: no such file or directory, chmod
'PATH_TO_BIN'" in windows is the 'cmd-shim' module.
cmd-shim creates 2 file in node_modules/.bin for each bin, sh file and cmd
file.
The process of creating these files divided in cmd-shim into four
asynchronous steps:
1. fs.writeFile crate the *.cmd file from the bin
2. In parallel to 1, fs.writeFile crate the sh file from the bin
After 1 and 2 resolved without an error
3. fs.chmod set the *.cmd file mode to 0755
4. In parallel to 3, fs.chmod set the sh file mode to 0755

The error is throwing by step 3 or 4 when 2nd call to 'cmd-shim' with the
same arguments is in step 1 or 2 while the 1st call is in step 2 or 4.

There are more than one why to fix this bug, here you can find two

Patch 1 – Queued the call to cmdShim

diff --git a/src/package-linker.js b/src/package-linker.js
--- a/src/package-linker.js
+++ b/src/package-linker.js
@@ -48,15 +48,16 @@ export default class PackageLinker {
async linkSelfDependencies(pkg: Manifest, pkgLoc: string, targetBinLoc: string): Promise {
targetBinLoc = await fs.realpath(targetBinLoc);
pkgLoc = await fs.realpath(pkgLoc);

• for (const [scriptName, scriptCmd] of entries(pkg.bin)) {
• const promises = entries(pkg.bin).map(async ([scriptName, scriptCmd]): Promise => {
  const dest = path.join(targetBinLoc, scriptName);
  const src = path.join(pkgLoc, scriptCmd);
  if (!await fs.exists(src)) {
  // TODO maybe throw an error

•    continue;
  

•    return null;
  

  }

•  await linkBin(src, dest);
  

• }

•  return linkBin(src, dest);
  

• });
• await Promise.all(promises);
  }

async linkBinDependencies(pkg: Manifest, dir: string): Promise {
@@ -103,9 +104,9 @@ export default class PackageLinker {
await fs.mkdirp(binLoc);

// write the executables

• for (const {dep, loc} of deps) {

•  await this.linkSelfDependencies(dep, loc, binLoc);
  

• }
• await promise.queue(deps, async ({dep, loc}): Promise => {

•  this.linkSelfDependencies(dep, loc, binLoc);
  

• });
  }

getFlatHoistedTree(patterns: Array): Promise {
@@ -190,7 +191,7 @@ export default class PackageLinker {
const binLoc = path.join(this.config.cwd, this.config.getFolder(pkg));
await this.linkBinDependencies(pkg, binLoc);
tickBin(dest);

•  }, 4);
  

•  });
  

  }
  }

Patch 2 – Call cmdShim with same 'dest' after the previous call resolved

diff --git a/src/package-linker.js b/src/package-linker.js
--- a/src/package-linker.js
+++ b/src/package-linker.js
@@ -22,9 +22,13 @@ type DependencyPairs = Array<{
loc: string
}>;

+const allBins: Map<string, Promise> = new Map();
+
export async function linkBin(src: string, dest: string): Promise {
if (process.platform === 'win32') {

• await cmdShim(src, dest);
• const next = () => cmdShim(src, dest);
• const promise = allBins.has(dest) ? allBins.get(dest).then(next) : next();
• allBins.set(dest, promise);
  } else {
  await fs.mkdirp(path.dirname(dest));
  await fs.symlink(src, dest);

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1867 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACBdWEmjycQCcbbNOD-x9vRy52O64g0dks5q-uWFgaJpZM4Ky7Fx
.

[onemen]

Do you prefer patch 1 or 2?
What would be consider to be a good test for these - yarn add finished without an error?

[bestander]

I think the algorithm should be a bit smarter - first priority for direct
bin dependencies, then all the others in any order but don't allow
rewriting existing links.

Should you depend in fs to sync whether file exists?

I would rather build a list of bins and resolve duplicates in memory and
then write them to disk.

The test should verify that proper bins get symlinked in .bin folder
(clashes resolved correctly)

On Wed, 16 Nov 2016 at 11:26, onemen notifications@github.com wrote:

Do you prefer patch 1 or 2?
What would be consider to be a good test for these - yarn add finished
without an error?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1867 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACBdWMSESXv6lliv8pSvAHnUvHpsSGpZks5q-uhUgaJpZM4Ky7Fx
.

[mateodelnorte]

Are we still intending a fix, here, for behavior reverted from #1210?

[bestander]

I think it is up for grabs for the community memebers

…

On 31 March 2017 at 15:07, Matt Walters ***@***.***> wrote: Are we still intending a fix, here, for behavior reverted from #1210 <#1210>? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1867 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWEDLHIWsu-maLJ6PGMoZdN-zWbU_ks5rrQicgaJpZM4Ky7Fx> .