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

Ensures windows extension less spawns run as .cmd #5

Closed
wants to merge 1 commit into from
Closed

Ensures windows extension less spawns run as .cmd #5

wants to merge 1 commit into from

Conversation

pflannery
Copy link

Updated version with a major to 3.0.0

This merge defaults all extension less command lines to spawn using '.cmd' instead of the default '.exe'.
This gives windows users the ability to run commands like npm init and other npm package cmd's without having to write the extension just like linux and mac users enjoy using shebangs.

It's a pain that every time we want to run a command in node on windows we have to fiddle the command line when it's not an .exe file. It just adds annoying complexity to our cross-os-compatible scripts.

By creating .cmd wrappers for exe's and placing them in a common path (i.e. %userprofile%\appdata\roaming\npm) they too can be run without specifying the ext. so this change seems the logical choice to me.

will fix chainyjs/chainy-cli/issues/4

@pflannery
Copy link
Author

I've refactored this pr to become:

Given an absolute path on windows os
and the command in the path has no extension specified
then attempt to discover the command with [.exe, .cmd, .bat] as the path
otherwise if an extension is specified
then leave as is

@balupton I think I originally went a bit too far by defaulting to .cmd but now it attempts to discover the path and gives .cmd.bat priority over .exe

if this is good then I will squash and change the version as it's not really a major change, it's more like a minor

@pflannery pflannery changed the title Ensures windows extention less spawns run as .cmd Ensures windows extension less spawns run as .cmd Jul 11, 2014
@pflannery
Copy link
Author

I've noticed that when determining paths that are absolute and have spaces in them causes an issue with the determineAbsoluteWindowsCommand because they are not enclosed in quotes.

So I believe the fix for this is to ensure that all determined paths get enclosed in quotes before using them. i.e. "c:\program files (x86)\git\bin\git.exe". I will look in to this

@balupton
Copy link
Member

I wonder if there is a less intense way of accomplishing this?

Right now getExecPath scans requires a relative path as it tries all of the environment paths.

Perhaps getExecPath needs to be split out into two different functions:

  1. One function for trying environment paths
  2. One function for trying extensions

With a flow something like this:

  1. If we have a relative path, try the environment paths
  2. With a absolute path, try extensions

@pflannery
Copy link
Author

I've replaced the code and added a method called getExecExt which calls getPossibleExecPaths and determineExecPath to try to discover paths with possible extensions [.cmd, .bat, .exe]

the flow is the following:

During getExecPath 
and its running on windows
and the path has no extension
then use getExecExt to try determine the correct path using [.cmd, .bat, .exe]

I've squashed all the commits to just one

fixes the ext detection issue in getPossibleExecPaths
@pflannery
Copy link
Author

I found an issue in getPossibleExecPaths when trying to determine if there was an extension on windows. Sometimes paths have '.' in them yet no extention so just swapped the indexof check to become a path.extname check

@pflannery pflannery closed this Mar 27, 2015
@balupton
Copy link
Member

@pflannery did this get merged? why the close?

@pflannery
Copy link
Author

@balupton i did a purge of all my very old PRs that never got attention and just presuming they weren't wanted\needed by me or the repo owner

@balupton
Copy link
Member

@pflannery ah k, yeah we may still want to merge this - just haven't got around to it with everything else that is going on

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.

2 participants