-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[GH-9095] Update to Electron 11 #9096
Conversation
67a9000
to
fefdbf3
Compare
Thank you, @AirSeeker. Please make these changes also:
Why? Was it required to be able to bump up to electron 11.x? If no, please do it in a subsequent PR. We want to minimize the changeset. Thank you! |
Signed-off-by: Petro Dzyuba <[email protected]>
fefdbf3
to
13f48e2
Compare
@kittaakos, I updated the description of PR, remove yarn upgrade changes from commit, and update the version of node for a project and in doc. |
@kittaakos, about CI I don't know for sure, but I think that if the project has |
Thanks for checking it. I agree; let's leave the CI as is. |
@AirSeeker, did you try to build it with Python 2?
I am getting an error like this 👆:
It's not a blocker, but we need to clearly communicate that Python 2 won't work. |
@AirSeeker, it does not work with Python 3.x either. Did you manage to build and run it locally? Can you share some details about your env? Thanks! I found a corresponding PR in |
@kittaakos I did build locally on Windows with Python 3.8.2. |
Thanks! I'll give it a try on another mac and Windows machines. |
I believe that @marechal-p was the last to uplift the fork for theia-ide/node-pty. |
I can confirm, it builds on Windows but not on macOS. I am using
if this matters. I also tried to use the upstream |
@kittaakos, I can't test on macOS, but I tested on Ubuntu 20.04 VM with Python 3.8.5 and project builds without errors and electron launch successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads-up, we will need to file appropriate CQs in order to accept the changes similarly to previous efforts:
@AirSeeker, no problem. I do not expect you to have access to all platforms, but the purpose of this electron update is to support the new M1 chip on macOS. We need someone who can verify that it is still possible to build it on macOS. In fact, we should verify it somehow with the new M1 core also. The entire change is useless if the electron part is OK, but we have other Theia issues. Of course, I might have the build issues because there is a problem with my env, but we need the steps on how to solve it. |
It's not your env. I'm seeing exactly the same problems on my Macbook Air M1.
It's not a problem with node-pty. I hacked out node-pty and the build failed with the same error with nsfw and others. I should have a chance to look further at this on Wednesday. |
@westbury do you need help with debugging this on an m1 machine? |
Closing in favor of #9936 Please share your feedback there if any. |
What it does
This PR:
Closes #9095.
How to test
yarn
in project root directory.yarn start:electron
yarn rebuild:clean && yarn rebuild:electron
and repeat previous step.process.versions
and verify that electron version is 11.Review checklist
Reminder for reviewers