Conversation
nodejs-github-bot
added
build
refack
left a comment
refack
left a comment
There was a problem hiding this comment.
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
Outdated
There was a problem hiding this comment.
Just a question, is this required, just a formality, or does it help keep track of patches?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
or moving the directory to another place.
I want to do that, I just need to find the time.
|
This apparently breaks the AIX build but I have no idea how. /cc @nodejs/platform-aix |
deps/v8/gypfiles/v8.gyp
Outdated
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
Nice :). I wonder how gyp interpreted this. Was it even valid syntax?
There was a problem hiding this comment.
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=.
Mostly reorders lists of source files to match more BUILD.gn. Fixes a few wrong entries.
|
Resumed build: https://ci.nodejs.org/job/node-test-pull-request/16186/ |
|
Landed in 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>
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>
|
😄 I just run the command I quoted above and found: Line 1265 in 0f58c44 I'll make a mental note... |
Mostly reorders lists of source files to match more BUILD.gn. Fixes a few wrong entries. Backport-PR-URL: nodejs/node#21668 PR-URL: nodejs/node#22017 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
5 participants
Mostly reorders lists of source files to match more BUILD.gn.
Fixes a few wrong entries.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes