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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jul 7, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
457a870
fix: translate() takes exactly 1 argument in python 3
zcbenz Jun 27, 2021
5067819
fix: float/int can not be compared to str in python 3
zcbenz Jun 27, 2021
6e024f2
fix: local variable 'ldxx_host' referenced before assignment
zcbenz Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion pylib/gyp/generator/ninja.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

else:
master_ninja.variable(
"ld_host", CommandWithWrapper("LINK", wrappers, ld_host)
Expand Down
4 changes: 2 additions & 2 deletions pylib/gyp/msvs_emulation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ def _TargetConfig(self, config):
# first level is globally for the configuration (this is what we consider
# "the" config at the gyp level, which will be something like 'Debug' or
# 'Release'), VS2015 and later only use this level
if self.vs_version.short_name >= 2015:
if int(self.vs_version.short_name) >= 2015:
return config
# and a second target-specific configuration, which is an
# override for the global one. |config| is remapped here to take into
Expand Down Expand Up @@ -537,7 +537,7 @@ def GetCflags(self, config):
)
]
)
if self.vs_version.project_version >= 12.0:
if float(self.vs_version.project_version) >= 12.0:
# New flag introduced in VS2013 (project version 12.0) Forces writes to
# the program database (PDB) to be serialized through MSPDBSRV.EXE.
# https://msdn.microsoft.com/en-us/library/dn502518.aspx
Expand Down
5 changes: 3 additions & 2 deletions pylib/gyp/win_tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,9 @@ def ExecLinkWithManifests(
# and sometimes doesn't unfortunately.
with open(our_manifest) as our_f:
with open(assert_manifest) as assert_f:
our_data = our_f.read().translate(None, string.whitespace)
assert_data = assert_f.read().translate(None, string.whitespace)
translator = str.maketrans('', '', string.whitespace)
our_data = our_f.read().translate(translator)
assert_data = assert_f.read().translate(translator)
if our_data != assert_data:
os.unlink(out)

Expand Down