From a779b0d26b42fde60137718b0448d877aa3f200a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 6 Sep 2023 01:40:33 +0200 Subject: [PATCH 1/4] bootstrap: do not expand argv1 for snapshots To avoid capturing build machine states into the snapshot --- lib/internal/main/mksnapshot.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js index 52d859d491a93f..34701716839326 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -132,7 +132,7 @@ function requireForUserSnapshot(id) { function main() { - prepareMainThreadExecution(true, false); + prepareMainThreadExecution(false, false); initializeCallbacks(); let stackTraceLimitDesc; From 2b541e3949e57fa2819f83e1aad52053e5da4e52 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 6 Sep 2023 01:40:59 +0200 Subject: [PATCH 2/4] test: fix argument computation in embedtest There were a few bugs in the original test that went unnoticed because with the bug the test did not actually get run anymore. This patch fixes the argument computation by accounting filtering of the arguments, and uses spawnSyncAndExit{WithoutError} in the test to enable better logging when the test fails. --- test/embedding/embedtest.cc | 27 +++--- test/embedding/test-embedding.js | 150 ++++++++++++++++++++----------- 2 files changed, 113 insertions(+), 64 deletions(-) diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc index a03dfbed93939c..260c4f06cacb18 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -89,7 +89,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform, snapshot_as_file = true; } else if (arg == "--embedder-snapshot-blob") { assert(i + 1 < args.size()); - snapshot_blob_path = args[i + i]; + snapshot_blob_path = args[i + 1]; i++; } else { filtered_args.push_back(arg); @@ -123,7 +123,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform, // It contains at least the binary path, the code to snapshot, // and --embedder-snapshot-create. Insert an anonymous filename // as process.argv[1]. - assert(filtered_args.size() >= 3); + assert(filtered_args.size() >= 2); filtered_args.insert(filtered_args.begin() + 1, node::GetAnonymousMainPath()); } @@ -153,19 +153,26 @@ int RunNodeInstance(MultiIsolatePlatform* platform, Context::Scope context_scope(setup->context()); MaybeLocal loadenv_ret; - if (snapshot) { + if (snapshot) { // Deserializing snapshot loadenv_ret = node::LoadEnvironment(env, node::StartExecutionCallback{}); - } else { + } else if (is_building_snapshot) { + // Environment created for snapshotting must set process.argv[1] to + // the name of the main script, which was inserted above. loadenv_ret = node::LoadEnvironment( env, - // Snapshots do not support userland require()s (yet) - "if (!require('v8').startupSnapshot.isBuildingSnapshot()) {" - " const publicRequire =" - " require('module').createRequire(process.cwd() + '/');" - " globalThis.require = publicRequire;" - "} else globalThis.require = require;" + "const assert = require('assert');" + "assert(require('v8').startupSnapshot.isBuildingSnapshot());" "globalThis.embedVars = { nön_ascıı: '🏳️‍🌈' };" + "globalThis.require = require;" "require('vm').runInThisContext(process.argv[2]);"); + } else { + loadenv_ret = node::LoadEnvironment( + env, + "const publicRequire = require('module').createRequire(process.cwd() " + "+ '/');" + "globalThis.require = publicRequire;" + "globalThis.embedVars = { nön_ascıı: '🏳️‍🌈' };" + "require('vm').runInThisContext(process.argv[1]);"); } if (loadenv_ret.IsEmpty()) // There has been a JS exception. diff --git a/test/embedding/test-embedding.js b/test/embedding/test-embedding.js index 5d448b78a433e8..1fb3bc73f494cb 100644 --- a/test/embedding/test-embedding.js +++ b/test/embedding/test-embedding.js @@ -3,7 +3,10 @@ const common = require('../common'); const fixtures = require('../common/fixtures'); const tmpdir = require('../common/tmpdir'); const assert = require('assert'); -const child_process = require('child_process'); +const { + spawnSyncAndExit, + spawnSyncAndExitWithoutError, +} = require('../common/child_process'); const path = require('path'); const fs = require('fs'); @@ -21,39 +24,54 @@ function resolveBuiltBinary(bin) { const binary = resolveBuiltBinary('embedtest'); -assert.strictEqual( - child_process.spawnSync(binary, ['console.log(42)']) - .stdout.toString().trim(), - '42'); - -assert.strictEqual( - child_process.spawnSync(binary, ['console.log(embedVars.nön_ascıı)']) - .stdout.toString().trim(), - '🏳️‍🌈'); - -assert.strictEqual( - child_process.spawnSync(binary, ['console.log(42)']) - .stdout.toString().trim(), - '42'); +spawnSyncAndExitWithoutError( + binary, + ['console.log(42)'], + { + trim: true, + stdout: '42', + }); -assert.strictEqual( - child_process.spawnSync(binary, ['throw new Error()']).status, - 1); +spawnSyncAndExitWithoutError( + binary, + ['console.log(embedVars.nön_ascıı)'], + { + trim: true, + stdout: '🏳️‍🌈', + }); -// Cannot require internals anymore: -assert.strictEqual( - child_process.spawnSync(binary, ['require("lib/internal/test/binding")']).status, - 1); +spawnSyncAndExit( + binary, + ['throw new Error()'], + { + status: 1, + signal: null, + }); -assert.strictEqual( - child_process.spawnSync(binary, ['process.exitCode = 8']).status, - 8); +spawnSyncAndExit( + binary, + ['require("lib/internal/test/binding")'], + { + status: 1, + signal: null, + }); +spawnSyncAndExit( + binary, + ['process.exitCode = 8'], + { + status: 8, + signal: null, + }); const fixturePath = JSON.stringify(fixtures.path('exit.js')); -assert.strictEqual( - child_process.spawnSync(binary, [`require(${fixturePath})`, 92]).status, - 92); +spawnSyncAndExit( + binary, + [`require(${fixturePath})`, 92], + { + status: 92, + signal: null, + }); function getReadFileCodeForPath(path) { return `(require("fs").readFileSync(${JSON.stringify(path)}, "utf8"))`; @@ -64,31 +82,49 @@ for (const extraSnapshotArgs of [[], ['--embedder-snapshot-as-file']]) { // readSync + eval since snapshots don't support userland require() (yet) const snapshotFixture = fixtures.path('snapshot', 'echo-args.js'); const blobPath = tmpdir.resolve('embedder-snapshot.blob'); - const buildSnapshotArgs = [ + const buildSnapshotExecArgs = [ `eval(${getReadFileCodeForPath(snapshotFixture)})`, 'arg1', 'arg2', + ]; + const embedTestBuildArgs = [ '--embedder-snapshot-blob', blobPath, '--embedder-snapshot-create', ...extraSnapshotArgs, ]; - const runEmbeddedArgs = [ - '--embedder-snapshot-blob', blobPath, ...extraSnapshotArgs, 'arg3', 'arg4', + const buildSnapshotArgs = [ + ...buildSnapshotExecArgs, + ...embedTestBuildArgs, + ]; + + const runSnapshotExecArgs = [ + 'arg3', 'arg4', + ]; + const embedTestRunArgs = [ + '--embedder-snapshot-blob', blobPath, + ...extraSnapshotArgs, + ]; + const runSnapshotArgs = [ + ...runSnapshotExecArgs, + ...embedTestRunArgs, ]; fs.rmSync(blobPath, { force: true }); - const child = child_process.spawnSync(binary, [ - '--', ...buildSnapshotArgs, - ], { - cwd: tmpdir.path, - }); - if (child.status !== 0) { - console.log(child.stderr.toString()); - console.log(child.stdout.toString()); - } - assert.strictEqual(child.status, 0); - const spawnResult = child_process.spawnSync(binary, ['--', ...runEmbeddedArgs]); - assert.deepStrictEqual(JSON.parse(spawnResult.stdout), { - originalArgv: [binary, ...buildSnapshotArgs], - currentArgv: [binary, ...runEmbeddedArgs], - }); + spawnSyncAndExitWithoutError( + binary, + [ '--', ...buildSnapshotArgs ], + { cwd: tmpdir.path }, + {}); + spawnSyncAndExitWithoutError( + binary, + [ '--', ...runSnapshotArgs ], + { cwd: tmpdir.path }, + { + stdout(output) { + assert.deepStrictEqual(JSON.parse(output), { + originalArgv: [binary, '__node_anonymous_main', ...buildSnapshotExecArgs], + currentArgv: [binary, ...runSnapshotExecArgs], + }); + return true; + }, + }); } // Create workers and vm contexts after deserialization @@ -99,14 +135,20 @@ for (const extraSnapshotArgs of [[], ['--embedder-snapshot-as-file']]) { `eval(${getReadFileCodeForPath(snapshotFixture)})`, '--embedder-snapshot-blob', blobPath, '--embedder-snapshot-create', ]; + const runEmbeddedArgs = [ + '--embedder-snapshot-blob', blobPath, + ]; fs.rmSync(blobPath, { force: true }); - assert.strictEqual(child_process.spawnSync(binary, [ - '--', ...buildSnapshotArgs, - ], { - cwd: tmpdir.path, - }).status, 0); - assert.strictEqual( - child_process.spawnSync(binary, ['--', '--embedder-snapshot-blob', blobPath]).status, - 0); + + spawnSyncAndExitWithoutError( + binary, + [ '--', ...buildSnapshotArgs ], + { cwd: tmpdir.path }, + {}); + spawnSyncAndExitWithoutError( + binary, + [ '--', ...runEmbeddedArgs ], + { cwd: tmpdir.path }, + {}); } From 8b9b658d678e1009b24a1eefb52fce6203477daa Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 6 Sep 2023 01:43:45 +0200 Subject: [PATCH 3/4] build: run embedtest using node executable We should use the node executable to run this test, instead of counting on embedtest, the binary being tested, as the test runner. Otherwise bugs can go unnoticed if the embedtest binary itself does not work correctly. --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index b1c267ed5526fe..90bf7aacfdef4f 100644 --- a/Makefile +++ b/Makefile @@ -286,7 +286,7 @@ coverage-report-js: # Runs the C++ tests using the built `cctest` executable. cctest: all @out/$(BUILDTYPE)/$@ --gtest_filter=$(GTEST_FILTER) - @out/$(BUILDTYPE)/embedtest "require('./test/embedding/test-embedding.js')" + $(NODE) ./test/embedding/test-embedding.js .PHONY: list-gtests list-gtests: @@ -550,7 +550,7 @@ test-ci: | clear-stalled bench-addons-build build-addons build-js-native-api-tes $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ --mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \ $(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) $(CI_DOC) - out/Release/embedtest 'require("./test/embedding/test-embedding.js")' + $(NODE) ./test/embedding/test-embedding.js $(info Clean up any leftover processes, error if found.) ps awwx | grep Release/node | grep -v grep | cat @PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \ From fbf82ab4afa96458938b8d6dcb796a33accdf5eb Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 13 Sep 2023 16:06:36 +0200 Subject: [PATCH 4/4] fixup! test: fix argument computation in embedtest --- test/embedding/embedtest.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc index 260c4f06cacb18..689891f0d1a5bf 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -121,9 +121,10 @@ int RunNodeInstance(MultiIsolatePlatform* platform, if (is_building_snapshot) { // It contains at least the binary path, the code to snapshot, - // and --embedder-snapshot-create. Insert an anonymous filename - // as process.argv[1]. + // and --embedder-snapshot-create (which is filtered, so at least + // 2 arguments should remain after filtering). assert(filtered_args.size() >= 2); + // Insert an anonymous filename as process.argv[1]. filtered_args.insert(filtered_args.begin() + 1, node::GetAnonymousMainPath()); }