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.

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

chore: update electron@30.3.1 #225106

Merged
merged 13 commits into from
Aug 9, 2024
Merged

chore: update electron@30.3.1 #225106

merged 13 commits into from
Aug 9, 2024

Conversation

deepak1556
Copy link
Collaborator

@deepak1556 deepak1556 commented Aug 8, 2024

https://releases.electronjs.org/release/compare/v30.1.2/v30.3.1

Fixes #213780
Fixes #217220

Features:

@deepak1556 deepak1556 added this to the August 2024 milestone Aug 8, 2024
@deepak1556 deepak1556 self-assigned this Aug 8, 2024
@deepak1556 deepak1556 force-pushed the robo/update_electron branch from 0a04e8c to e834f75 Compare August 8, 2024 16:20
@deepak1556
Copy link
Collaborator Author

deepak1556 commented Aug 9, 2024

Following task smoke tests fail consistently on linux product build with issue opening integrated terminal, logs show the following error, fyi @Tyriar

The terminal process failed to launch: A native exception occurred during launch ("fd" must be a positive integer: undefined)

Tried a couple of debugging steps:

yarn smoketest-no-compile --build <insiders-build>
  • No changes in either Node.js update or Electron update stands out that could cause this regression.
  • Not so surprising but using the node-pty version before the tty change does pass in product builds https://dev.azure.com/monacotools/Monaco/_build/results?buildId=287174&view=results, maybe this was an issue we had all along but still doesn't answer why only the current update makes it more consistent in CI.
  • Running product build with OSS electron to rule out any internal patches also repros the failure, agents and software included are same for both pipelines (product and dev). Only difference is running out of sources vs bundled build. Wonder if it is some kind of sandbox issue, will run with sandbox disabled as next step.

@deepak1556
Copy link
Collaborator Author

Wonder if it is some kind of sandbox issue, will run with sandbox disabled as next step.

Still fails, I am out of ideas with a lack of repro. We need some logging in the node-pty module. @Tyriar @meganrogge can I disable the smoke tests to unblock this PR and create a follow-up issue to debug further. It would also help to see if we get any real world issues with repro around this failure.

@Tyriar
Copy link
Member

Tyriar commented Aug 9, 2024

@deepak1556 if it's erroring at tty.ReadStream it seems like this would totally brick a terminal. I'll leave it up to you on whether this should be merged in now or not based on what you think the impact will be and how fast better logging can be put in place.

@deepak1556
Copy link
Collaborator Author

The problem is that I don't have an idea of the impact given the issue only repros in the product CI. I will try these as next steps,

  1. Disable the tests and merge the Electron update
  2. Add logging to see why fd is invalid in CI
  3. Additionally, wait for some real world repro to turn up
  4. Enable back the tests eventually

@deepak1556 deepak1556 force-pushed the robo/update_electron branch from 8596ef5 to e64326b Compare August 9, 2024 14:25
@deepak1556 deepak1556 marked this pull request as ready for review August 9, 2024 16:35
@deepak1556 deepak1556 enabled auto-merge (squash) August 9, 2024 16:35
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

CI network flake?

@deepak1556 deepak1556 merged commit 00427fd into main Aug 9, 2024
6 checks passed
@deepak1556 deepak1556 deleted the robo/update_electron branch August 9, 2024 23:44
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Sep 24, 2024
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.

VSC keeps crashing Linux file dialog does not use default file path
2 participants