Skip to content

chore: delete node-pty prebuilds#282528

Closed
rzhao271 wants to merge 6 commits intomainfrom
rzhao271/unblock-pty
Closed

chore: delete node-pty prebuilds#282528
rzhao271 wants to merge 6 commits intomainfrom
rzhao271/unblock-pty

Conversation

@rzhao271
Copy link
Copy Markdown
Collaborator

@rzhao271 rzhao271 commented Dec 10, 2025

This PR deletes the prebuilt binaries that come with the latest node-pty considering that VS Code does not require them and that they break the VS Code build.

Copilot AI review requested due to automatic review settings December 10, 2025 16:44
@rzhao271 rzhao271 added this to the December / January 2026 milestone Dec 10, 2025
@rzhao271 rzhao271 self-assigned this Dec 10, 2025
@rzhao271 rzhao271 requested a review from anthonykim1 December 10, 2025 16:44
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds error handling to the Windows dependency patching task to gracefully handle node-pty prebuilds that fail the rcedit operation. When a .node file from node-pty cannot be patched (likely because it's for the wrong OS), the code now catches the error, logs a warning, and attempts to delete the problematic file. This unblocks updating node-pty until a proper solution using optionalDependencies is implemented.

Key Changes:

  • Wraps the rcedit call in a try-catch block to handle failures for node-pty prebuilds
  • Logs a warning and deletes node-pty files that cannot be patched
  • Re-throws errors for non-node-pty dependencies to preserve existing error behavior

Comment thread build/gulpfile.vscode.ts Outdated
@rzhao271
Copy link
Copy Markdown
Collaborator Author

Verification build: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=383769&view=results

I'd also like to check how much the macOS and Linux binaries grow in size.

@anthonykim1
Copy link
Copy Markdown
Contributor

anthonykim1 commented Dec 10, 2025

I think we should also update the conpty version to v1.23.251008001 that comes with the node-pty bump.
https://github.com/microsoft/vscode/pull/282291/files

I'll manually test again once build passes.

/cc @Tyriar

Copy link
Copy Markdown
Collaborator

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

This doesn't need a fix in node-pty, please follow the pattern similar to

function removeParcelWatcherPrebuild(dir: string) {
const parcelModuleFolder = path.join(root, dir, 'node_modules', '@parcel');
if (!fs.existsSync(parcelModuleFolder)) {
return;
}
const parcelModules = fs.readdirSync(parcelModuleFolder);
for (const moduleName of parcelModules) {
if (moduleName.startsWith('watcher-')) {
const modulePath = path.join(parcelModuleFolder, moduleName);
fs.rmSync(modulePath, { recursive: true, force: true });
log(dir, `Removed @parcel/watcher prebuilt module ${modulePath}`);
}
}
}
and remove the prebuilds of node-pty during postinstall. We build our own versions for each target platform and the prebuilds were meant for 3rd party clients of node-pty.

@rzhao271 rzhao271 changed the title chore: delete wrong-OS node-pty prebuilds chore: delete node-pty prebuilds Dec 10, 2025
@rzhao271 rzhao271 requested a review from deepak1556 December 10, 2025 18:17
@rzhao271
Copy link
Copy Markdown
Collaborator Author

@rzhao271 rzhao271 marked this pull request as ready for review December 10, 2025 18:19
Comment thread build/npm/postinstall.ts Outdated
@rzhao271
Copy link
Copy Markdown
Collaborator Author

rzhao271 commented Dec 10, 2025

deepak1556
deepak1556 previously approved these changes Dec 10, 2025
@deepak1556
Copy link
Copy Markdown
Collaborator

@rzhao271 update the cachesalt, x64 ran with cache that has prebuilds not removed https://dev.azure.com/monacotools/Monaco/_build/results?buildId=383817&view=logs&jobId=12844632-35fb-5b51-892a-9f6fe8a80e65&j=c3c6d686-fd72-58b2-fb22-32758a96abc0&t=43669b33-f7cc-5b15-d4df-074717949166 leading to the universal step failure.

@rzhao271
Copy link
Copy Markdown
Collaborator Author

@rzhao271
Copy link
Copy Markdown
Collaborator Author

Closing now that we have microsoft/node-pty#829

@rzhao271 rzhao271 closed this Dec 10, 2025
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Jan 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants