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

deps: refactor v8.gyp #22017

Closed
wants to merge 2 commits into from
Closed

deps: refactor v8.gyp #22017

wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Jul 29, 2018

Mostly reorders lists of source files to match more BUILD.gn.
Fixes a few wrong entries.

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Jul 29, 2018
refack
refack approved these changes Jul 29, 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 50% rubber stamp
(I skimmed the changes, but did not do a thorough compare since if it passes CI it's most probably correct)

common.gypi
@@ -29,7 +29,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.12',
'v8_embedder_string': '-node.13',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question, is this required, just a formality, or does it help keep track of patches?

Copy link
Member Author

Choose a reason for hiding this comment

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

Required by the V8 update guide. I would be +1 on either removing the requirement for deps/v8/gypfiles or moving the directory to another place.

Copy link
Contributor

Choose a reason for hiding this comment

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

or moving the directory to another place.

I want to do that, I just need to find the time.

@targos
Copy link
Member Author

targos commented Aug 1, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/16113/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1559/

AIX rebuild: https://ci.nodejs.org/job/node-test-commit-aix/16634/

@targos
Copy link
Member Author

targos commented Aug 2, 2018

This apparently breaks the AIX build but I have no idea how.

/cc @nodejs/platform-aix

richardlau
richardlau reviewed Aug 2, 2018
View reviewed changes
deps/v8/gypfiles/v8.gyp Outdated
'sources': [
'../src/base/debug/stack_trace_posix.cc',
'../src/base/platform/platform-aix.cc',
'../src/base/platform/platform-posix.cc'
Copy link
Member

Choose a reason for hiding this comment

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

Missing trailing comma?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

NP. I didn't initially spot the missing comma either, but I started from the first link failure from https://ci.nodejs.org/job/node-test-commit-aix/16634/nodes=aix61-ppc64/console

10:02:02 ld: 0711-317 ERROR: Undefined symbol: .v8::base::OS::VSNPrintF(char*, int, char const*, char*)

found that v8::base::OS::VSNPrintF is defined in src/base/platform/platform-posix.cc and worked back from there. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice :). I wonder how gyp interpreted this. Was it even valid syntax?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, one of the problems of .gyp files is that this is valid python syntax (concat the two strings).
You can run the following monstrosity to get GYP to check the syntex:

python tools\gyp\gyp_main.py --check deps\v8\gypfiles\v8.gyp -I common.gypi -I config.gypi --depth=.

or for the whole project

python tools\gyp\gyp_main.py --check node.gyp -I common.gypi -I config.gypi --depth=.

targos added 2 commits August 2, 2018 23:09
@targos
deps: refactor v8.gyp
3f363ea 
Mostly reorders lists of source files to match more BUILD.gn.
Fixes a few wrong entries.
@targos
fixup aix
0f58c44
@targos
Copy link
Member Author

targos commented Aug 2, 2018

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

@targos
Copy link
Member Author

targos commented Aug 4, 2018 •
edited
Loading

Resumed build: https://ci.nodejs.org/job/node-test-pull-request/16186/
Edit: Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19665/

@targos
Copy link
Member Author

targos commented Aug 4, 2018

Landed in 8d10557

@targos targos closed this Aug 4, 2018
@targos targos deleted the refactor-v8-gyp branch August 4, 2018 16:09
targos added a commit that referenced this pull request Aug 4, 2018
@targos
deps: refactor v8.gyp
8d10557 
Mostly reorders lists of source files to match more BUILD.gn.
Fixes a few wrong entries.

PR-URL: #22017
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to psmarshall/node that referenced this pull request Aug 4, 2018
@targos
deps: refactor v8.gyp
16a807a 
Mostly reorders lists of source files to match more BUILD.gn.
Fixes a few wrong entries.

PR-URL: nodejs#22017
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Contributor

refack commented Aug 5, 2018

😄 I just run the command I quoted above and found:

Warning: Missing input files:
..\deps\v8\gypfiles\..\src\parsing\preparse-data-format.h

node/deps/v8/gypfiles/v8.gyp

Line 1265 in 0f58c44

'../src/parsing/preparse-data-format.h',

I'll make a mental note...

MylesBorins pushed a commit that referenced this pull request Aug 10, 2018
@targos @MylesBorins
deps: refactor v8.gyp
ce5cacd 
Mostly reorders lists of source files to match more BUILD.gn.
Fixes a few wrong entries.

Backport-PR-URL: #21668
PR-URL: #22017
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@refack refack mentioned this pull request Aug 17, 2018
Closed
2 tasks
@refack refack mentioned this pull request Aug 26, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@targos @nodejs-github-bot @refack @jasnell @richardlau