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

Add Python symlink to path (for non-Windows OSes only) #2362

Merged
merged 6 commits into from
Jun 10, 2022

Conversation

DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Apr 8, 2021

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

An attempt to fix #2351 for the rest of users not yet addressed by #2355.

  • Symlink the valid Python binary found by lib/find-python.js at build/node_gyp_bins/python3
  • Add build/node_gyp_bins to the PATH just before building
    • Ensures any of the build-time Python scripts (such as build/gyp-mac-tool or build/gyp-flock-tool) will be run with the Python binary validated bylib/find-python.js
More notes/thoughts/explanation

The corner case/bug is not applicable to Windows. So this PR changes nothing on Windows.

This PR changes nothing on Windows. The bug being fixed is Not Applicable to Windows; There are no Python scripts to run at build time on Windows, per my testing, nor as far as I can deduce from research.

(If someone can prove that there are ever Python scripts at build-time on Windows, or the maintainers prefer to do something on Windows just in case, I can work on a solution involving writing a batch script to build/node_gyp_bins/python3.bat that runs the validated Python binary from lib/find-python.js. That should work just as well. Non-administrator users cannot create symlinks by default on Windows, so in my opinion we can't rely on symlinks on Windows. But again, I don't think there's any Python to run during the build phase on Windows. So this all should be moot!)

Who is this for?

Specifically, folks with older or somehow invalid python3 (< 3.6) first on their PATH, who have nevertheless managed to supply a newer, valid Python binary to node-gyp, probably by command-line --python=/some/python3 or as an env var PYTHON=/some/python3.

(Note that Debian Jessie and Stretch, as well as Ubuntu Xenial all ship python3 older than 3.6. Perhaps these users will get newer Python3 and opt not to put it early on the PATH?)

What does this PR achieve? A user with older Python on their PATH who manages to supply a valid Python version to node-gyp and make it through the configure stage (including lib/find-python.js) should not abruptly hit errors at the build stage. This PR aims to solve that corner case. (See: #2351 for details.)

Where did the idea come from? This was inspired by the approach that nodejs/node takes when setting up the build for NodeJS itself. See configure.py at that repository:

@DeeDeeG DeeDeeG marked this pull request as draft April 8, 2021 07:53
@DeeDeeG DeeDeeG changed the title Add Python symlink to path (UNIX-only) Add Python symlink to path (for non-Windows OSes only) Apr 9, 2021
@DeeDeeG DeeDeeG marked this pull request as ready for review April 9, 2021 01:05
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Apr 9, 2021

Hi folks, just wanted to mention the other solution I found for fixing this bug. It involves patching gyp's Makefile generator to record the absolute path to the Python binary we want into the generated Makefile. (And check if that binary still exists at build time, with command -v /path/to/python3, otherwise fall back to whatever's first on the PATH again.)

See this commit at my fork for the alternative fix: DeeDeeG@a68d184

Weighing the pros and cons of this alternate approach. (Click to expand)

Pros:

  • I think the Makefile generator patch is more surgically precise than adding a new directory to the PATH.

Cons:

  • but maintaining node-gyp-specific patches to gyp is more involved, and the patch might or might not be suitable to upstream into gyp-next. That's a a whole additional conversation that would need to be had before going with that approach.
    • (Where to land this? Here at node-gyp or upstream in gyp-next? How to maintain the patch if not upstreamed? Consider any potential implications for various gyp consumers such a nodejs/node itself and various users of gyp-next, e.g. via the pip package.)
  • Not tested on the AIX or Solaris (SmartOS) OSes, where the flock stuff apparently comes into play.
  • If we ever enable the Ninja generator with node-gyp, a similar approach would be needed patching that generator as well. (See feat: support gyp with ninja #2171 for draft ninja support).

I am open to either approach, but this PR currently has the solution that is the more general one (no need to patch each individual generator, just set the environment up the way we want and it all works after that), and one which is easier to land without considering implications for gyp-next and its consumers.

Also, I can acknowledge that being "surgically precise" also means getting more of the precise details right, as opposed to throwing the desired Python on the PATH and being done. I also would want this to be better tested on SmartOS and AIX. I am not aware of a way to test on AIX. Testing on SmartOS is a pain, but doable in VirtualBox if I get some time.

(Overall I'm leaning toward the symlinks, which is why I submitted that approach in this PR.)

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Apr 11, 2021

I added a couple of commits just now to pare down redundant information in the warning output and improve its formatting. (Although, of course, in most cases I expect there will be no warnings at all.)

Before (click to expand)
# ...
gyp info find Python using Python version 3.10.0 found at "/usr/local/bin/python3"
gyp WARN python symlink Error: EACCES: permission denied, unlink '/Users/[user]/superstring/build/node_gyp_bins/python3'
gyp WARN python symlink  error when attempting to remove existing symlink
gyp WARN python symlink  [Error: EACCES: permission denied, unlink '/Users/[user]/superstring/build/node_gyp_bins/python3'] {
gyp WARN python symlink   errno: -13,
gyp WARN python symlink   code: 'EACCES',
gyp WARN python symlink   syscall: 'unlink',
gyp WARN python symlink   path: '/Users/[user]/superstring/build/node_gyp_bins/python3'
gyp WARN python symlink }
gyp WARN python symlink Error: EEXIST: file already exists, symlink '/usr/local/bin/python3' -> '/Users/[user]/superstring/build/node_gyp_bins/python3'
gyp WARN python symlink  error when attempting to create Python symlink
gyp WARN python symlink  [Error: EEXIST: file already exists, symlink '/usr/local/bin/python3' -> '/Users/[user]/superstring/build/node_gyp_bins/python3'] {
gyp WARN python symlink   errno: -17,
gyp WARN python symlink   code: 'EEXIST',
gyp WARN python symlink   syscall: 'symlink',
gyp WARN python symlink   path: '/usr/local/bin/python3',
gyp WARN python symlink   dest: '/Users/[user]/superstring/build/node_gyp_bins/python3'
gyp WARN python symlink }
gyp info spawn /usr/local/bin/python3
# ...
After (click to expand)
# ...
gyp info find Python using Python version 3.10.0 found at "/usr/local/bin/python3"
gyp WARN python symlink error when attempting to remove existing symlink
gyp WARN python symlink Error: EACCES: permission denied, unlink '/Users/[user]/superstring/build/node_gyp_bins/python3' errno: -13
gyp WARN python symlink error when attempting to create Python symlink
gyp WARN python symlink Error: EEXIST: file already exists, symlink '/usr/local/bin/python3' -> '/Users/[user]/superstring/build/node_gyp_bins/python3' errno: -17
gyp info spawn /usr/local/bin/python3
# ...

(These example warnings were generated by changing the owner of the symlink to root before running node-gyp configure again. sudo chown -R root build/node_gyp_bins.)

I don't want these warnings to be too scary or attract too much attention in case we go on to successfully build the module. I just want them to be there to let folks know what's going on behind the scenes, and in case of an eventual build failure along the lines of #2351.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Apr 11, 2021

Questions for reviewers:

  • Should this PR use rimraf instead of fs.unlink() to remove existing symlinks?

I have gone with fs.unlink() for simplicity, and because it looked like the suitable option from browsing the Node fs API documentation. But I notice elsewhere in the repository the code uses rimraf to remove files/dirs. Apparently rimraf is a more sophisticated and reliable way to remove a file? Hopefully the symlink is not in use at configure time, so the chances of errors are lower. But I can use rimraf if we want to make a stronger effort to remove the symlink, and to be consistent with the rest of the project, at the cost of bringing a slightly more heavy-duty tool to bear on a pretty simple and usually optional task. I would go either way, but sticking with my first instinct fs.unlink() unless advised otherwise. Thanks for considering.

  • Should this PR skip making the symlink unless the Python from lib/find-python is different from the first python3 on the PATH? (Which would closely mimic what they do over at nodejs/node)

I noticed that the nodejs/node repository only makes the symlink if the Python used is somehow different from the first python on the PATH. (In our case, it would be python3, not python, but otherwise it's the same idea. For node-gyp, that scenario implies that the user has manually indicated to node-gyp some version of Python to use, or it is on the PATH as python and would not be found by gyp-mac-tool's python3 shebang line. So these are the real situations where we know the symlink would be useful. We can avoid introducing the symlink in other cases.) I think that's probably a good idea, so I want to explore that and potentially do so here before this is merged. Please let me know any comments for or against, or any clarifying questions you may have about this. Thanks.

Implementation ideas:

  • Update find-python.js to additionally return a variable indicating whether the "found Python" was found via checking python3 on the PATH -- if not, we can make the symlink.
  • Copy the approach from nodejs/node more directly: Use the JS which module to find the first python3 on the PATH, if any, then use fs.realpath() to get the real locations at the bases of any symlink chains for said python3 from the PATH and python from lib/find-python.js. Compare. If they are the same, no symlink needed. If they are different, we can make the symlink.

@imatlopez
Copy link
Contributor

I'd stick with rimraf, symlinks in windows are finicky at best so I'd go for the solution that has worked so far.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Apr 14, 2021

rimraf() vs fs.unlink(): I suppose rimraf() is still very fast to execute. I wonder if it is slower to load...? Perf impact is probably low, I just have a hard time benchmarking load time. The "optimize it" part of me wants to use fs.unlink(), but the hyper-reliability part of me wants to use rimraf().

Update: A quick microbenchmark shows that loading and using rimraf appears to slow things down by... 0.02 seconds (20 milliseconds) over just loading/using the fs module's unlink(), in order to remove a single symlink. So this is within the realm of measurable results, but not awful. Measured with ln -s target symlink && time node delete-symlink.js on my old but serviceable MacBook Pro with spinning HDD. Result: ~60 milliseconds with fs.unlink(), ~80 milliseconds with rimraf().

@imatlopez thanks for the feedback. But this PR isn't doing symlinks on Windows, only on other OSes.

More on why symlinks are weird on Windows, but this is a bit off-topic

Yeah, I agree symlinks on Windows are weird from what I've heard. And they require Administrator permissions or a special "Developer Mode" toggle in the Settings app, so the vast majority of people probably can't use symlinks on Windows.

@owl-from-hogvarts
Copy link
Contributor

Non-administrator users cannot create symlinks by default on Windows, so in my opinion we can't rely on symlinks on Windows

But in new tests in #2254 (test/test-find-python.js:321) there is check that make symlink (to be honest junction) to python's directory and it works well

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Jul 4, 2021

@cclauss @gengjiawen this would help users with Python 3.5 or older first on the PATH and newer Python 3.x located somewhere else. (Debian 9 users maybe?)

(Without this PR, for those users, gyp will ignore the compatible/new enough version of Python; Instead, it would use the first python3 on the PATH. This is due to the generated Makefile invoking gyp's Python files directly, which simply interprets gyp's #!/usr/bin/env python3 shebang lines. As mentioned, the first python3 on the PATH may or may not be new enough. This PR ensures a symlink to the Python that python-finder okayed is on the PATH.)

Thoughts? Please take a look, thank you.

@DeeDeeG DeeDeeG mentioned this pull request May 27, 2022
4 tasks
@cclauss
Copy link
Contributor

cclauss commented May 27, 2022

Please fix the conflicts. Our minimum Python is now 3.7 in alignment with README.md and https://devguide.python.org/#status-of-python-branches. How does this code ensure we are not building a symlink to an invalid python3?

I like this approach because it is JavaScript-based. We improve maintainability when we remove Python code from gyp.

lib/build.js Show resolved Hide resolved
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented May 28, 2022

Our minimum Python is now 3.7 in alignment with README.md and https://devguide.python.org/#status-of-python-branches. How does this code ensure we are not building a symlink to an invalid python3?

That is a matter handled in node-gyp/lib/find-python.js.

(This PR makes a symlink to whatever copy of Python find-python.js found.)

Edit: On another note, merge conflicts are fixed.

Helps to ensure a version of Python validated by lib/find-python.js
is used to run various Python scripts generated by gyp.

Known to affect gyp-mac-tool, probably affects gyp-flock-tool as well.

These Python scripts (such as `gyp-mac-tool`) are invoked directly,
via the generated Makefile, so their shebang lines determine
which Python binary is used to run them.
The shebang lines of these scripts are all `#!/usr/bin/env python3`,
so the first `python3` on the user's PATH will be used.

By adding a symlink to the Python binary validated by find-python.js,
and putting this symlink first on the PATH, we can ensure we use
a compatible version of Python to run these scripts.

(Only on Unix/Unix-like OSes. Symlinks are tricky on Windows,
and Python isn't used at build-time anyhow on Windows,
so this intervention isn't useful or necessary on Windows.

A similar technique for Windows, no symlinks required,
would be to make batch scripts which execute the target binary,
much like what Node does for its bundled copy of npm on Windows.)
Add missing functions "unlink()" and "symlink()" to mocked module.
Warn users about errors, but continue on in case the user does
happen to have new enough Python on their PATH.

(The symlinks are only meant to fix an issue in a corner case,
where the user told `node-gyp` where new enough Python is,
but it's not the first `python3` on their PATH.
We should not introduce a new potential failure mode to all users
when fixing this bug. So no hard errors during the symlink process.)
Logging the entire error object shows the stack twice,
and all the other information is contained in the stack.

It also messes with the order of what is logged.

Rather than logging a bunch of redundant information in a messy way,
we can log only the stack. Logging it in a separate log.warn()
also gets rid of an extra space character at the beginning of the line.
This info (err.errno) is the only piece of information
in the error object that is not redundant to err.stack.
These messages aren't important enough to be `log.warn`s.

Log as verbose only; they will also appear in full error output.
@DeeDeeG DeeDeeG force-pushed the add-Python-symlink-to-PATH branch from e3de45d to 54328e1 Compare May 28, 2022 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python syntax error in dynamically generated 'build/gyp-mac-tool' file when python3 on the PATH is < 3.6
4 participants