gulp-cli icon indicating copy to clipboard operation
gulp-cli copied to clipboard

Add initial cwd to directories for searching config files

Open sttk opened this issue 7 years ago • 13 comments

I've add initCwd entry to the last of opts.configFiles.

This PR is for the issue #146 and changes gulp's behaviors about using config file and selecting gulpfile.

  • There is an ancestor directory having gulpfile.*.
  • And current directory has a config file which has flags.gulpfile property.
  • And current directory has no gulpfile or a gulpfile of which name is not 'gulpfile.*'.

In the situation above, gulp after this modification uses the config file in current dir and uses gulpfile specified in the config file, but changes cwd to the ancestor dir. Before this modification, gulp ignores this config file, and finds up a gulpfile in the ancestor dir, changes cwd to the dir and uses the gulpfile.

sttk avatar Jan 27 '18 16:01 sttk

@sttk I assume this is going to be changed with the new liftoff stuff?

phated avatar Mar 13 '19 21:03 phated

@phated Yes. I think so, too. In addition, I think we can include a change for flags.nodeFlags too with the new liftoff stuff.

sttk avatar Mar 14 '19 12:03 sttk

I think we can include a change for flags.nodeFlags too with the new liftoff stuff.

In separate PRs 😛

phated avatar Mar 14 '19 19:03 phated

Sorry. I made a mistake on pushing.

sttk avatar Mar 21 '19 11:03 sttk

@sttk I'm continuing to dig into this and I think the behavior is even more incorrect when initCwd isn't the highest priority. If I run cd my-sub-project; gulp and there's a renamed gulpfile there, my working directory should not be changed to a top-level directory containing a gulpfile.js - as most of these tests indicate.

I'm experimenting with initCwd being the highest priority and also setting the env.cwd if a gulpfile is found in that config.

phated avatar Mar 23 '19 22:03 phated

@sttk I've also found that when using a .gulp.* file to reference a gulpfile in another location, the .gulp.* file relative to that gulpfile isn't resolved. I'm about to push some changes and skipped/failing tests that we need to fix before I think this can land. I'm also not sure how we can fix it without deeper integrations in Liftoff with the config files. I'm open to any suggestions.

phated avatar Mar 23 '19 23:03 phated

@sttk I've just force-pushed a rebase on the lastest master plus my changes where I think our behavior is incorrect. I've tried to note in comments how things aren't working correctly or the temporary solutions to some problems exist. Please let me know what you think of all of this.

phated avatar Mar 23 '19 23:03 phated

@phated This issue is very difficult. Please give me some time to think.

sttk avatar Mar 24 '19 14:03 sttk

@phated Does what your changes suggest that if a config file in initCwd has flags.gulpfile or flags.cwd, gulp-cli should change cwd to env.cwd and then loads .gulp.* file in the changed env.cwd?

It means that flags by config files in initCwd behaves like flags by CLI options?

sttk avatar Mar 25 '19 13:03 sttk

If so, I have two ideas to fix this issue: (The following codes are not actual but an example.)

  1. Process a config file in initCwd before cli.prepare.
var cli = new Liftoff({ ... });
var foundInInitCwd = fined({ path: '.', name: '.gulp', extensions: cli.extensions });
var cfg = { flags: {} };
if (foundInInitCwd) {
  cfg = require(foundInInitCwd.path);
  opts = mergeConfigToCliFlags(opts, cfg);
  if (!opts.gulpfile) { opts.gulpfile = cfg.flags.gulpfile; }
  if (!opts.cwd) { opts.cwd = cfg.flags.cwd || cfg.flags.gulpfile; }
}

cli.prepare({ cwd: opts.cwd, configPath: opts.gulpfile, ... }, function(env) {
  var cfgLoadOrder = ['home'];
  if (env.configFiles['.gulp']['cwd'].path !== foundInInitCwd.path) { cfgLoadOrder.push('cwd'); }
  var cfg = loadConfigFiles(env.configFiles['.gulp'], cfgLoadOrder);
  opts = mergeConfigToCliFlags(opts, cfg);
  env = mergeConfigToEnvFlags(env, cfg, opts);
   ...
  cli.execute(env, handleArguments);

});
  1. Apply the PR #95
var opts = { ... };
var env = {};
var cli = new Liftoff({
    ...
    configFiles: {
      '.gulp': {
        initCwd: {
          order: 1,
          path: opts.cwd || process.env.INIT_CWD,
          onFound: loadAndMergeConfig,

        },
        cwd: {
 
         order: 2,
          path: env.cwd
          onFound: loadAndMergeConfig,

        },
        home: {

          order: 3,
          path: '~',
          onFound: loadAndMergeConfig,

        },
      },
    },
});
cli.prepare({ cwd: opts.cwd, configPath: opts.gulpfile, ... }, function(env) {
   ...
  cli.execute(env, handleArguments);
});

function loadAndMergeConfigs(name, path) {

  var cfg = require(path);
  opts = mergeConfigToCliFlags(opts, cfg);
  env = mergeConfigToEnvFlags(env, cfg, opts);
}

sttk avatar Mar 25 '19 13:03 sttk

@sttk I think the issue is actually that too much filesystem traversal is being made in Liftoff.prototype.buildEnvironment - I'm wondering if we shouldn't move https://github.com/js-cli/js-liftoff/blob/master/index.js#L62-L111 into the execute phase.

The reason I think this is that the cwd config file entry in the following code should actually be searching the process.cwd() (or --cwd or the resolved dirname of --gulpfile if either are provided) https://github.com/gulpjs/gulp-cli/blob/1b80d67d6c4c7b9c80a8f49cda009187a91ad88e/index.js#L43-L54

I believe that is what you were trying to achieve by adding initCwd config file entry. If config files were always started at the cwd the same way that gulpfiles are looked up, before we attempt to look up the gulpfile, I believe the issue would be solved. Though, there might be an issue with a .gulp.* file existing at a different place in the tree, but traversing up the filesystem should handle that, I think.

phated avatar Mar 26 '19 03:03 phated

I'm wondering if we shouldn't move https://github.com/js-cli/js-liftoff/blob/master/index.js#L62-L111 into the execute phase.

Rather, that moving may be necessary because there is a case that gulp cannot be run only by changing cwd and gulpfile. It's needed to set env.modulePath and env.modulePackage too by executing those part in the link.

$ find . -path './b/node_modules/*' -prune -o -print
.
./a
./a/.gulp.json    => `{ "flags.gulpfile": "../b/gulpfile.js" }`
./a/gulpfile.js    // This is a dummy gulpfile to make gulp-cli load `a/.gulp.json`
./b
./b/gulpfile.js
./b/node_modules
./b/package.json

$ cd a
$ gulp --gulpfile ../b/gulpfile.js
[00:23:11] Working directory changed to ~/path/to/b
[00:23:11] Using gulpfile ~/path/to/b/gulpfile.js
[00:23:11] Starting 'default'...
[00:23:11] Finished 'default' after 2.71 ms

$ gulp
[00:24:57] Local gulp not found in ~/path/to/a
[00:24:57] Try running: npm install gulp

sttk avatar Mar 27 '19 14:03 sttk

Furthermore, the current gulp with --cwd option loads and processes .gulp.* in the chaged directory. I think this behavior is correct, but to reproduce the same behavior with a config file it is needed to load and process .gulp.* in initCwd before resolving paths of other config files.

$ find . -path './b/node_modules/*' -prune -o -print
.
./a
./a/.gulp.json    => `{ "flags.gulpfile": "../b/gulpfile.js" }`
./a/gulpfile.js
./b
./b/.gulp.json    => `{ "flags.require": "ansi-colors" }`
./b/gulpfile.js
./b/node_modules
./b/package.json

$ cd a
$ gulp --cwd ../b
[23:38:20] Requiring external module ansi-colors     // => Load and process b/.gulp.json
[23:38:20] Working directory changed to ~/path/to/b
[23:38:20] Using gulpfile ~/path/to/b/gulpfile.js
[23:38:20] Starting 'default'...
[23:38:20] Finished 'default' after 2.4 ms

sttk avatar Mar 27 '19 15:03 sttk

@sttk I believe this needs conflicts resolved since I merged #239

phated avatar Jan 07 '24 20:01 phated