Add support for Unicode characters in paths

[owl-from-hogvarts]

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

On Windows 10 i discovered an issue that if there are
Unicode letters or symbols in path
for Python, find-python.js script can't find it.
This bug is caused by an encoding issue which (i hope)
I have fixed. At least this bug fix works for me.

Have changed:

• modified: gyp/pylib/gyp/easy_xml.py
  • Python 2.7 handle xml_string in the wrong way
    because of encoding (line 128)
• modified: gyp/pylib/gyp/input.py
  • if "encoding:utf8" on line 240 is not marked it causes an error
• new file: lib/find-python-script.py
  • I have created this file for convenience.
  • Script which can handle Unicode characters
    letters and symbols can't fit on one line
    because it is necessary to specify
    instructions for several versions of python
• modified: lib/find-python.js
  • to make js understand Python (line 249)

[cclauss]

Please do not request reviews until all tests are green.

[owl-from-hogvarts]

Please do not request reviews until all tests are green. @cclauss

At first, can i edit tests?

At second, in my opinion, they are looking not good becouse find-python.js script is not looking good (may be i dont know something).

and in therd, i just dont know how to test it (as previos author). In such way i am fixing complex bug, complex becouse it is formed inside python it self... may be in python in symbiosis with windows. in this case we cant use unit tests. we should use something bigger or more complex...

the bug is in that python when called from, another process (in our case node.js) doesn't specifing default encodng for stds streams to utf8. then we just reconfiguring these streams to right encoding. and in case of python 2.7 it even return sys.executable in wrong encoding.

in general, i dont know how to test it and what to do next.

Now work in progress. I will publish commits soon.

[owl-from-hogvarts]

@cclauss
if i merge changes with master then test/test-addon.js is breaking. also Error: Could not locate the bindings file. Tried

[owl-from-hogvarts]

@richardlau, @DeeDeeG, @cclauss can some one pls review my code or at least approve to run workflows?

[owl-from-hogvarts]

can some one pls review my code or at least approve to run workflows?

thanks)

[DeeDeeG]

@owl-from-hogvarts I'm not sure why the tests/CI is not running here. (Reminder: I am not part of the node-gyp team, so I personally can't fix CI here.) But you can run the tests at your fork if you want. Just enable the CI actions here: https://github.com/owl-from-hogvarts/node-gyp/settings/actions (and you might need to push a commit after the workflows are enabled, to trigger a new CI run.)

Edit: Okay, someone appears to have triggered the CI manually, after being notified of these comments.

[owl-from-hogvarts]

@DeeDeeG thank you for smart idea)

[owl-from-hogvarts]

@rvagg hi) can you pls review this PR and if possible, merge it)

[DeeDeeG]

@owl-from-hogvarts hmm, there is a merge conflict from my PRs. Sorry about that.

I believe this pull request has changed lib/find-python.js to have Windows line endings (CRLF). And this PR changed/added some other files with mixed or Windows (CRLF) line endings. Can you please change them all back to Unix line endings (LF), for consistency with the rest of the repository? Or I can help with that, by opening a PR at your fork to change them back.

Using Unix line endings (LF) solves the merge conflict.

[owl-from-hogvarts]

@DeeDeeG now i can't see any merge conflicts. In lib i have changed endings to unix type

[DeeDeeG]

@owl-from-hogvarts this branch has overwritten/undone the change I made in #2375.

I suppose you can re-apply it by doing git cherry-pick -x fca4795512c67dc8420aaa0d913b5b89a4b147f3.

Note about merge conflicts (click to expand if you want)

(I guess I should clarify briefly: All of this is not really relevant to this PR right now. The git cherry-pick command above would solve things for this PR at the moment.)

Maybe this doesn't need to be said, and maybe it was just an oversight. Anyhow, I say this to give a friendly hint:

When encountering merge conflicts during a git merge or a git rebase, it is good to carefully examine the conflicting code from both branches, and try to include the logical changes from both branches into the final result that you resolve. (And once you are done resolving the conflict, git add/git commit the file with the resolved changes.) (A generic guide, regardless of your text editor or IDE: https://www.git-tower.com/learn/git/faq/solve-merge-conflicts/)

I personally like to resolve complex merge conflicts with the Atom text editor: https://flight-manual.atom.io/using-atom/sections/github-package/#resolve-conflicts, but I have found that the interface there shows what's going on so clearly, over time I have become familiar enough with the process that I have become comfortable doing this with a basic text editor like Notepad or nano if I need to. I think Atom's merge conflict interface is a good learning tool, not just a quick solution for the problem.

It is probably too late to resolve the merge conflict the usual way on this branch, as the "resolution" of the conflicts is baked into the rebased commits now. It would require some complex interactive rebasing (git rebase -i origin/master), but that is a bit of an advanced topic... https://about.gitlab.com/blog/2020/11/23/keep-git-history-clean-with-interactive-rebase/ and not recommended to try unless you are bored or curious, and interested in learning more command-line git usage. And if you do try it, please keep a backup of your current branch in a good state: git branch pr_2254_backup master. (See: another resource: http://think-like-a-git.net/sections/experimenting-with-git/branches-as-savepoints.html).

Sorry this got really long. I dunno, I am a nerd about git.

[owl-from-hogvarts]

@DeeDeeG i have done this!!! your changes are picked up. No merge conflicts

[owl-from-hogvarts]

https://github.com/nodejs/node-gyp/pull/2254/checks?check_run_id=2634323722#step:9:26
https://github.com/nodejs/node-gyp/pull/2254/checks?check_run_id=2634323375#step:9:44

@DeeDeeG @cclauss as i understand, tests are not passing due to timeout. So does this mean that node-gyp works too slow? or it may be an error internally with some kind of infinite loop or never ending awaiting or so?

[cclauss]

I re-ran the tests and all pass now. My sense is that we have at least one flaky test.

I approve the Python changes which are reasonable and minimal but someone else needs to carefully review and approve the JavaScript changes and there are a lot of them. Are all of these changes required to support Unicode pathnames?

[owl-from-hogvarts]

Thank you very much)

In fact in js part only one line change is strictly necessary, but that one line breaks all old tests. I think i can use new tests but with old source of find-python.js. But i recommend to preserve all changes because in my opinion (which is most likely biased) it contain important improvments on extensability and readability.

[cclauss]

My strong recommendation would be to move all improvments on extensability and readability to a separate PR called improvments on extensability and readability so that we can get this PR reviewed and landed without a lot of unrelated changes.

[owl-from-hogvarts]

@cclauss i have folowed your advice: now node-gyp use old find-python.js but new tests because old are broken for this changes

[owl-from-hogvarts]

@rvagg @DeeDeeG @joaocgreis PLEASE review this PR!

[owl-from-hogvarts]

@joaocgreis also what the purpose of running some checks in find-python.js on windows with shell mode on?

[owl-from-hogvarts]

@rvagg @joaocgreis hey! Will this PR be reviewed some day? The year has passed since this PR was opened. There is really not so much to review. In JS part only tests were changed.

[rvagg]

I'd like to see this resolved but I don't have the expertise, either on Python or Windows, to make a call either way.

Does someone else here feel confident enough this is a reasonable change? @nodejs/platform-windows maybe?