-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix: build failure with ninja and Python 3 on Windows #113
Conversation
@@ -2389,7 +2389,6 @@ def GenerateOutputForConfig(target_list, target_dicts, data, params, config_name | |||
) | |||
if flavor == "win": | |||
master_ninja.variable("ld_host", ld_host) | |||
master_ninja.variable("ldxx_host", ldxx_host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ldxx_host
is also referenced in the other branch and it doesn't seem to be an issue (I always use ninja on unix systems)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python 3 fails with this error here:
Traceback (most recent call last):
File "node/tools/gyp/gyp_main.py", line 45, in <module>
sys.exit(gyp.script_main())
File "node/tools/gyp\pylib\gyp\__init__.py", line 662, in script_main
return main(sys.argv[1:])
File "node/tools/gyp\pylib\gyp\__init__.py", line 654, in main
return gyp_main(args)
File "node/tools/gyp\pylib\gyp\__init__.py", line 639, in gyp_main
generator.GenerateOutput(flat_list, targets, data, params)
File "node/tools/gyp\pylib\gyp\generator\ninja.py", line 2936, in GenerateOutput
target_list, target_dicts, data, params, config_name
File "node/tools/gyp\pylib\gyp\generator\ninja.py", line 2393, in GenerateOutputForConfig
master_ninja.variable("ldxx_host", ldxx_host)
UnboundLocalError: local variable 'ldxx_host' referenced before assignment
And that is because ldxx_host
has never been declared for Windows:
https://github.com/nodejs/gyp-next/blob/main/pylib/gyp/generator/ninja.py#L2235-L2250
Since ldxx_host
is never used on Windows, I think removing this line would be a reasonable fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, do you successfully build Node.js using Ninja on windows ?
Yes, but it requires some patches:
https://github.com/yue/node/commits/node-16.4.0
(It is used in a modified version of Node: https://github.com/yue/yode)
There are a few python code not updated to support python 3, the code path seems to only run when using ninja on Windows.