Skip to content

Commit

Permalink
test: build test add-ons like third-party add-ons
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bnoordhuis committed Apr 21, 2017
1 parent 30989d3 commit d427cac
Show file tree
Hide file tree
Showing 9 changed files with 157 additions and 224 deletions.
144 changes: 20 additions & 124 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,13 @@ endif
# to check for changes.
.PHONY: $(NODE_EXE) $(NODE_G_EXE)

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

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

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

test: all
$(MAKE) build-addons
$(MAKE) build-addons-napi
test: build-addons
$(MAKE) cctest
$(PYTHON) tools/test.py --mode=release -J \
doctool inspector known_issues message pseudo-tty parallel sequential $(CI_NATIVE_SUITES)
Expand All @@ -205,99 +198,6 @@ test-parallel: all
test-valgrind: all
$(PYTHON) tools/test.py --mode=release --valgrind sequential parallel message

# Implicitly depends on $(NODE_EXE). We don't depend on it explicitly because
# it always triggers a rebuild due to it being a .PHONY rule. See the comment
# near the build-addons rule for more background.
test/gc/build/Release/binding.node: test/gc/binding.cc test/gc/binding.gyp
$(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
--python="$(PYTHON)" \
--directory="$(shell pwd)/test/gc" \
--nodedir="$(shell pwd)"

# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
DOCBUILDSTAMP_PREREQS = tools/doc/addon-verify.js doc/api/addons.md

ifeq ($(OSTYPE),aix)
DOCBUILDSTAMP_PREREQS := $(DOCBUILDSTAMP_PREREQS) out/$(BUILDTYPE)/node.exp
endif

test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
$(RM) -r test/addons/??_*/
$(NODE) $<
touch $@

ADDONS_BINDING_GYPS := \
$(filter-out test/addons/??_*/binding.gyp, \
$(wildcard test/addons/*/binding.gyp))

ADDONS_BINDING_SOURCES := \
$(filter-out test/addons/??_*/*.cc, $(wildcard test/addons/*/*.cc)) \
$(filter-out test/addons/??_*/*.h, $(wildcard test/addons/*/*.h))

# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
# Depends on node-gyp package.json so that build-addons is (re)executed when
# node-gyp is updated as part of an npm update.
test/addons/.buildstamp: config.gypi \
deps/npm/node_modules/node-gyp/package.json \
$(ADDONS_BINDING_GYPS) $(ADDONS_BINDING_SOURCES) \
deps/uv/include/*.h deps/v8/include/*.h \
src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h \
test/addons/.docbuildstamp
# Cannot use $(wildcard test/addons/*/) here, it's evaluated before
# embedded addons have been generated from the documentation.
@for dirname in test/addons/*/; do \
printf "\nBuilding addon $$PWD/$$dirname\n" ; \
env MAKEFLAGS="-j1" $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
--loglevel=$(LOGLEVEL) rebuild \
--python="$(PYTHON)" \
--directory="$$PWD/$$dirname" \
--nodedir="$$PWD" || exit 1 ; \
done
touch $@

# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
# directly because it calls make recursively. The parent make cannot know
# if the subprocess touched anything so it pessimistically assumes that
# .buildstamp and .docbuildstamp are out of date and need a rebuild.
# Just goes to show that recursive make really is harmful...
# TODO(bnoordhuis) Force rebuild after gyp update.
build-addons: $(NODE_EXE) test/addons/.buildstamp

ADDONS_NAPI_BINDING_GYPS := \
$(filter-out test/addons-napi/??_*/binding.gyp, \
$(wildcard test/addons-napi/*/binding.gyp))

ADDONS_NAPI_BINDING_SOURCES := \
$(filter-out test/addons-napi/??_*/*.cc, $(wildcard test/addons-napi/*/*.cc)) \
$(filter-out test/addons-napi/??_*/*.h, $(wildcard test/addons-napi/*/*.h))

# Implicitly depends on $(NODE_EXE), see the build-addons-napi rule for rationale.
test/addons-napi/.buildstamp: config.gypi \
deps/npm/node_modules/node-gyp/package.json \
$(ADDONS_NAPI_BINDING_GYPS) $(ADDONS_NAPI_BINDING_SOURCES) \
deps/uv/include/*.h deps/v8/include/*.h \
src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h \
src/node_api.h src/node_api_types.h
# Cannot use $(wildcard test/addons-napi/*/) here, it's evaluated before
# embedded addons have been generated from the documentation.
@for dirname in test/addons-napi/*/; do \
printf "\nBuilding addon $$PWD/$$dirname\n" ; \
env MAKEFLAGS="-j1" $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
--loglevel=$(LOGLEVEL) rebuild \
--python="$(PYTHON)" \
--directory="$$PWD/$$dirname" \
--nodedir="$$PWD" || exit 1 ; \
done
touch $@

# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
# directly because it calls make recursively. The parent make cannot know
# if the subprocess touched anything so it pessimistically assumes that
# .buildstamp and .docbuildstamp are out of date and need a rebuild.
# Just goes to show that recursive make really is harmful...
# TODO(bnoordhuis) Force rebuild after gyp or node-gyp update.
build-addons-napi: $(NODE_EXE) test/addons-napi/.buildstamp

clear-stalled:
# Clean up any leftover processes but don't error if found.
ps awwx | grep Release/node | grep -v grep | cat
Expand All @@ -306,28 +206,25 @@ clear-stalled:
echo $${PS_OUT} | xargs kill; \
fi

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

test-gc-clean:
$(RM) -r test/gc/build

test-build: | all build-addons build-addons-napi
# Builds test/addons, test/addons-napi and test/gc.
build-addons: $(NODE_EXE)
./$< tools/build-addons.js

test-build-addons-napi: all build-addons-napi

test-all: test-build test/gc/build/Release/binding.node
test-all: build-addons
$(PYTHON) tools/test.py --mode=debug,release

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

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

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

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

test-release: test-build
test-release: build-addons
$(PYTHON) tools/test.py --mode=release

test-debug: test-build
test-debug: build-addons
$(PYTHON) tools/test.py --mode=debug

test-message: test-build
test-message: build-addons
$(PYTHON) tools/test.py message

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

test-addons-napi: test-build-addons-napi
$(PYTHON) tools/test.py --mode=release addons-napi

test-addons: test-build test-addons-napi
test-addons: build-addons
$(PYTHON) tools/test.py --mode=release addons

test-addons-napi: build-addons
$(PYTHON) tools/test.py --mode=release addons-napi

test-addons-clean:
$(RM) -rf test/addons/??_*/
$(RM) -rf test/addons/*/build
$(RM) test/addons/.buildstamp test/addons/.docbuildstamp
$(RM) -rf test/addons/Release/
$(RM) -rf test/addons/include/

test-timers:
$(MAKE) --directory=tools faketime
Expand Down Expand Up @@ -934,7 +832,6 @@ endif
blog \
blogclean \
build-addons \
build-addons-napi \
build-ci \
cctest \
check \
Expand Down Expand Up @@ -974,7 +871,6 @@ endif
test-ci-js \
test-ci-native \
test-gc \
test-gc-clean \
test-v8 \
test-v8-all \
test-v8-benchmarks \
Expand Down
2 changes: 0 additions & 2 deletions test/addons-napi/.gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
.buildstamp
.docbuildstamp
Makefile
*.Makefile
*.mk
Expand Down
4 changes: 2 additions & 2 deletions test/addons/.gitignore
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.buildstamp
.docbuildstamp
Makefile
*.Makefile
*.mk
gyp-mac-tool
/*/build
/Release/
/include/
1 change: 0 additions & 1 deletion test/addons/openssl-binding/binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
'conditions': [
['node_use_openssl=="true"', {
'sources': ['binding.cc'],
'include_dirs': ['../../../deps/openssl/openssl/include'],
}]
]
},
Expand Down
1 change: 0 additions & 1 deletion test/addons/zlib-binding/binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
{
'target_name': 'binding',
'sources': ['binding.cc'],
'include_dirs': ['../../../deps/zlib'],
},
]
}
92 changes: 92 additions & 0 deletions tools/build-addons.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
'use strict';

const fs = require('fs');
const os = require('os');
const { spawn, spawnSync } = require('child_process');
const { resolve } = require('path');

const kTopLevelDirectory = resolve(__dirname, '..');
const kAddonsDirectory = resolve(kTopLevelDirectory, 'test/addons');
const kNapiAddonsDirectory = resolve(kTopLevelDirectory, 'test/addons-napi');

// Location where the headers are installed to.
const kIncludeDirectory = kAddonsDirectory;

const kPython = process.env.PYTHON || 'python';
const kNodeGyp =
resolve(kTopLevelDirectory, 'deps/npm/node_modules/node-gyp/bin/node-gyp');

process.chdir(kTopLevelDirectory);

// Copy headers to `${kIncludeDirectory}/include`. install.py preserves
// timestamps so this won't cause unnecessary rebuilds.
{
const args = [ 'tools/install.py', 'install', kIncludeDirectory, '/' ];
const env = Object.assign({}, process.env);
env.HEADERS_ONLY = 'yes, please'; // Ask nicely.
env.LOGLEVEL = 'WARNING';

const options = { env, stdio: 'inherit' };
const result = spawnSync(kPython, args, options);
if (result.status !== 0) process.exit(1);
}

// Scrape embedded add-ons from doc/api/addons.md.
require('./doc/addon-verify.js');

// Regenerate build files and rebuild if necessary.
let failures = 0;
process.on('exit', () => process.exit(failures > 0));

const jobs = [];

// test/gc contains a single add-on to build.
{
const path = resolve(kTopLevelDirectory, 'test/gc');
exec(path, 'configure', () => exec(path, 'build'));
}

for (const directory of [kAddonsDirectory, kNapiAddonsDirectory]) {
for (const basedir of fs.readdirSync(directory)) {
const path = resolve(directory, basedir);
const gypfile = resolve(path, 'binding.gyp');
if (!fs.existsSync(gypfile)) continue;
exec(path, 'configure', () => exec(path, 'build'));
}
}

// FIXME(bnoordhuis) I would have liked to derive the desired level of
// parallelism from MAKEFLAGS but it's missing the actual -j<jobs> flag.
for (const _ of os.cpus()) next(); // eslint-disable-line no-unused-vars

function next() {
const job = jobs.shift();
if (job) job();
}

function exec(path, command, done) {
jobs.push(() => {
if (failures > 0) return;

const args = [kNodeGyp,
'--loglevel=silent',
'--directory=' + path,
'--nodedir=' + kIncludeDirectory,
'--python=' + kPython,
command];

const env = Object.assign({}, process.env);
env.MAKEFLAGS = '-j1';

const options = { env, stdio: 'inherit' };
const proc = spawn(process.execPath, args, options);

proc.on('exit', (exitCode) => {
if (exitCode !== 0) ++failures;
if (done) done();
next();
});

console.log(command, path);
});
}
Loading

0 comments on commit d427cac

Please sign in to comment.