Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: build failure with ninja and Python 3 on Windows #113

Merged
merged 3 commits into from
Jul 7, 2021

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Jun 28, 2021

There are a few python code not updated to support python 3, the code path seems to only run when using ninja on Windows.

@gengjiawen gengjiawen requested review from targos and cclauss June 28, 2021 07:10
@@ -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)
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

@targos Since the code part is in block if flavor == "win", I guess it's okay ?

@zcbenz Out of curiosity, do you successfully build Node.js using Ninja on windows ?

Copy link
Contributor Author

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)

@gengjiawen gengjiawen requested a review from targos July 2, 2021 12:55
@targos targos removed their request for review July 2, 2021 14:05
@gengjiawen gengjiawen merged commit c172d10 into nodejs:main Jul 7, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants