Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parent argv #4

Closed
wants to merge 6 commits into from
Closed

Parent argv #4

wants to merge 6 commits into from

Conversation

dylancwood
Copy link

More background can be found at the original issue in the grunt repository: gruntjs/grunt#1172

This patch turned out to be far less simple than I originally thought, but it is working well now, thanks to unit-testing!!

Motivation:

When spawning a child process using grunt.util.spawn with the options grunt: true, the child process should inherit any command line flags passed to the parent process. Most importantly, the --debug and --no-write flags should always be passed to any children to prevent unwanted behavior.

Original solution:

It seemed so simple at first:

I modified grunt-legacy-utils/index.js as follows to ensure that the parent's CLI args were passed to the child:

- args = process.execArgv.concat(process.argv[1], opts.args);
+ args = process.execArgv.concat(process.argv[1], opts.args, process.argv.slice(3));

Problem(s) with original solution:

  1. Assumes that the task argument is at position two of the process.argv array. This is wrong because the task may not even be in the arguments, and if it is, there is no guarantee that it will be the third element in the array.
  2. Does not account for --gruntfile path/to/gruntfile: luckily, this was a heavily used paradigm in the unit tests, and I caught this problem when the tests that use --gruntfile failed. The reason that --gruntfile causes failures with the original solution is that the --gruntfile argument was the third argument in the args array, and was therefore being stripped. As a result, the gruntfile path was being interpreted as a task name, which was undefined.
  3. Does not account for relative --base paths. For instance, if a parent process is invoked from /var/www with grunt default --base path/to/somewhere, then the child process will attempt to calculate it's base as /var/www/path/to/somewhere/path/to/somewhere, which is probably not the user's intention.

New Solution:

Instead of simply chopping off the first three arguments, we now need to filter them to remove the task name(s). This can be accomplished by removing any arguments that do not start with a -.

Since filepaths do not start with a -, the will be stripped from the arguments during filtering. This is a problem when it comes to the file paths specified after the --gruntfile arguments, which we want to preserve (or at least I assume that we do). To solve this problem, I created a whitelist of arguments whose next argument should always be allowed to stay. This allows the argument immediately following a --gruntfile argument to remain in the args list even if it does not start with a -.

To address the issue of relative --base paths, I implemented a blacklist. Any arguments on the blacklist will be stripped during the filtering process. Since the file path associated with the blacklisted --base argument probably does not start with a -, it will be removed as well with no further logic required. There is no need to pass the --base argument to the child process, as it will be spawned from the parent's base path by default.

If a process invoked with a --gruntfile argument spawns a child, and provides different --base or gruntfile arguments to the spawn method, then the new arguments will override the parent's. This is because the parent's arguments are inserted in the args array before the opts.args arguments are, and thus any arguments ins opts.args will override the parent's arguments.

Testing

I've added three tests to ensure that this feature is working properly:

  • Test to be sure that a --no-write flag is passed automatically from a parent to a child grunt process.
  • Test to be sure that the --gruntfile flag and its associated value are passed to the child process.
  • Test to be sure that the --base flag and its associated value are passed to the child process.

Conclusion

This proved to be more complicated than I originally anticipated. I hope that there are not any more gotchas or use-cases that this change would cause problems with. An alternative, simpler solution would be to only pass specific arguments from parent to child. For example: only pass --debug, --verbose and --no-write, as those are the most likely to cause irreversible problems if they are not passed. Failing to pass the other flags may cause a child process to fail or stop, however, the developer can manually add any needed flags to the opts.args as needed.

I eagerly await your review.

var pathSeparatorRe = /[\\\/]/g;
// List of args that are followed by a corresponding value
var argsWithValues = ['--gruntfile'];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since filepaths do not start with a -, the will be stripped from the arguments during filtering. This is a problem when it comes to the file paths specified after the --gruntfile arguments, which we want to preserve (or at least I assume that we do). To solve this problem, I created a whitelist of arguments whose next argument should always be allowed to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants