Skip to content
This repository has been archived by the owner on Sep 15, 2023. It is now read-only.

Blendid Run Error #532

Closed
kferran opened this issue Jan 19, 2018 · 11 comments · Fixed by #533
Closed

Blendid Run Error #532

kferran opened this issue Jan 19, 2018 · 11 comments · Fixed by #533
Labels

Comments

@kferran
Copy link

kferran commented Jan 19, 2018

Running into this issue when trying to run yarn run blendid after upgrading to version 4.4.2.
http://prntscr.com/i2drtz

@StueyKent
Copy link

Looks like it is something to do with the path to the package in blendid/gulpfile.js/lib/task-defaults.js

Swapping out var pkg process.env.PWD with a relative path means that blendid can find the package but other references break later down the chain.

@maoueh
Copy link
Contributor

maoueh commented Jan 19, 2018

The problem is indeed coming from process.env.PWD being different than before. And as I thought, the culprit is indeed the bin/blendid fix I did recently (#471). I'm not sure why though :)

So, the problem is multi-fold. I did two kind of tests with one variance for each kind. First, I tested with the bin fix I did and without. For each kind, I also tested running blendid from project directory and from a child directory of the project. I did my tests on Windows, Windows + MSYS2 (a Cygwin like environment where POSIX translation exists) and Mac OS X. You'll find the findings at the end of my post.

On Mac OS X, the PWD remain mostly constant with or without the bin fix. I say mostly because when running from the test subdirectory, the PWD becoming the real canonical file /private/tmp/blendid-test2 which is the resolving of the /tmp dir. This however remain constant with and without bin fix.

On Windows, it seems the PWD does change with the bin fix I did having the extra node_modules added. Why, it's really unclear. The main difference is that before bin fix, the --gulpfile argument was node_modules/blendid/gulpfile.js while with the bin fix, the index.js is also present node_modules/blendid/gulpfile.js/index.js. Strangely, I get there is an extra path layer but I don't get why it plays with PWD. My guess is that gulp is fiddling with it and the extra index.js breaks the inference ... but only on Windows.

On Windows + MSYS2, the terminal is POSIX aware and convert from <-> to Windows path if the executed binary if Windows only. The problem that arise is that PWD is not converted correctly and does not become a canonical path remaining at /tmp/test-blendid-run for example. More problematic, moving into the test subdirectory make the PWD change to /tmp/test-blendid-run/test which is not the case on the other cases as it remains constant be it root or subdirectory. The path /tmp/test-blendid-run is then forwarded to a pure Windows executable (not MSYS2 aware) (i.e. node.exe in our case) which does not handle POSIX path and as such, resolves it to C:\tmp\test-blendid-run. Of course, this path is wrong.

The ultimate fix is coming from a special environment variable that is set by Gulp. Indeed, they have the INIT_CWD variable (see commit) which seems to always point at the right location no matter what the system and the subdirectory:

  • Mac + bin fix + root: /private/tmp/blendid-test2
  • Mac + bin fix + subdir: /private/tmp/blendid-test2
  • Mac + no bin fix + root: /private/tmp/blendid-test2
  • Mac + no bin fix + subdir: /private/tmp/blendid-test2
  • Windows + bin fix + root: C:\Gnu\tmp\test-blendid-run
  • Windows + bin fix + subdir: C:\Gnu\tmp\test-blendid-run
  • Windows + no bin fix + root: C:\Gnu\tmp\test-blendid-run
  • Windows + no bin fix + subdir: C:\Gnu\tmp\test-blendid-run
  • MSYS2 + bin fix + root: C:\Gnu\tmp\test-blendid-run
  • MSYS2 + bin fix + subdir: C:\Gnu\tmp\test-blendid-run
  • MSYS2 + not bin fix + root: C:\Gnu\tmp\test-blendid-run
  • MSYS2 + not bin fix + subdir: C:\Gnu\tmp\test-blendid-run

I will open a PR with the updated code in task-defaults.js in the upcoming hours.

Results

Mac OS X (without bin fix)

Terminal at /tmp/blendid-test2
  • PWD /tmp/blendid-test2
  • INIT_CWD /private/tmp/blendid-test2
  • process.cwd() /private/tmp/blendid-test2/node_modules/blendid
Terminal at /tmp/blendid-test2/test
  • PWD /private/tmp/blendid-test2
  • INIT_CWD /private/tmp/blendid-test2
  • process.cwd() /private/tmp/blendid-test2/node_modules/blendid

Mac OS X (with bin fix)

Terminal at /tmp/blendid-test2
  • PWD /tmp/blendid-test2
  • INIT_CWD /private/tmp/blendid-test2
  • process.cwd() /private/tmp/blendid-test2/node_modules/blendid/gulpfile.js
Terminal at /tmp/blendid-test2/test
  • PWD /private/tmp/blendid-test2
  • INIT_CWD /private/tmp/blendid-test2
  • process.cwd() /private/tmp/blendid-test2/node_modules/blendid/gulpfile.js

Windows (without bin fix)

Terminal at C:\Gnu\tmp\test-blendid-run
  • PWD C:\Gnu\tmp\test-blendid-run
  • INIT_CWD C:\Gnu\tmp\test-blendid-run
  • process.cwd() C:\Gnu\tmp\test-blendid-run\node_modules\blendid
Terminal at C:\Gnu\tmp\test-blendid-run\test
  • PWD C:\Gnu\tmp\test-blendid-run
  • INIT_CWD C:\Gnu\tmp\test-blendid-run
  • process.cwd() C:\Gnu\tmp\test-blendid-run\node_modules\blendid

Windows (with bin fix)

Terminal at C:\Gnu\tmp\test-blendid-run
  • PWD C:\Gnu\tmp\test-blendid-run\node_modules
  • INIT_CWD C:\Gnu\tmp\test-blendid-run
  • process.cwd() C:\Gnu\tmp\test-blendid-run\node_modules\blendid\gulpfile.js
Terminal at C:\Gnu\tmp\test-blendid-run\test
  • PWD C:\Gnu\tmp\test-blendid-run\node_modules
  • INIT_CWD C:\Gnu\tmp\test-blendid-run
  • process.cwd() C:\Gnu\tmp\test-blendid-run\node_modules\blendid\gulpfile.js

Windows + MSYS2 (without bin fix)

Terminal at /tmp/test-blendid-run
  • PWD /tmp/test-blendid-run
  • INIT_CWD C:\Gnu\tmp\test-blendid-run
  • process.cwd() C:\Gnu\tmp\test-blendid-run\node_modules\blendid
Terminal at /tmp/test-blendid-run/test
  • PWD /tmp/test-blendid-run/test
  • INIT_CWD C:\Gnu\tmp\test-blendid-run
  • process.cwd() C:\Gnu\tmp\test-blendid-run\node_modules\blendid

Windows + MSYS2 (with bin fix)

Terminal at /tmp/test-blendid-run
  • PWD /tmp/test-blendid-run
  • INIT_CWD C:\Gnu\tmp\test-blendid-run
  • process.cwd() C:\Gnu\tmp\test-blendid-run\node_modules\blendid\gulpfile.js
Terminal at /tmp/test-blendid-run/test
  • PWD /tmp/test-blendid-run/test
  • INIT_CWD C:\Gnu\tmp\test-blendid-run
  • process.cwd() C:\Gnu\tmp\test-blendid-run\node_modules\blendid\gulpfile.js

@maoueh
Copy link
Contributor

maoueh commented Jan 19, 2018

I did test the fix on the three systems mentioned above. I'm pretty confident it should continue to work after the changes.

@kferran Can you describe a bit your system. You seem to be using special terminal as I see in your screenshot that there is a ~. Also, you could try the upcoming fix I'm planning to send by modifying inline the node_modules/blendid/gulpfile.js/lib/task-defaults.js file. See the git diff below:

diff --git a/gulpfile.js/lib/task-defaults.js b/gulpfile.js/lib/task-defaults.js
index 2bc608b..8c930b1 100644
--- a/gulpfile.js/lib/task-defaults.js
+++ b/gulpfile.js/lib/task-defaults.js
@@ -1,6 +1,6 @@
 const os   = require('os')
 const path = require('path')
-const pkg  = require(path.resolve(process.env.PWD, 'package.json'))
+const pkg  = require(path.resolve(process.env.INIT_CWD, 'package.json'))

 module.exports = {
   javascripts: {

@maoueh
Copy link
Contributor

maoueh commented Jan 19, 2018

@benjtinsley As for the init problem you solved, it seems the #471 was also the culprit. In the tests results above, if you look at the process.cwd(), you'll see that it also has changed after the bin fix.

  • Prior bin fix: /private/tmp/blendid-test2/node_modules/blendid
  • With bin fix: /private/tmp/blendid-test2/node_modules/blendid/gulpfile.js

Since the cwd() is now on folder above, files were not resolving correctly anymore and that's why you needed to add ../ everywhere.

I never thought changing adding index.js part tot the --gulpfile argument would play on on the cwd. Seems it could even be a Gulp bug to my eyes because folder blendid/gulpfile.js should resolve to blendid/gulpfile.js/index.js so I thought it was not a problem. Seems I was wrong :)

For the bug fix you did, we can either left it as-is or I can patch bin/blendid in this way and revert your commit:

diff --git a/bin/blendid.js b/bin/blendid.js
index bd5641a..eff4853 100755
--- a/bin/blendid.js
+++ b/bin/blendid.js
@@ -2,7 +2,7 @@
 const path = require('path');

 const additionalArgs = require('minimist')(process.argv.slice(2))._
-const blendidEntryFile = require.resolve('blendid');
+const blendidEntryFile = path.dirname(require.resolve('blendid'));
 const gulpModulePath = path.dirname(require.resolve('gulp'));
 const gulpBinaryFile = path.join(gulpModulePath, '/bin/gulp');

Tell me if you think we should do that (path bin/blendid and revert your commit) or left stuff as-is. Another possibility is to use __dirname instead to resolve the files relatively to the invoking script instead of relying on cwd. Ultimately, it's probably even a better fix as I highly suggest my developers to avoid relying on cwd whenever possible.

I really did not oversee all those problems. Sorry for the trouble.

@maoueh
Copy link
Contributor

maoueh commented Jan 19, 2018

@kferran Note that I just noticed there is 66 other places where process.env.PWD is used. So the small change I did should help pass the initial error. However, it might continue to be problematic at some other places. Trying my upcoming PR if possible would probably be a better shot and would be appreciated.

@benjtinsley
Copy link
Contributor

@maoueh thanks for you exhaustive research on this issue. i was also noticing some strange behavior when trying to yarn link my blendid directory with a working one and thought that it may have been related to bin/blendid.js but couldn't resolve the problem. ideally we could revert or fix all changes related to this because, as you mentioned before i wasn't entirely clear how such meaningless changes could have such huge ramifications on the entire project 🧐

if you haven't already i can change back the relative path commits in the init files and merge those in with your bin changes and pray that this does the trick!

@maoueh
Copy link
Contributor

maoueh commented Jan 19, 2018

For the process.cwd() thing, I will update the bin/blendid script and revert your commit as part of my upcoming PR.

I will introduce a projectPath function which will do the resolving of path relative to the project directory (the consumer's project). I started with resolvePathRelativeToProject but found it too long so I now using projectPath. Tell me if you would prefer a better name.

The projectPath function will use process.env.INIT_CWD environment. I did extensive tests and this is working really well, even when blendid is hoisted at top level node_modules instead of the node_modules of the module where blendid is used.

I will try to complete the PR today but that may need to complete it this weekend, there is lot of places where PWD is used currently.

@benjtinsley
Copy link
Contributor

@maoueh i think projectPath makes perfect sense.

@kferran
Copy link
Author

kferran commented Jan 19, 2018

@maoueh Sorry just getting back to my desk. I am running cmder as my terminal in Windows. I'll bring down your pull request and try it out.

maoueh added a commit to maoueh/blendid that referenced this issue Jan 20, 2018
…s#532)

It appears that `process.env.PWD` is not consistent across platform as well
as across sub-directory invocations. Indeed, even if it is consistent on
Mac OS X (and probably Unix at the same time but untested), there is
inconsitencies problem on Windows when using a POSIX transformer platform
like Cygwin or MSYS2.

Indeed, in those cases, `PWD` is not the canonical Windows path but the
original POSIX path of the terminal. Furthermore, invoking from a
sub-directory also fiddles with the `PWD` adding subdirectory into it.

To fix that, it seems that Gulp is providing an environment variable that
points to the root directory of the project and which is constant across
platforms. See gulpjs/gulp@2bf23ba
for the actual INIT_CWD implementation.

So, to fix the problem everywhere, added a function `resolveRelativeToProject`
that does the correct resolving using `INIT_CWD` environment variable
instead of the previously used `PWD`.

To make stuff easier to use next time, a `projectPath` function was added
that does the resolving of paths relative to the project root directory.
All previous usages of `path.(resolve|join)(process.env.PWD, ...)` that were
used throughout the website are now using `projectPath(...)` instead.

Fixed bin script `blendid/gulpfile.js/index.js` resolution to use `__dirname`
instead of `require.resolve`. The `require.resolve` does not work when using
`yarn link` to development `blendid` features. Indeed, `require.resolve` does
the resolution from the linked project leading to `blendid` not being
resolvable (because not present in the linked `node_modules` directory). By
using `__dirname`, we achieve the same goal as before but being more robust
to linked project for development.
@maoueh
Copy link
Contributor

maoueh commented Jan 22, 2018

@kferran PR #533 ready to test out.

@kferran
Copy link
Author

kferran commented Jan 24, 2018

Finally had a chance to bring down PR #533. Everything seems to be running fine with these fixes.

Thanks for hammering this out guys!

benjtinsley pushed a commit that referenced this issue Jan 24, 2018
It appears that `process.env.PWD` is not consistent across platform as well
as across sub-directory invocations. Indeed, even if it is consistent on
Mac OS X (and probably Unix at the same time but untested), there is
inconsitencies problem on Windows when using a POSIX transformer platform
like Cygwin or MSYS2.

Indeed, in those cases, `PWD` is not the canonical Windows path but the
original POSIX path of the terminal. Furthermore, invoking from a
sub-directory also fiddles with the `PWD` adding subdirectory into it.

To fix that, it seems that Gulp is providing an environment variable that
points to the root directory of the project and which is constant across
platforms. See gulpjs/gulp@2bf23ba
for the actual INIT_CWD implementation.

So, to fix the problem everywhere, added a function `resolveRelativeToProject`
that does the correct resolving using `INIT_CWD` environment variable
instead of the previously used `PWD`.

To make stuff easier to use next time, a `projectPath` function was added
that does the resolving of paths relative to the project root directory.
All previous usages of `path.(resolve|join)(process.env.PWD, ...)` that were
used throughout the website are now using `projectPath(...)` instead.

Fixed bin script `blendid/gulpfile.js/index.js` resolution to use `__dirname`
instead of `require.resolve`. The `require.resolve` does not work when using
`yarn link` to development `blendid` features. Indeed, `require.resolve` does
the resolution from the linked project leading to `blendid` not being
resolvable (because not present in the linked `node_modules` directory). By
using `__dirname`, we achieve the same goal as before but being more robust
to linked project for development.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants