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

Windows-specific command name, arguments, and pass through #125

Closed
wants to merge 1 commit into from

Conversation

adamralph
Copy link
Owner

@dtitenko @damianh @pawelros @leastprivilege this is an idea to allow workarounds for the problems described in #119 and elsewhere.

It adds some optional params which allow you to do a few things.

Specify a specific command for Windows only

Run("path/to/foo", "arg1 arg2", windowsName: "path/to/foo.exe");

Specify specific args for Windows only

Run("path/to/foo", "arg1 arg2", windowsArgs: "arg1 arg2 windows-is-great");

Pass the command name and args straight through, without using cmd.exe

Run("path/to/foo", "arg1 arg2", windowsPassthrough: true);

Using an appropriate combination of these optional params, it should be possible to work around any Windows weirdness to do with exe vs bat vs cmd, npm wrappers like yarn, quotes, PATH, etc. when required. The methods can continue to be used as they are today, but when you really need full control, you can have it.

What do you think?

@adamralph adamralph added the enhancement New feature or request label May 17, 2019
@leastprivilege
Copy link

Even if it is a breaking change - wouldn't it even be nicer if not using cmd.exe at all by default - and rather opt-in if needed?

@adamralph
Copy link
Owner Author

adamralph commented May 20, 2019

@leastprivilege that did cross my mind but I discarded it because, IIRC, using cmd.exe more often solves problems than causes them (that's why we introduced it in the first place). But now that I think about it again, it would probably would be a better to stop using it, since the windowsName and windowsArgs params allow you to opt-in to cmd.exe if you want to. Then we don't even need a param to opt-in or opt-out of it.

I'll sleep on it and give others chance to chime in, but right now I'm leaning towards this breaking change. Thanks!

@damianh
Copy link
Contributor

damianh commented May 21, 2019

I too first suggested that using cmd should be opt-in (though don't know how to do it in nice way). But I do agree that using cmd.exe solves more problems than it creates.

My (weakly held) vote is for number 3 so it can be used for exceptions. The problem with all this though is that it's not pit-of-success. It won't be clear from failures that a user should attempt the pass through.

Perhaps an alpha version with all 3 so we can kick the tires?

@adamralph
Copy link
Owner Author

adamralph commented May 21, 2019

@damianh what do you mean by this (emphasis mine)?

My (weakly held) vote is for number 3 so it can be used for exceptions.

BTW, the current proposal is not a choice between three proposals. All three optional parameters have been added in this PR.

But my preference right now is to remove all the cmd.exe and double quotes magic from SimpleExec entirely, and introduce only the windowsName and windowsArgs parameters, so that the consumer has full control.

@damianh
Copy link
Contributor

damianh commented May 22, 2019

so it can be used for exceptions.

I mean that you use is only in exceptional situations.

@adamralph
Copy link
Owner Author

Closing in favour of #128.

@adamralph adamralph closed this Jun 1, 2019
@adamralph adamralph deleted the windows-unicorns branch June 1, 2019 08:29
@adamralph adamralph added the wontfix This will not be worked on label Jun 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants