From b5ee1b09dfd2dbad5abe761b284b236fdcf7d90c Mon Sep 17 00:00:00 2001 From: Neil Date: Tue, 15 Sep 2026 22:51:07 -0700 Subject: [PATCH] fix(windows): repair what running the node-pty suites on Windows exposed Putting these files on the Windows job turned four assertions red on the first run. Three of them were in tests that carried `skipIf(platform !== 'win32')` and had therefore never executed anywhere, on any branch. - `writeFakeElectronRebuild` emitted the `windows-process-tree` addon a real rebuild leaves but never node-pty's, so every Windows test of the rebuild path ran against a tree no real rebuild can produce: node-pty "rebuilt" with nothing in `build/Release`. The new same-host check reads that state correctly and said so. The fake rebuild now writes `build/Release/conpty.node` when it was asked to rebuild node-pty for win32, with the marker and the target machine. - `mkTempProject` never staged `windows-process-tree-creation-time.cjs`. The rebuild script reaches it through `projectRequire`, which resolves against the project root, so the module walker cannot follow it and must not try. Staged by name, with a comment saying which of the two it is. Without it the windows-process-tree probe failed to load its own checker and the module joined `modulesToRebuild`, which is the second and third red assertion. - the two `nodePtyAddonPath` cases compared against a literal POSIX string. `resolve` returns a drive letter and backslashes on Windows, so they could only ever pass off it. Built from segments now, which still pins the `..` traversal that is the point of the test. Verified on macOS: ensure-native-runtime-job-ownership, verify-packaged-node-pty-job-ownership, windows-pe-machine, script-module-dependencies, rebuild-native-deps-node-pty, rebuild-native-deps, rebuild-native-deps-windows-process-tree, ensure-native-runtime -- 109 passed, 6 skipped. The 6 are the Windows-gated rebuild tests, which is the job this change is aimed at; Windows CI is the arbiter. --- ...sure-native-runtime-job-ownership.test.mjs | 12 ++-- .../rebuild-native-deps-test-fixtures.mjs | 56 +++++++++++++++---- 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/config/scripts/ensure-native-runtime-job-ownership.test.mjs b/config/scripts/ensure-native-runtime-job-ownership.test.mjs index 410d6ecc2a9..8ff3fcc12cc 100644 --- a/config/scripts/ensure-native-runtime-job-ownership.test.mjs +++ b/config/scripts/ensure-native-runtime-job-ownership.test.mjs @@ -1,7 +1,7 @@ import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs' import { createRequire } from 'node:module' import { tmpdir } from 'node:os' -import { dirname, join } from 'node:path' +import { dirname, join, resolve } from 'node:path' import { describe, expect, it, vi } from 'vitest' import { peImage } from './windows-pe-image-fixture.mjs' @@ -152,24 +152,26 @@ describe('assertNodePtyJobOwnership', () => { }) describe('nodePtyAddonPath', () => { + // Built from segments rather than a POSIX string: on Windows `resolve` returns + // a drive letter and backslashes, so a literal only ever passed off Windows. it('resolves the addon against node-pty lib, which is the only base callers share', () => { expect( nodePtyAddonPath( - '/app/node_modules/node-pty/lib/utils.js', + resolve('/app/node_modules/node-pty/lib/utils.js'), { dir: '../build/Release/' }, 'conpty' ) - ).toBe('/app/node_modules/node-pty/build/Release/conpty.node') + ).toBe(join(resolve('/app/node_modules/node-pty'), 'build', 'Release', 'conpty.node')) }) it('handles the bundled layout, where the addon sits beside lib', () => { expect( nodePtyAddonPath( - '/app/resources/node-pty/lib/utils.js', + resolve('/app/resources/node-pty/lib/utils.js'), { dir: './build/Release/' }, 'conpty' ) - ).toBe('/app/resources/node-pty/lib/build/Release/conpty.node') + ).toBe(join(resolve('/app/resources/node-pty/lib'), 'build', 'Release', 'conpty.node')) }) }) diff --git a/config/scripts/rebuild-native-deps-test-fixtures.mjs b/config/scripts/rebuild-native-deps-test-fixtures.mjs index 936ad9eeea8..337659998b5 100644 --- a/config/scripts/rebuild-native-deps-test-fixtures.mjs +++ b/config/scripts/rebuild-native-deps-test-fixtures.mjs @@ -14,6 +14,16 @@ import { fileURLToPath } from 'node:url' import { copyScriptWithLocalModules } from './script-module-dependencies.mjs' import { peImage } from './windows-pe-image-fixture.mjs' +/** + * The wide literal `usesCygwinRuntime` holds, as it sits in a real addon. A + * fixture addon without it is a build that predates the MSYS breakaway denial, + * which is what these tests need to be able to represent. + * + * Taken from the gate itself: a re-typed copy agrees with a stale gate by + * construction, which is the one thing these fixtures must not do. + */ +const { CYGWIN_BREAKAWAY_MARKER } = createRequire(import.meta.url)('./node-pty-job-ownership.cjs') + const sourceScriptPath = fileURLToPath(new URL('./rebuild-native-deps.mjs', import.meta.url)) const sourceInstallScriptPath = fileURLToPath( new URL('./install-electron-package-binary.mjs', import.meta.url) @@ -24,6 +34,11 @@ const sourceNodePtyJobOwnershipPath = fileURLToPath( const sourceWindowsProcessTreeGypRebuildPath = fileURLToPath( new URL('./windows-process-tree-gyp-rebuild.mjs', import.meta.url) ) +// Reached through projectRequire, so the module walker cannot see it: that +// specifier resolves against the project root, not against the script. +const sourceWindowsProcessTreeCreationTimePath = fileURLToPath( + new URL('./windows-process-tree-creation-time.cjs', import.meta.url) +) const sourceWindowsProcessTreePatchPath = fileURLToPath( new URL('../patches/@vscode__windows-process-tree@0.8.0.patch', import.meta.url) ) @@ -93,6 +108,10 @@ export function mkTempProject() { copyFileSync(sourceScriptPath, join(projectDir, 'config', 'scripts', 'rebuild-native-deps.mjs')) copyScriptWithLocalModules(sourceInstallScriptPath, join(projectDir, 'config', 'scripts')) copyScriptWithLocalModules(sourceNodePtyJobOwnershipPath, join(projectDir, 'config', 'scripts')) + copyFileSync( + sourceWindowsProcessTreeCreationTimePath, + join(projectDir, 'config', 'scripts', 'windows-process-tree-creation-time.cjs') + ) copyFileSync( sourceWindowsProcessTreeGypRebuildPath, join(projectDir, 'config', 'scripts', 'windows-process-tree-gyp-rebuild.mjs') @@ -224,10 +243,33 @@ const FAKE_ADDON_BYTES = { * because "produced the upstream reader" and "produced nothing" are both real * outcomes that assertion has to tell apart. */ +/** What a real node-gyp Windows build leaves behind, per target arch. */ +const FAKE_CONPTY_BY_ARCH = Object.fromEntries( + ['x64', 'arm64'].map((arch) => [ + arch, + Buffer.concat([peImage({ arch }), CYGWIN_BREAKAWAY_MARKER]).toString('base64') + ]) +) + export function writeFakeElectronRebuild(projectDir, { logPathEnv = null, addon = 'clean' } = {}) { const rebuildDir = join(projectDir, 'node_modules', '@electron', 'rebuild') mkdirSync(rebuildDir, { recursive: true }) writeFileSync(join(rebuildDir, 'package.json'), JSON.stringify({ type: 'module' })) + // A real Windows rebuild leaves conpty.node in build/Release; a fake one that + // does not makes the gates downstream see a tree that cannot happen. + const emitNodePty = ` + const nodePtyDir = join('node_modules', 'node-pty') + if ( + options.platform === 'win32' && + (options.onlyModules ?? []).includes('node-pty') && + existsSync(join(nodePtyDir, 'lib', 'utils.js')) + ) { + mkdirSync(join(nodePtyDir, 'build', 'Release'), { recursive: true }) + writeFileSync( + join(nodePtyDir, 'build', 'Release', 'conpty.node'), + Buffer.from(${JSON.stringify(FAKE_CONPTY_BY_ARCH)}[options.arch] ?? '', 'base64') + ) + }` const emitAddon = addon === 'none' ? '' @@ -239,7 +281,7 @@ export function writeFakeElectronRebuild(projectDir, { logPathEnv = null, addon join(packageDir, 'build', 'Release', 'windows_process_tree.node'), ${JSON.stringify(FAKE_ADDON_BYTES[addon])} ) - }` + }${emitNodePty}` const emitImports = addon === 'none' ? '' @@ -269,7 +311,7 @@ export async function rebuild(options) {${emitAddon} } ` : `${emitImports} -export async function rebuild() {${emitAddon} +export async function rebuild(options) {${emitAddon} } ` ) @@ -337,16 +379,6 @@ export function writeFakeNodePtyConptyPayload( writeFileSync(join(sourceDir, 'OpenConsole.exe'), `OpenConsole.exe ${arch}`) } -/** - * The wide literal `usesCygwinRuntime` holds, as it sits in a real addon. A - * fixture addon without it is a build that predates the MSYS breakaway denial, - * which is what these tests need to be able to represent. - * - * Taken from the gate itself: a re-typed copy agrees with a stale gate by - * construction, which is the one thing these fixtures must not do. - */ -const { CYGWIN_BREAKAWAY_MARKER } = createRequire(import.meta.url)('./node-pty-job-ownership.cjs') - function writeFakeNodePtyAddon(nodePtyDir, nativeDir, { cygwinBreakawayDenied }) { const addonDir = resolve(join(nodePtyDir, 'lib'), nativeDir) mkdirSync(addonDir, { recursive: true })