test: build test add-ons like third-party add-ons by bnoordhuis · Pull Request #12231 · nodejs/node

bnoordhuis · 2017-04-05T12:46:00Z

Until now we built add-ons by pointing node-gyp at the src/ directory.
We've had at least one DOA release where add-ons were broken because of
a header dependency issue that wasn't caught because we build our test
add-ons in a non-standard way.

This commit does the following:

Use tools/install.py to install the headers to test/addons/include.
Add a script to build everything in test/addons.
Remove the pile-up of hacks from the Makefile.

The same logic is applied to test/addons-napi and test/gc.

Everything is done in parallel as much as possible to speed up builds.
Ideally, we derive the level of parallelism from MAKEFLAGS but it lacks
the actual -j<n> flag. That's why it simply spawns as many processes
as there are processors for now.

The exception is tools/doc/addon-verify.js: I switched it to synchronous
logic to make it easy to use from another script. Since it takes no time
at all to do its work, that seemed like a reasonable trade-off to me.

Refs: #11628
CI: https://ci.nodejs.org/job/node-test-pull-request/7225/

bnoordhuis · 2017-04-05T12:50:14Z

Makefile

This does the configure + build dance before running any of the JS tests. Probably should be moved but I kept it as close as possible to the existing order for now.

danbev · 2017-04-05T13:10:42Z

tools/build-addons.js

Could console.log call perhaps be removed in favour of just printing console.log(`building addon ${path}`); before 9864090#diff-31601f8dbf082ea61af036efe1e9c00dR54?

danbev · 2017-04-05T13:11:29Z

tools/build-addons.js

Perhaps --silent could be added to this to avoid extra output?

mhdawson

LGTM as long as ci is green.

bnoordhuis · 2017-04-05T18:39:17Z

Remember that time when my pull request worked on Windows on the first try? That's right, me neither...

LINK : fatal error LNK1181: cannot open input file 'c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015\label\win2008r2\test\addons\Release\node.lib' [c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015\label\win2008r2\test\gc\build\binding.vcxproj]

node-gyp looks for node.lib in a hard-coded (in lib/build.js) location relative to --nodedir=.... Needs more bovine coiffure.

^{(yak shaving)}

richardlau

Please could you also remove the relative include_dirs from test/addons/zlib-binding/binding.gyp?

richardlau

Also test/addons/include should be added to .gitignore?

refack · 2017-04-14T01:33:18Z

All the red lines (not red tests) make me so warm and fuzzy inside 😻

tools/install.py

refack · 2017-04-14T03:36:01Z

I'm not really sure where commit eebfadc ended up but it's a solution for passing on windows

refack · 2017-04-14T03:37:47Z

Let's see if I'm right: ~~https://ci.nodejs.org/job/node-test-pull-request/7392/~~ https://ci.nodejs.org/job/node-test-commit/9097/

refack · 2017-04-14T04:16:28Z

Well it's green where it counts https://ci.nodejs.org/job/node-test-binary-windows/7690/ 🤷

bnoordhuis · 2017-04-19T10:25:22Z

Rebased and included a simplified version of @refack's fix. Not sure yet how I feel about installing node.lib to $(PREFIX)/node/Release but I suppose it doesn't matter much because we don't otherwise use install.py on Windows.

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

Please could you also remove the relative include_dirs from test/addons/zlib-binding/binding.gyp?

Done.

Also test/addons/include should be added to .gitignore?

I'm not sure why but it doesn't show up with git status. It's not because of an exclusion in my global .gitignore, that I checked by moving it out of the way.

bnoordhuis · 2017-04-19T11:23:23Z

Well, that clearly didn't work. The ARM addons buildbot is passing an Intel-only flag to V8:

g++ '-DV8_TARGET_ARCH_ARM' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' '-DUSE_EABI_HARDFLOAT=1' -I../deps/v8  -pthread -Wall -Wextra -Wno-unused-parameter -fno-strict-aliasing -m32 -fdata-sections -ffunction-sections -O3 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/iojs/build/workspace/node-test-binary-arm/out/Release/.deps//home/iojs/build/workspace/node-test-binary-arm/out/Release/obj.host/v8_libbase/deps/v8/src/base/bits.o.d.raw   -c -o /home/iojs/build/workspace/node-test-binary-arm/out/Release/obj.host/v8_libbase/deps/v8/src/base/bits.o ../deps/v8/src/base/bits.cc
g++: error: unrecognized command line option ‘-m32’
deps/v8/src/v8_libbase.host.mk:112: recipe for target '/home/iojs/build/workspace/node-test-binary-arm/out/Release/obj.host/v8_libbase/deps/v8/src/base/bits.o' failed

Some weird interaction with the distributed build perhaps? One more try in case it was a fluke, I need to test the fixed Windows fix anyway: https://ci.nodejs.org/job/node-test-pull-request/7513/

refack · 2017-04-19T11:26:36Z

Not sure yet how I feel about installing node.lib to $(PREFIX)/node/Release

Not sure either, but it kinda makes sense, since otherwise the headers are only half useful (unless build target was a shared_lib).

Ref: a short discussion about this by the GYP authors indutny/gyp.js#37

refack · 2017-04-19T12:05:07Z

https://ci.nodejs.org/job/node-test-commit-windows-fanned/8427/

richardlau

LGTM if CI passes.

Until now we built add-ons by pointing node-gyp at the src/ directory. We've had at least one DOA release where add-ons were broken because of a header dependency issue that wasn't caught because we build our test add-ons in a non-standard way. This commit does the following: * Use tools/install.py to install the headers to test/addons/include. * Add a script to build everything in test/addons. * Remove the pile-up of hacks from the Makefile. The same logic is applied to test/addons-napi and test/gc. Everything is done in parallel as much as possible to speed up builds. Ideally, we derive the level of parallelism from MAKEFLAGS but it lacks the actual `-j<n>` flag. That's why it simply spawns as many processes as there are processors for now. The exception is tools/doc/addon-verify.js: I switched it to synchronous logic to make it easy to use from another script. Since it takes no time at all to do its work, that seemed like a reasonable trade-off to me. Refs: nodejs#11628

BridgeAR · 2017-08-26T11:26:12Z

@bnoordhuis this needs a rebase.

BridgeAR · 2017-09-12T19:43:18Z

@bnoordhuis I think this mainly just needs a rebase and is otherwise good to go? It is stalled since quite a while and I would go ahead and close this in a couple of days otherwise.

BridgeAR · 2017-09-20T00:32:26Z

Closing this due to a long inactivity and no response.

@bnoordhuis please reopen if you want to follow up on this.

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Apr 5, 2017

bnoordhuis mentioned this pull request Apr 5, 2017

build: make build-addons execute parallel #12230

Closed

2 tasks

bnoordhuis commented Apr 5, 2017

View reviewed changes

danbev approved these changes Apr 5, 2017

View reviewed changes

danbev mentioned this pull request Apr 5, 2017

build: create test_addons function #12204

Closed

2 tasks

mscdex added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. and removed build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform. labels Apr 5, 2017

mhdawson approved these changes Apr 5, 2017

View reviewed changes

fhinkel approved these changes Apr 5, 2017

View reviewed changes

richardlau requested changes Apr 5, 2017

View reviewed changes

jasnell approved these changes Apr 11, 2017

View reviewed changes

refack reviewed Apr 14, 2017

View reviewed changes

tools/install.py Outdated Show resolved Hide resolved

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

This was referenced Apr 17, 2017

GypPathToUniqueOutput from ninja.py indutny/gyp.js#37

Closed

tools: bump gyp to eb296f6 #12450

Closed

bnoordhuis force-pushed the revamp-addon-testing branch from eebfadc to 1f56dcf Compare April 19, 2017 10:17

bnoordhuis force-pushed the revamp-addon-testing branch from 1f56dcf to 6dbb055 Compare April 19, 2017 11:21

richardlau approved these changes Apr 19, 2017

View reviewed changes

refack mentioned this pull request Apr 19, 2017

build: fix ninja build failure #12484

Closed

2 tasks

bnoordhuis added 2 commits April 21, 2017 10:52

squash! partial revert for arm addons buildbot

4d56418

bnoordhuis force-pushed the revamp-addon-testing branch from ce81a1b to 4d56418 Compare April 21, 2017 09:24

refack mentioned this pull request May 15, 2017

icu-generic.gyp refactor - merge similar actions #12218

Closed

2 tasks

richardlau mentioned this pull request Aug 23, 2017

change test/addons/ to ignore empty folders #14843

Closed

BridgeAR added the stalled Issues and PRs that are stalled. label Sep 8, 2017

BridgeAR closed this Sep 20, 2017

richardlau mentioned this pull request Sep 22, 2017

src: do not include internals from node_buffer.h #15554

Closed

2 tasks

This was referenced Nov 26, 2017

build: refactor addon targets #17115

Closed

[request] cmake build job nodejs/build#1003

Closed

bnoordhuis mentioned this pull request Nov 30, 2017

build,test: make building addon tests less fragile #17407

Closed

richardlau mentioned this pull request Jun 6, 2018

build: build addon tests in parallel #21155

Closed

2 tasks

richardlau mentioned this pull request May 21, 2019

src: remove util-inl.h include in node.h #27804

Closed

2 tasks

Uh oh!

Conversation

bnoordhuis commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

danbev Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

danbev Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Apr 5, 2017

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 14, 2017

Uh oh!

Uh oh!

refack commented Apr 14, 2017

Uh oh!

refack commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Apr 14, 2017

Uh oh!

bnoordhuis commented Apr 19, 2017

Uh oh!

bnoordhuis commented Apr 19, 2017

Uh oh!

refack commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Apr 19, 2017

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 26, 2017

Uh oh!

BridgeAR commented Sep 12, 2017

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

bnoordhuis commented Apr 5, 2017 •

edited

Loading

refack commented Apr 14, 2017 •

edited

Loading

refack commented Apr 19, 2017 •

edited

Loading