-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Python syntax error in dynamically generated 'build/gyp-mac-tool' file when python3 on the PATH is < 3.6 #2351
Comments
f-strings are a syntax error only on Python < 3.6. Given that we plan to drop those versions, I do not see the problem. After a closer look... This run was executed on a pre-release version of Python 3.10 -- We should not support pre-release software. |
It occurs with stable Python 3 as well. Such as Python 3.8.2 currently shipped with command line tools for Xcode. I admit it was careless and a bit misleading of me to include Python 3.10 in my verbose output, but in my testing the behavior is the same with stable Python versions. For example: It occurs with stable Python 3.8.2 (which I believe is currently shipped with command line tools for Xcode). With Python 3.8.2, I can use fstrings in general. For example, I can run this file, which I named package_type = 'a'
signature_code = 'b'
print(f"{package_type}{signature_code}") % /usr/bin/python3 test.py
ab
% /usr/bin/python3 --version
Python 3.8.2 |
node-gyp/gyp/pylib/gyp/mac_tool.py Line 1 in e81602e
#!/usr/bin/env python header so maybe it's not using the version of Python you're expecting it to.
|
I always use |
@richardlau that seems to have been the problem. The problem goes away if that shebang line is changed to macOS Catalina users still have Python 2.7.16 as The shebang lines are updated in |
Modifying all shebang lines to add the |
I humbly suggest updating to use I think UPDATE: I opened a pull request. See: #2355. |
It also seems prudent to use the same Python that It seems to me that the Makefile node-gyp/gyp/pylib/gyp/generator/make.py Line 527 in c3c510d
That's why the Python that is run is whatever is found by the shebang line in Thorough solutions (allowing use of newer Python language features and protecting users from this bug): Change the Makefile to work better for (We could change this in Workaround the problem by manipulating the environment: We could manipulate the environment such that the Python we want is the first on the PATH. (Maybe using a Python Simpler solutions: Don't fix this bug: Hope (and require) that macOS users' Older Python support in |
Even if I have a tentative fix for this corner case. (DeeDeeG@8250dec) It records the full path to the Python binary located by It's pretty specific to Lots of details & my thoughts on where to land this (click to expand):Implementation notes (click to expand):I changed So in the end we have: The reason I didn't just hard-code a Python path found by Caveat: This relies on reading the Some options:
Caveat 2: I'm not really sure about the |
I did a little research on how the They actually do their own Python detection, looking for If we could set up a similar Python symlink in the In general, I think we need to either:
And commit that to disk during Any advice on an implementation approach would be much appreciated. If I don't get any advice, I suppose I'll keep improvising and see what I can come up with. I think patching |
Awesome detective work @DeeDeeG Thanks much for digging into the details. It would be great if Node.js and gyp shared a common method for finding the right Python to use. Unless others have objections, I believe that creating a symlink at a well known path is the best approach. That would be easiest to reason about and easiest to test. |
I have two WIP implementations (both fully working, but code style could be cleaned up a bit), and a third idea that also probably would not be that hard to do.
Any comment or preference among these from Speaking from my personal opinion, I feel like this bug is very fixable in time for the upcoming v8.0.0 release of P.S. my thoughts: I'm leaning toward the symlinks, because we wouldn't have to patch |
Re: @cclauss
If you mean full code deduplication across these two projects, some of the duplicate code in either repository could be moved into If you meant we should use a similar algorithm or perform the same checks to find a usable Python binary/decide which Python is the right Python to use, even if technically the code is still duplicated, then I don't see why not. At least we should "compare notes". For example, their idea of checking
(EDIT: We could (If you just meant we should roughly copy their symlinking method, then see DeeDeeG@b2251a8 as mentioned in my previous comment.) |
Now that the shebangs in This issue now affects anyone with |
Verbose output (from node-gyp rebuild --verbose):
More info:
Only
node-gyp
from gitmaster
is affected; The latest release to the npm package registry7.1.2
is unaffected.The error looks like this:
When this error occurs, it's always the same syntax error at line 252 of
gyp-mac-tool
. Some (most?) packages appear to be unaffected, but I've found a few packages which are affected: git-utils, leveldown, and superstring.(Maybe this has to do with the
gyp-next
v0.7.0 or v0.8.0 upgrades?)The text was updated successfully, but these errors were encountered: