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.

Sign up for GitHub

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

Jump to bottom

tools: refactor build-addons.js to ESM #43099

Merged
merged 1 commit into from
May 22, 2022
Merged

tools: refactor build-addons.js to ESM #43099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion .github/label-pr-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ subSystemLabels:
/^tools\/make-v8/: tools, v8 engine, needs-ci
/^tools\/v8_gypfiles/: tools, v8 engine, needs-ci
/^tools\/(code_cache|snapshot)/: needs-ci
/^tools\/build-addons.js/: needs-ci
/^tools\/build-addons.mjs/: needs-ci
# all other tool changes should be marked as such
/^tools\//: tools
/^\.eslint|\.remark|\.editorconfig/: tools
Expand Down
4 changes: 2 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,13 +366,13 @@ ADDONS_BINDING_SOURCES := \
$(filter-out test/addons/??_*/*.h, $(wildcard test/addons/*/*.h))

ADDONS_PREREQS := config.gypi \
deps/npm/node_modules/node-gyp/package.json tools/build-addons.js \
deps/npm/node_modules/node-gyp/package.json tools/build-addons.mjs \
deps/uv/include/*.h deps/v8/include/*.h \
src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h

define run_build_addons
env npm_config_loglevel=$(LOGLEVEL) npm_config_nodedir="$$PWD" \
npm_config_python="$(PYTHON)" $(NODE) "$$PWD/tools/build-addons" \
npm_config_python="$(PYTHON)" $(NODE) "$$PWD/tools/build-addons.mjs" \
"$$PWD/deps/npm/node_modules/node-gyp/bin/node-gyp.js" \
$1
touch $2
Expand Down
2 changes: 1 addition & 1 deletion doc/contributing/collaborator-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ There are some other files that touch the build chain. Changes in the following
files also qualify as affecting the `node` binary:

* `tools/*.py`
* `tools/build-addons.js`
* `tools/build-addons.mjs`
* `*.gyp`
* `*.gypi`
* `configure`
Expand Down
58 changes: 0 additions & 58 deletions tools/build-addons.js
View file
Open in desktop

This file was deleted.

63 changes: 63 additions & 0 deletions tools/build-addons.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#!/usr/bin/env node

// Usage: e.g. node build-addons.mjs <path to node-gyp> <directory>
F3n67u marked this conversation as resolved.
Show resolved Hide resolved

import child_process from 'node:child_process';
import path from 'node:path';
import fs from 'node:fs/promises';
import util from 'node:util';
import process from 'node:process';
import os from 'node:os';

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

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

async function buildAddon(dir) {
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;
throw err;
}

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

// We buffer the output and print it out once the process is done in order
// to avoid interleaved output from multiple builds running at once.
process.stdout.write(stdout);
process.stderr.write(stderr);
}

async function parallel(jobQueue, limit) {
const next = async () => {
if (jobQueue.length === 0) {
return;
}
const job = jobQueue.shift();
await job();
await next();
};

const workerCnt = Math.min(limit, jobQueue.length);
await Promise.all(Array.from({ length: workerCnt }, next));
}

const jobs = [];
for await (const dirent of await fs.opendir(directory)) {
if (dirent.isDirectory()) {
jobs.push(() => buildAddon(path.join(directory, dirent.name)));
}
}
await parallel(jobs, parallelization);
Comment on lines +43 to +63
Copy link
Contributor

@aduh95 aduh95 May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

non-blocking suggestion: we don't need to wait for the for await loop to finish to start the building addons. But also it does add a bit of complexity to the code, so feel free to ignore (or to come up with a simpler code 😅).

Suggested change
async function parallel(jobQueue, limit) {
const next = async () => {
if (jobQueue.length === 0) {
return;
}
const job = jobQueue.shift();
await job();
await next();
};
const workerCnt = Math.min(limit, jobQueue.length);
await Promise.all(Array.from({ length: workerCnt }, next));
}
const jobs = [];
for await (const dirent of await fs.opendir(directory)) {
if (dirent.isDirectory()) {
jobs.push(() => buildAddon(path.join(directory, dirent.name)));
}
}
await parallel(jobs, parallelization);
const jobQueue = [];
const workers = Array(parallelization);
let aliveWorkers = 0;
let finishedWorkers = 0;
async function runJob(job) {
await job();
if (jobQueue.length === 0) {
finishedWorkers++;
} else {
await runJob(jobQueue.shift());
}
}
for await (const dirent of await fs.opendir(directory)) {
if (dirent.isDirectory()) {
const job = () => buildAddon(path.join(directory, dirent.name));
if (aliveWorkers < parallelization) {
workers[aliveWorkers++] = runJob(job);
} else {
jobQueue.push(job);
}
}
}
if (finishedWorkers !== 0) {
const remainingAvailableCores =
parallelization - aliveWorkers + finishedWorkers;
if (jobQueue.length < remainingAvailableCores) {
workers.push(...jobQueue.splice(0).map(runJob));
} else {
workers.push(
...Array.from({ length: remainingAvailableCores }, () =>
runJob(jobQueue.shift())
)
);
}
}
await Promise.all(workers);

Copy link
Member Author

@F3n67u F3n67u May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have thought about this previously:
is it a solution that read a directory entry lazily when I have an idle core? so that I don't have to read all entries at once. but I have not come out with an elegant solution.

IMHO, I would prefer my solution because it's simpler and more readable. build-addon.mjs is not that performance sensitive, so I will trade a little performance for simplicity and readability.

anyway, thanks for your advice😀.

Copy link
Contributor

Choose a reason for hiding this comment

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

build-addon.mjs is not that performance sensitive, so I will trade a little performance for simplicity and readability.

tbf I don't think my proposed solution would be more performant (or marginally so maybe), but it could be less memory intensive (because it lets the engine garbage collect more eagerly). Of course, that would only matter in the case the directory we're iterating over is very large, and it doesn't seem to be case so far, so let's keep it as is.

6 changes: 3 additions & 3 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ if %errorlevel% neq 0 exit /b %errorlevel%
:: building addons
setlocal
set npm_config_nodedir=%~dp0
"%node_exe%" "%~dp0tools\build-addons.js" "%~dp0deps\npm\node_modules\node-gyp\bin\node-gyp.js" "%~dp0test\addons"
"%node_exe%" "%~dp0tools\build-addons.mjs" "%~dp0deps\npm\node_modules\node-gyp\bin\node-gyp.js" "%~dp0test\addons"
if errorlevel 1 exit /b 1
endlocal

Expand All @@ -626,7 +626,7 @@ for /d %%F in (test\js-native-api\??_*) do (
:: building js-native-api
setlocal
set npm_config_nodedir=%~dp0
"%node_exe%" "%~dp0tools\build-addons.js" "%~dp0deps\npm\node_modules\node-gyp\bin\node-gyp.js" "%~dp0test\js-native-api"
"%node_exe%" "%~dp0tools\build-addons.mjs" "%~dp0deps\npm\node_modules\node-gyp\bin\node-gyp.js" "%~dp0test\js-native-api"
if errorlevel 1 exit /b 1
endlocal
goto build-node-api-tests
Expand All @@ -645,7 +645,7 @@ for /d %%F in (test\node-api\??_*) do (
:: building node-api
setlocal
set npm_config_nodedir=%~dp0
"%node_exe%" "%~dp0tools\build-addons.js" "%~dp0deps\npm\node_modules\node-gyp\bin\node-gyp.js" "%~dp0test\node-api"
"%node_exe%" "%~dp0tools\build-addons.mjs" "%~dp0deps\npm\node_modules\node-gyp\bin\node-gyp.js" "%~dp0test\node-api"
if errorlevel 1 exit /b 1
endlocal
goto run-tests
Expand Down