★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24553Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Reviewers

@bnoordhuis bnoordhuis

@refack refack

@addaleax addaleax

@richardlau richardlau

Assignees
No one assigned
Labels
build gyp tools
Projects
None yet
Milestone
No milestone
7 participants
@bzoz @nodejs-github-bot @targos @refack @bnoordhuis @addaleax @richardlau
@bzoz
Copy link
Contributor

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
@bzoz
build, tools, win: add .S files support to GYP 
Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89
Loading status checks…
160d2d2
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added build tools labels Nov 21, 2018

@bzoz bzoz referenced this pull request Nov 21, 2018

Closed

Broken build on Windows #89

@targos

This comment has been minimized.

Sign in to view
Copy link
Member

targos commented Nov 21, 2018 •
edited

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)
@refack

refack approved these changes Nov 21, 2018
View changes

Copy link
Member

refack left a comment

LGTM % nit
tools/gyp/pylib/gyp/generator/msvs.py Outdated
@@ -2171,6 +2171,9 @@ def _MapFileToMsBuildSourceType(source, rule_dependencies,
elif ext == '.asm':
group = 'masm'
element = 'MASM'
elif ext == '.S':

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Nov 21, 2018 •
edited

Member

I'd suggest doing it like this

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

This comment has been minimized.

Sign in to view
Copy link
@bnoordhuis

bnoordhuis Nov 21, 2018

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Nov 21, 2018

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Nov 21, 2018

Member

node/tools/gyp/pylib/gyp/generator/ninja.py

Lines 1038 to 1047 in 0c64816

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 label Nov 21, 2018

@richardlau

richardlau approved these changes Nov 21, 2018
View changes

@refack refack changed the base branch from master to canary-base Nov 21, 2018

@refack refack changed the base branch from canary-base to master Nov 21, 2018

@addaleax

addaleax approved these changes Nov 21, 2018
View changes

@targos

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bzoz commented Nov 22, 2018

Updated to support both lower and upper cases, PTAL.
@refack

refack approved these changes Nov 22, 2018
View changes

@bzoz

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bzoz commented Nov 29, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/19029/
@bzoz

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bzoz commented Nov 29, 2018

Failures are unrelated.
@bzoz

This comment has been minimized.

Sign in to view
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

@bzoz
build, tools, win: add .S files support to GYP 
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>
9920dbc

targos added a commit that referenced this pull request Nov 29, 2018

@bzoz @targos
build, tools, win: add .S files support to GYP 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
383d809

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

This was referenced Dec 7, 2018

Closed
Closed
Closed
Closed
Closed

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

@bzoz @refack
build, tools, win: add .S files support to GYP 
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>
18cef55

BethGriggs added a commit that referenced this pull request Feb 12, 2019

@bzoz @BethGriggs
build, tools, win: add .S files support to GYP 
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>
Verified
This commit was signed with a verified signature.
@BethGriggs BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
32d93cd

@BethGriggs BethGriggs referenced this pull request Feb 12, 2019

Merged

v10.15.3 proposal #26063

rvagg added a commit that referenced this pull request Feb 28, 2019

@bzoz @rvagg
build, tools, win: add .S files support to GYP 
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>
6fe278d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.