Conversation
There was a problem hiding this comment.
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
rceditcall 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
|
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. |
|
I think we should also update the conpty version to v1.23.251008001 that comes with the node-pty bump. I'll manually test again once build passes. /cc @Tyriar |
deepak1556
left a comment
There was a problem hiding this comment.
This doesn't need a fix in node-pty, please follow the pattern similar to
vscode/build/npm/postinstall.ts
Lines 112 to 126 in f1db08a
|
New verification build: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=383805&view=results |
|
New verification build: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=383817&view=results |
|
@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. |
|
New verification build: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=383839&view=results |
|
Closing now that we have microsoft/node-pty#829 |
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.