search

vigetlabs / blendid

A delicious blend of gulp tasks combined into a configurable asset pipeline and static site builder
MIT License
4.97k stars 683 forks source link

Blendid Run Error #532

Closed kferran closed 6 years ago

kferran commented 6 years ago

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

StueyKent commented 6 years ago

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 commented 6 years ago

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:

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
Terminal at /tmp/blendid-test2/test

Mac OS X (with bin fix)

Terminal at /tmp/blendid-test2
Terminal at /tmp/blendid-test2/test

Windows (without bin fix)

Terminal at C:\Gnu\tmp\test-blendid-run
Terminal at C:\Gnu\tmp\test-blendid-run\test

Windows (with bin fix)

Terminal at C:\Gnu\tmp\test-blendid-run
Terminal at C:\Gnu\tmp\test-blendid-run\test

Windows + MSYS2 (without bin fix)

Terminal at /tmp/test-blendid-run
Terminal at /tmp/test-blendid-run/test

Windows + MSYS2 (with bin fix)

Terminal at /tmp/test-blendid-run
Terminal at /tmp/test-blendid-run/test
maoueh commented 6 years ago

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 commented 6 years ago

@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.

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@maoueh i think projectPath makes perfect sense.

kferran commented 6 years ago

@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 commented 6 years ago

@kferran PR #533 ready to test out.

kferran commented 6 years ago

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

Thanks for hammering this out guys!