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

build, tools, win: add .S files support to GYP #24553

Closed
wants to merge 2 commits into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Nov 21, 2018

Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Nov 21, 2018
@targos
Copy link
Member

targos commented Nov 21, 2018

Tentative CI on top of canary-base: https://ci.nodejs.org/job/node-test-commit-windows-fanned/22588/

Edit: rebase cannot work ofc. Let me apply it manually...

Edit: https://ci.nodejs.org/job/node-test-commit-windows-fanned/22589/ (on https://github.com/targos/node/commits/test-24553)

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % nit

@@ -2171,6 +2171,9 @@ def _MapFileToMsBuildSourceType(source, rule_dependencies,
elif ext == '.asm':
group = 'masm'
element = 'MASM'
elif ext == '.S':
Copy link
Contributor

@refack refack Nov 21, 2018

Choose a reason for hiding this comment

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

I'd suggest doing it like this

Suggested change
elif ext == '.S':
elif ext.lower() in ['.asm', '.s']:

Copy link
Member

Choose a reason for hiding this comment

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

Is that equivalent? On Unices the convention is that .S should be run through cpp whereas .s should not. I don't know if that's applicable to usually-case-insensitive Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT .s nor .S are standard for MSVS. It's more common to see .asm.
And in this case while the file indeed has .S semantics on Unices, cl.exe doesn't except it.

Seems like we have the same bug in ninja.py, so maybe the fix should be in v8.gyp, changing the extension to .asm iff Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

elif ext == 'c' or (ext == 'S' and self.flavor != 'win'):
command = 'cc'
elif ext == 's' and self.flavor != 'win': # Doesn't generate .o.d files.
command = 'cc_s'
elif (self.flavor == 'win' and ext == 'asm' and
not self.msvs_settings.HasExplicitAsmRules(spec)):
command = 'asm'
# Add the _asm suffix as msvs is capable of handling .cc and
# .asm files of the same name without collision.
obj_ext = '_asm.obj'

@refack refack added the gyp Issues and PRs related to the GYP tool and .gyp build files label Nov 21, 2018
@refack refack changed the base branch from master to canary-base November 21, 2018 13:37
@refack refack changed the base branch from canary-base to master November 21, 2018 13:38
@targos
Copy link
Member

targos commented Nov 21, 2018

The canary CI failed on 32bit but maybe it's unrelated?

https://ci.nodejs.org/job/node-compile-windows/22547/label=win-vs2017-x86/console

@refack
Copy link
Contributor

refack commented Nov 21, 2018

The canary CI failed on 32bit but maybe it's unrelated?

Error seems unrelated as it refers to an .h file. Also it's in v8_base_2 which is a dependency of v8_snapshot so it should be present in the job stated yesterday and refereed in nodejs/node-v8#89 - https://ci.nodejs.org/job/node-compile-windows/22413/label=win-vs2017-x86/console:

08:44:22 c:\workspace\node-compile-windows\deps\v8\src/ia32/assembler-ia32-inl.h(282): error C2440: '?': cannot convert from 'v8::internal::Code' to 'bool' (compiling source file ..\src\snapshot\startup-deserializer.cc) [c:\workspace\node-compile-windows\deps\v8\gypfiles\v8_base_2.vcxproj]
08:44:22   c:\workspace\node-compile-windows\deps\v8\src/ia32/assembler-ia32-inl.h(282): note: Ambiguous user-defined-conversion (compiling source file ..\src\snapshot\startup-deserializer.cc)
08:44:22 c:\workspace\node-compile-windows\deps\v8\src/ia32/assembler-ia32-inl.h(281): error C2660: 'v8::internal::Assembler::set_target_address_at': function does not take 2 arguments (compiling source file ..\src\snapshot\startup-deserializer.cc) [c:\workspace\node-compile-windows\deps\v8\gypfiles\v8_base_2.vcxproj]

@bzoz
Copy link
Contributor Author

bzoz commented Nov 22, 2018

Updated to support both lower and upper cases, PTAL.

@bzoz
Copy link
Contributor Author

bzoz commented Nov 29, 2018

@bzoz
Copy link
Contributor Author

bzoz commented Nov 29, 2018

Failures are unrelated.

@bzoz
Copy link
Contributor Author

bzoz commented Nov 29, 2018

Landed in 9920dbc

@bzoz bzoz closed this Nov 29, 2018
pull bot pushed a commit to shakir-abdo/node that referenced this pull request Nov 29, 2018
Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89

PR-URL: nodejs#24553
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Nov 29, 2018
Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89

PR-URL: #24553
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89

PR-URL: nodejs#24553
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89

PR-URL: #24553
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89

PR-URL: #24553
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. gyp Issues and PRs related to the GYP tool and .gyp build files tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken build on Windows
7 participants