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

win: perform case insensitive PATH= comparison #1837

Merged
merged 1 commit into from May 9, 2018
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 9, 2018

If I'm reading nodejs/node#20605 properly, then it seems that this comparison should be case insensitive. Untested locally, but that's what the CI is for.

@bzoz
Copy link
Member

bzoz commented May 9, 2018

@@ -831,7 +831,7 @@ int make_program_env(char* env_block[], WCHAR** dst_ptr) {
*/
static WCHAR* find_path(WCHAR *env) {
for (; env != NULL && *env != 0; env += wcslen(env) + 1) {
if (wcsncmp(env, L"PATH=", 5) == 0)
if (_wcsicmp(env, L"PATH=", 5) == 0)
Copy link
Member

Choose a reason for hiding this comment

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

_wcsicmp() is affected by calls to setlocale(). I'd just write it out in full to avoid any confusion:

if ((env[0] == L'P' || env[0] == L'p') &&
    (env[1] == L'A' || env[1] == L'a') &&
    // etc

@JamesMGreene
Copy link

JamesMGreene commented May 9, 2018

Thanks for the quick turnaround on getting this change underway, @cjihrig. 👍

@cjihrig
Copy link
Contributor Author

cjihrig commented May 9, 2018

Good enough CI with Ben's suggestion: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/831/

@richardlau
Copy link
Contributor

Should we note in the docs what happens on Windows when more than one variant of PATH is in options->env (e.g., PATH and Path)? make_program_env sorts, and this change will use the first variant found after sorting. It's an edge case, but is not hard to do in Node.js if you copy process.env and inadvertently set a different variant in the copy.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 9, 2018

@richardlau would you be willing to open a docs PR? You seem to have a pretty good handle on what you'd like them to say.

Refs: nodejs/node#20605
PR-URL: libuv#1837
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@richardlau
Copy link
Contributor

@richardlau would you be willing to open a docs PR? You seem to have a pretty good handle on what you'd like them to say.

Yes, but I'd like to convince myself first that the behaviour is actually desired. Copying from nodejs/node#20605 (comment):

This is kind of a note for myself since I haven't had time to look into this further, but thinking about this a bit more, I'm wondering if the libuv behaviour of using the passed in env.PATH (or env.Path after #1837) on Windows to find the command to execute is correct. I don't think the Unix platforms behave this way (need to check) and the passed in env is given to the child process but not used to execute the child process?

@Embraser01 Embraser01 mentioned this pull request Apr 1, 2019
lundibundi added a commit to lundibundi/node that referenced this pull request Mar 4, 2020
addaleax pushed a commit to nodejs/node that referenced this pull request Mar 11, 2020
Fixes: #20605
Refs: libuv/libuv#1837

PR-URL: #32091
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Mar 11, 2020
Fixes: #20605
Refs: libuv/libuv#1837

PR-URL: #32091
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Mar 11, 2020
Fixes: #20605
Refs: libuv/libuv#1837

PR-URL: #32091
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to nodejs/node that referenced this pull request Apr 22, 2020
Fixes: #20605
Refs: libuv/libuv#1837

PR-URL: #32091
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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

5 participants