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.

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

build: build addon tests in parallel #21155

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 9 additions & 30 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -315,25 +315,14 @@ ADDONS_BINDING_SOURCES := \
# 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 \
deps/npm/node_modules/node-gyp/package.json tools/build-addons.js \
$(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.
# Ignore folders without binding.gyp
# (https://github.com/nodejs/node/issues/14843)
@for dirname in test/addons/*/; do \
if [ ! -f "$$PWD/$${dirname}binding.gyp" ]; then \
continue; fi ; \
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
env npm_config_loglevel=$(LOGLEVEL) npm_config_nodedir="$$PWD" \
npm_config_python="$(PYTHON)" $(NODE) "$$PWD/tools/build-addons" \
"$$PWD/deps/npm/node_modules/node-gyp/bin/node-gyp.js" "$$PWD/test/addons"
touch $@

.PHONY: build-addons
Expand All @@ -355,25 +344,15 @@ ADDONS_NAPI_BINDING_SOURCES := \

# 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 \
deps/npm/node_modules/node-gyp/package.json tools/build-addons.js \
$(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.
# Ignore folders without binding.gyp
# (https://github.com/nodejs/node/issues/14843)
@for dirname in test/addons-napi/*/; do \
if [ ! -f "$$PWD/$${dirname}binding.gyp" ]; then \
continue; fi ; \
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
env npm_config_loglevel=$(LOGLEVEL) npm_config_nodedir="$$PWD" \
npm_config_python="$(PYTHON)" $(NODE) "$$PWD/tools/build-addons" \
"$$PWD/deps/npm/node_modules/node-gyp/bin/node-gyp.js" \
"$$PWD/test/addons-napi"
touch $@

.PHONY: build-addons-napi
Expand Down
56 changes: 56 additions & 0 deletions tools/build-addons.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';

// Usage: e.g. node build-addons.js <path to node-gyp> <directory>

const child_process = require('child_process');
const path = require('path');
const fs = require('fs').promises;
const util = require('util');

const execFile = util.promisify(child_process.execFile);

const parallelization = +process.env.JOBS || require('os').cpus().length;
const nodeGyp = process.argv[2];

async function runner(directoryQueue) {
if (directoryQueue.length === 0)
return;

const dir = directoryQueue.shift();
const next = () => runner(directoryQueue);

try {
// Only run for directories that have a `binding.gyp`.
// (https://github.com/nodejs/node/issues/14843)
await fs.stat(path.join(dir, 'binding.gyp'));
} catch (err) {
if (err.code === 'ENOENT' || err.code === 'ENOTDIR')
return next();
throw err;
}

console.log(`Building addon in ${dir}`);
const { stdout, stderr } =
await execFile(process.execPath, [nodeGyp, 'rebuild', `--directory=${dir}`],
{
stdio: 'inherit',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this for execFile? We will write the output to this process later anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

@joyeecheung This is so we don’t get interleaved output from multiple builds running in parallel … will add a comment :)

env: { ...process.env, MAKEFLAGS: '-j1' }
});

process.stdout.write(stdout);
process.stderr.write(stderr);

return next();
}

async function main(directory) {
const directoryQueue = (await fs.readdir(directory))
.map((subdir) => path.join(directory, subdir));

const runners = [];
for (let i = 0; i < parallelization; ++i)
runners.push(runner(directoryQueue));
return Promise.all(runners);
}

main(process.argv[3]).catch((err) => setImmediate(() => { throw err; }));
Copy link
Member

Choose a reason for hiding this comment

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

Is the setImmediate() to get around a UnhandledPromiseRejectionWarning?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis Yup, it’s so the process really crashes in that case

Copy link
Member

Choose a reason for hiding this comment

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

Makes you wonder if we should export common.crashOnUnhandledRejection() as a public API..