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

Fixed commandExists() not working for absolute path on windows #24

Merged
merged 5 commits into from
Apr 15, 2020

Conversation

Antyos
Copy link
Contributor

@Antyos Antyos commented Mar 28, 2020

This fixes issue #23, where the RegEx check that causes commandExistsWindows async/sync to return early for commands with an absolute path, e.g. C:\Program Files\...

Also included absolute paths test in test.js and replaced dir with xcopy because dir is not a command that has an associated file. where dir will never return true on Windows.

@mathisonian
Copy link
Owner

Thanks @Antyos! Any idea why the absolute path test is failing on Travis? Happy to merge once tests are passing.

@Antyos
Copy link
Contributor Author

Antyos commented Mar 30, 2020

It looks like it failed on the check to find a command that does exist with an absolute path, how strange. I would guess it has something to do with the format of absolute paths on Linux. I'll look into it when I have a chance later this week.

Thanks!

@mathisonian
Copy link
Owner

Sounds good, thanks @Antyos!

@Antyos
Copy link
Contributor Author

Antyos commented Apr 1, 2020

All fixed! The test code I initially added tried to get Linux to run a .cmd file, which of course can't run on Linux... I've switched it to the .js file.

As a side note, I noticed that the test code runs about 20x slower on a Windows machine than a Linux one, even before my changes. I looked into it a bit and I think this is an issue with Node's exec function, not this code. I'm still fairly new to Nodejs/npm/JavaScript, so I'm not sure if it's worth looking into further.

Also, npm install said that some of this project's dependencies have known vulnerabilities. Is it worth updating the dependencies to fix the vulnerabilities, or is it better to leave things as they are?

@Antyos
Copy link
Contributor Author

Antyos commented Apr 14, 2020

Hey @mathisonian, I just wanted to check in and see if we're still good with the pull request. I'm sure you've been busy the last couple of week, so no worries if this got buried in your inbox.

@mathisonian
Copy link
Owner

Hey thanks for pinging me on this @Antyos. The changes look good. I'm not sure about about the windows exec performance thing; I think that and dependencies are separate issues, we can address in a different PR.

GitHub isn't alerting me about any vulns but NPM may be ahead, I will look into that .

@mathisonian mathisonian merged commit 9c0f4c1 into mathisonian:master Apr 15, 2020
@mathisonian
Copy link
Owner

To follow up re: dependencies -- it looks like all the issues are coming from subdependencies of mocha. This code is only used for the test suite, so it shouldn't affect end users of this library, but we should still update.

@Antyos
Copy link
Contributor Author

Antyos commented Apr 15, 2020

Thanks for merging it @mathisonian! You should also be able to close issue #23 now.

Also, do you plan to update Mocha or does that need a pull request?

@mathisonian
Copy link
Owner

No problem, thanks @Antyos! A PR for mocha would be appreciated if you have the chance, otherwise I will get to it, but no promises on how quickly (I'm a bit bogged down at the moment).

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