Skip to content

Commit d427cac

Browse files
committed
test: build test add-ons like third-party add-ons
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
1 parent 30989d3 commit d427cac

File tree

9 files changed

+157
-224
lines changed

9 files changed

+157
-224
lines changed

Makefile

Lines changed: 20 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -66,18 +66,13 @@ endif
6666
# to check for changes.
6767
.PHONY: $(NODE_EXE) $(NODE_G_EXE)
6868

69-
# The -r/-L check stops it recreating the link if it is already in place,
70-
# otherwise $(NODE_EXE) being a .PHONY target means it is always re-run.
71-
# Without the check there is a race condition between the link being deleted
72-
# and recreated which can break the addons build when running test-ci
73-
# See comments on the build-addons target for some more info
7469
$(NODE_EXE): config.gypi out/Makefile
7570
$(MAKE) -C out BUILDTYPE=Release V=$(V)
76-
if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/Release/$(NODE_EXE) $@; fi
71+
ln -fs out/Release/$(NODE_EXE) $@
7772

7873
$(NODE_G_EXE): config.gypi out/Makefile
7974
$(MAKE) -C out BUILDTYPE=Debug V=$(V)
80-
if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/Debug/$(NODE_EXE) $@; fi
75+
ln -fs out/Debug/$(NODE_EXE) $@
8176

8277
out/Makefile: common.gypi deps/uv/uv.gyp deps/http_parser/http_parser.gyp \
8378
deps/zlib/zlib.gyp deps/v8/gypfiles/toolchain.gypi \
@@ -191,9 +186,7 @@ v8:
191186
tools/make-v8.sh
192187
$(MAKE) -C deps/v8 $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS)
193188

194-
test: all
195-
$(MAKE) build-addons
196-
$(MAKE) build-addons-napi
189+
test: build-addons
197190
$(MAKE) cctest
198191
$(PYTHON) tools/test.py --mode=release -J \
199192
doctool inspector known_issues message pseudo-tty parallel sequential $(CI_NATIVE_SUITES)
@@ -205,99 +198,6 @@ test-parallel: all
205198
test-valgrind: all
206199
$(PYTHON) tools/test.py --mode=release --valgrind sequential parallel message
207200

208-
# Implicitly depends on $(NODE_EXE). We don't depend on it explicitly because
209-
# it always triggers a rebuild due to it being a .PHONY rule. See the comment
210-
# near the build-addons rule for more background.
211-
test/gc/build/Release/binding.node: test/gc/binding.cc test/gc/binding.gyp
212-
$(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
213-
--python="$(PYTHON)" \
214-
--directory="$(shell pwd)/test/gc" \
215-
--nodedir="$(shell pwd)"
216-
217-
# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
218-
DOCBUILDSTAMP_PREREQS = tools/doc/addon-verify.js doc/api/addons.md
219-
220-
ifeq ($(OSTYPE),aix)
221-
DOCBUILDSTAMP_PREREQS := $(DOCBUILDSTAMP_PREREQS) out/$(BUILDTYPE)/node.exp
222-
endif
223-
224-
test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
225-
$(RM) -r test/addons/??_*/
226-
$(NODE) $<
227-
touch $@
228-
229-
ADDONS_BINDING_GYPS := \
230-
$(filter-out test/addons/??_*/binding.gyp, \
231-
$(wildcard test/addons/*/binding.gyp))
232-
233-
ADDONS_BINDING_SOURCES := \
234-
$(filter-out test/addons/??_*/*.cc, $(wildcard test/addons/*/*.cc)) \
235-
$(filter-out test/addons/??_*/*.h, $(wildcard test/addons/*/*.h))
236-
237-
# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
238-
# Depends on node-gyp package.json so that build-addons is (re)executed when
239-
# node-gyp is updated as part of an npm update.
240-
test/addons/.buildstamp: config.gypi \
241-
deps/npm/node_modules/node-gyp/package.json \
242-
$(ADDONS_BINDING_GYPS) $(ADDONS_BINDING_SOURCES) \
243-
deps/uv/include/*.h deps/v8/include/*.h \
244-
src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h \
245-
test/addons/.docbuildstamp
246-
# Cannot use $(wildcard test/addons/*/) here, it's evaluated before
247-
# embedded addons have been generated from the documentation.
248-
@for dirname in test/addons/*/; do \
249-
printf "\nBuilding addon $$PWD/$$dirname\n" ; \
250-
env MAKEFLAGS="-j1" $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
251-
--loglevel=$(LOGLEVEL) rebuild \
252-
--python="$(PYTHON)" \
253-
--directory="$$PWD/$$dirname" \
254-
--nodedir="$$PWD" || exit 1 ; \
255-
done
256-
touch $@
257-
258-
# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
259-
# directly because it calls make recursively. The parent make cannot know
260-
# if the subprocess touched anything so it pessimistically assumes that
261-
# .buildstamp and .docbuildstamp are out of date and need a rebuild.
262-
# Just goes to show that recursive make really is harmful...
263-
# TODO(bnoordhuis) Force rebuild after gyp update.
264-
build-addons: $(NODE_EXE) test/addons/.buildstamp
265-
266-
ADDONS_NAPI_BINDING_GYPS := \
267-
$(filter-out test/addons-napi/??_*/binding.gyp, \
268-
$(wildcard test/addons-napi/*/binding.gyp))
269-
270-
ADDONS_NAPI_BINDING_SOURCES := \
271-
$(filter-out test/addons-napi/??_*/*.cc, $(wildcard test/addons-napi/*/*.cc)) \
272-
$(filter-out test/addons-napi/??_*/*.h, $(wildcard test/addons-napi/*/*.h))
273-
274-
# Implicitly depends on $(NODE_EXE), see the build-addons-napi rule for rationale.
275-
test/addons-napi/.buildstamp: config.gypi \
276-
deps/npm/node_modules/node-gyp/package.json \
277-
$(ADDONS_NAPI_BINDING_GYPS) $(ADDONS_NAPI_BINDING_SOURCES) \
278-
deps/uv/include/*.h deps/v8/include/*.h \
279-
src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h \
280-
src/node_api.h src/node_api_types.h
281-
# Cannot use $(wildcard test/addons-napi/*/) here, it's evaluated before
282-
# embedded addons have been generated from the documentation.
283-
@for dirname in test/addons-napi/*/; do \
284-
printf "\nBuilding addon $$PWD/$$dirname\n" ; \
285-
env MAKEFLAGS="-j1" $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
286-
--loglevel=$(LOGLEVEL) rebuild \
287-
--python="$(PYTHON)" \
288-
--directory="$$PWD/$$dirname" \
289-
--nodedir="$$PWD" || exit 1 ; \
290-
done
291-
touch $@
292-
293-
# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
294-
# directly because it calls make recursively. The parent make cannot know
295-
# if the subprocess touched anything so it pessimistically assumes that
296-
# .buildstamp and .docbuildstamp are out of date and need a rebuild.
297-
# Just goes to show that recursive make really is harmful...
298-
# TODO(bnoordhuis) Force rebuild after gyp or node-gyp update.
299-
build-addons-napi: $(NODE_EXE) test/addons-napi/.buildstamp
300-
301201
clear-stalled:
302202
# Clean up any leftover processes but don't error if found.
303203
ps awwx | grep Release/node | grep -v grep | cat
@@ -306,28 +206,25 @@ clear-stalled:
306206
echo $${PS_OUT} | xargs kill; \
307207
fi
308208

309-
test-gc: all test/gc/build/Release/binding.node
209+
test-gc: build-addons
310210
$(PYTHON) tools/test.py --mode=release gc
311211

312-
test-gc-clean:
313-
$(RM) -r test/gc/build
314-
315-
test-build: | all build-addons build-addons-napi
212+
# Builds test/addons, test/addons-napi and test/gc.
213+
build-addons: $(NODE_EXE)
214+
./$< tools/build-addons.js
316215

317-
test-build-addons-napi: all build-addons-napi
318-
319-
test-all: test-build test/gc/build/Release/binding.node
216+
test-all: build-addons
320217
$(PYTHON) tools/test.py --mode=debug,release
321218

322-
test-all-valgrind: test-build
219+
test-all-valgrind: build-addons
323220
$(PYTHON) tools/test.py --mode=debug,release --valgrind
324221

325222
CI_NATIVE_SUITES := addons addons-napi
326223
CI_JS_SUITES := doctool inspector known_issues message parallel pseudo-tty sequential
327224

328225
# Build and test addons without building anything else
329226
test-ci-native: LOGLEVEL := info
330-
test-ci-native: | test/addons/.buildstamp test/addons-napi/.buildstamp
227+
test-ci-native: build-addons
331228
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
332229
--mode=release --flaky-tests=$(FLAKY_TESTS) \
333230
$(TEST_CI_ARGS) $(CI_NATIVE_SUITES)
@@ -345,7 +242,7 @@ test-ci-js: | clear-stalled
345242
fi
346243

347244
test-ci: LOGLEVEL := info
348-
test-ci: | clear-stalled build-addons build-addons-napi
245+
test-ci: | clear-stalled build-addons
349246
out/Release/cctest --gtest_output=tap:cctest.tap
350247
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
351248
--mode=release --flaky-tests=$(FLAKY_TESTS) \
@@ -357,13 +254,13 @@ test-ci: | clear-stalled build-addons build-addons-napi
357254
echo $${PS_OUT} | xargs kill; exit 1; \
358255
fi
359256

360-
test-release: test-build
257+
test-release: build-addons
361258
$(PYTHON) tools/test.py --mode=release
362259

363-
test-debug: test-build
260+
test-debug: build-addons
364261
$(PYTHON) tools/test.py --mode=debug
365262

366-
test-message: test-build
263+
test-message: build-addons
367264
$(PYTHON) tools/test.py message
368265

369266
test-simple: | cctest # Depends on 'all'.
@@ -397,16 +294,17 @@ test-npm: $(NODE_EXE)
397294
test-npm-publish: $(NODE_EXE)
398295
npm_package_config_publishtest=true $(NODE) deps/npm/test/run.js
399296

400-
test-addons-napi: test-build-addons-napi
401-
$(PYTHON) tools/test.py --mode=release addons-napi
402-
403-
test-addons: test-build test-addons-napi
297+
test-addons: build-addons
404298
$(PYTHON) tools/test.py --mode=release addons
405299

300+
test-addons-napi: build-addons
301+
$(PYTHON) tools/test.py --mode=release addons-napi
302+
406303
test-addons-clean:
407304
$(RM) -rf test/addons/??_*/
408305
$(RM) -rf test/addons/*/build
409-
$(RM) test/addons/.buildstamp test/addons/.docbuildstamp
306+
$(RM) -rf test/addons/Release/
307+
$(RM) -rf test/addons/include/
410308

411309
test-timers:
412310
$(MAKE) --directory=tools faketime
@@ -934,7 +832,6 @@ endif
934832
blog \
935833
blogclean \
936834
build-addons \
937-
build-addons-napi \
938835
build-ci \
939836
cctest \
940837
check \
@@ -974,7 +871,6 @@ endif
974871
test-ci-js \
975872
test-ci-native \
976873
test-gc \
977-
test-gc-clean \
978874
test-v8 \
979875
test-v8-all \
980876
test-v8-benchmarks \

test/addons-napi/.gitignore

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
.buildstamp
2-
.docbuildstamp
31
Makefile
42
*.Makefile
53
*.mk

test/addons/.gitignore

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
.buildstamp
2-
.docbuildstamp
31
Makefile
42
*.Makefile
53
*.mk
64
gyp-mac-tool
75
/*/build
6+
/Release/
7+
/include/

test/addons/openssl-binding/binding.gyp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
'conditions': [
66
['node_use_openssl=="true"', {
77
'sources': ['binding.cc'],
8-
'include_dirs': ['../../../deps/openssl/openssl/include'],
98
}]
109
]
1110
},

test/addons/zlib-binding/binding.gyp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
{
44
'target_name': 'binding',
55
'sources': ['binding.cc'],
6-
'include_dirs': ['../../../deps/zlib'],
76
},
87
]
98
}

tools/build-addons.js

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
'use strict';
2+
3+
const fs = require('fs');
4+
const os = require('os');
5+
const { spawn, spawnSync } = require('child_process');
6+
const { resolve } = require('path');
7+
8+
const kTopLevelDirectory = resolve(__dirname, '..');
9+
const kAddonsDirectory = resolve(kTopLevelDirectory, 'test/addons');
10+
const kNapiAddonsDirectory = resolve(kTopLevelDirectory, 'test/addons-napi');
11+
12+
// Location where the headers are installed to.
13+
const kIncludeDirectory = kAddonsDirectory;
14+
15+
const kPython = process.env.PYTHON || 'python';
16+
const kNodeGyp =
17+
resolve(kTopLevelDirectory, 'deps/npm/node_modules/node-gyp/bin/node-gyp');
18+
19+
process.chdir(kTopLevelDirectory);
20+
21+
// Copy headers to `${kIncludeDirectory}/include`. install.py preserves
22+
// timestamps so this won't cause unnecessary rebuilds.
23+
{
24+
const args = [ 'tools/install.py', 'install', kIncludeDirectory, '/' ];
25+
const env = Object.assign({}, process.env);
26+
env.HEADERS_ONLY = 'yes, please'; // Ask nicely.
27+
env.LOGLEVEL = 'WARNING';
28+
29+
const options = { env, stdio: 'inherit' };
30+
const result = spawnSync(kPython, args, options);
31+
if (result.status !== 0) process.exit(1);
32+
}
33+
34+
// Scrape embedded add-ons from doc/api/addons.md.
35+
require('./doc/addon-verify.js');
36+
37+
// Regenerate build files and rebuild if necessary.
38+
let failures = 0;
39+
process.on('exit', () => process.exit(failures > 0));
40+
41+
const jobs = [];
42+
43+
// test/gc contains a single add-on to build.
44+
{
45+
const path = resolve(kTopLevelDirectory, 'test/gc');
46+
exec(path, 'configure', () => exec(path, 'build'));
47+
}
48+
49+
for (const directory of [kAddonsDirectory, kNapiAddonsDirectory]) {
50+
for (const basedir of fs.readdirSync(directory)) {
51+
const path = resolve(directory, basedir);
52+
const gypfile = resolve(path, 'binding.gyp');
53+
if (!fs.existsSync(gypfile)) continue;
54+
exec(path, 'configure', () => exec(path, 'build'));
55+
}
56+
}
57+
58+
// FIXME(bnoordhuis) I would have liked to derive the desired level of
59+
// parallelism from MAKEFLAGS but it's missing the actual -j<jobs> flag.
60+
for (const _ of os.cpus()) next(); // eslint-disable-line no-unused-vars
61+
62+
function next() {
63+
const job = jobs.shift();
64+
if (job) job();
65+
}
66+
67+
function exec(path, command, done) {
68+
jobs.push(() => {
69+
if (failures > 0) return;
70+
71+
const args = [kNodeGyp,
72+
'--loglevel=silent',
73+
'--directory=' + path,
74+
'--nodedir=' + kIncludeDirectory,
75+
'--python=' + kPython,
76+
command];
77+
78+
const env = Object.assign({}, process.env);
79+
env.MAKEFLAGS = '-j1';
80+
81+
const options = { env, stdio: 'inherit' };
82+
const proc = spawn(process.execPath, args, options);
83+
84+
proc.on('exit', (exitCode) => {
85+
if (exitCode !== 0) ++failures;
86+
if (done) done();
87+
next();
88+
});
89+
90+
console.log(command, path);
91+
});
92+
}

0 commit comments

Comments
 (0)