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

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

Closed
wants to merge 2 commits into from
Closed

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

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

@bzoz
build, tools, win: add .S files support to GYP
160d2d2 
Makes GYP properly handle .S files.

Fixes: nodejs/node-v8#89
@nodejs-github-bot
Copy link
Collaborator

@bzoz build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1665/pipeline

@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
@bzoz bzoz mentioned this pull request Nov 21, 2018
Closed
@targos
Copy link
Member

targos commented Nov 21, 2018 •
edited
Loading

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 reviewed changes
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

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':
Copy link
Contributor

@refack refack Nov 21, 2018 •
edited
Loading

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.

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 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

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

@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
@bzoz
build, tools, win: add .S files support to GYP
9920dbc 
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
@bzoz @targos
build, tools, win: add .S files support to GYP
383d809 
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
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@bzoz @refack
build, tools, win: add .S files support to GYP
18cef55 
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
@bzoz @BethGriggs
build, tools, win: add .S files support to GYP
32d93cd 
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
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@bzoz @rvagg
build, tools, win: add .S files support to GYP
6fe278d 
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
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@refack refack refack approved these changes

@addaleax addaleax addaleax approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Broken build on Windows
7 participants
@bzoz @nodejs-github-bot @targos @refack @bnoordhuis @addaleax @richardlau