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

update to Electron 15.1.2 and support Node.js 14 #9936

Merged
merged 16 commits into from
Jan 27, 2022

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Aug 20, 2021

What it does

We currently have a hard dependency on electron, meaning that once
packaged we have a redundant electron installation part of the
application.

A workaround has been to configure the packagers to ignore that runtime
dependency on electron, but this is a workaround that has to be done
for each app.

In order to make it as easy as possible for adopters to consume the
framework, the goal is to encapsulate Electron-specific dependencies
into @theia/electron and have electron itself added to application's
package.json under devDependencies.

As part of updating the version of electron, this commit also fixes
the dependency problems in the framework:

  • Update electron from 9.4.4 to ^15.3.5.
  • Move the ffmepg related C code into its own @theia/ffmpeg package.
  • Move the ffmpeg replace and check scripts from @theia/electron to
    @theia/cli as commands: ffmpeg:replace and ffmpeg:check.
  • Run the FFMPEG library replacing for Electron when doing theia build on the electron target.
  • Add theia-re-exports utility to generate re-exports and generate
    documentation (based on mustache templates).
  • Break @theia/electron to re-export Electron-specific dependencies as
    @theia/electron/shared/....
  • Modify @theia/core re-export scripts to re-export everything from
    @theia/electron/shared/... as @theia/core/electron-shared/....
  • Update the @theia/eslint-plugin rule that helps with re-using shared
    exports within the code base.
  • Modify the generators to automatically add electron to
    devDependencies for Electron applications.
  • Add utilities to detect or set electron-rebuild's ABI when
    rebuilding modules for Electron as it might get lost with versions
    that are too recent.
  • Re-export ajv from @theia/core.
  • Update the package.json scripts to silence yarn, otherwise it will
    stack error messages when nested yarn run calls are executed.

How to test

  • Everything should work like before.
  • Delete the electron entry from examples/electron/package.json#devDependencies.
  • Running yarn build in the Electron example application should add it back.
  • Delete the @theia/electron entry from examples/electron/package.json#dependencies.
  • Running yarn build in the Electron example application should fail because of the missing dependency.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added electron issues related to the electron target dependencies pull requests that update a dependency file labels Aug 20, 2021
@paul-marechal
Copy link
Member Author

paul-marechal commented Aug 20, 2021

Technically just bumping Electron is a rather trivial change. Most of the diff is about fixing the dependency issues we noticed in downstream applications.

@paul-marechal paul-marechal force-pushed the mp/electron-update branch 3 times, most recently from 85112b2 to 12d0984 Compare August 20, 2021 03:53
@paul-marechal
Copy link
Member Author

paul-marechal commented Aug 20, 2021

About the --ignore-engines on Node.js 14: It seems like the issue comes from dugite-extra that constrains the engine. I am not sure why this is done, since dugite does not really care about the engine.

@msujew
Copy link
Member

msujew commented Aug 20, 2021

@paul-marechal The reason dugite-extra has the engine limit is due to find-git-exec. I recently updated the repo and removed the limit. We'll release a newer version at the start of next week. There's already a PR in the dugite-extra repo for removing the limit, see eclipse-theia/dugite-extra#49, which will be updated accordingly.

@vince-fugnitto vince-fugnitto self-requested a review August 20, 2021 12:21
@vince-fugnitto vince-fugnitto added the CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) label Aug 20, 2021
@vince-fugnitto
Copy link
Member

@marcdumais-work I assume we can start registering the necessary parts for a cq now so it might be completed in time for the review.

I'll link the past cq we performed for electron (only visible to committers):

@vince-fugnitto
Copy link
Member

I created the two CQs (electron + ffmpeg) and included them in the pull-request description 👍

@paul-marechal
Copy link
Member Author

Ah, looks like we'll need to uplift @theia/node-pty if we want Theia to install/build on mac with node 16...

@linhanyu
Copy link
Contributor

Will this PR apply on 1.18.0 ? I saw it's add to milestone

@vince-fugnitto
Copy link
Member

Will this PR apply on 1.18.0 ? I saw it's add to milestone

@linhanyu it likely won’t be ready in time for 1.18.0, we are still waiting for the dependency (electron) to be approved by the Eclipse Foundation before we can proceed.

@paul-marechal paul-marechal removed this from the 1.18.0 milestone Sep 23, 2021
@vince-fugnitto
Copy link
Member

@paul-marechal I went ahead and filed a CQ to be able to use electron@15.1.2. I updated the pr description to reference the necessary CQs.

@linhanyu
Copy link
Contributor

linhanyu commented Oct 19, 2021

@paul-marechal I went ahead and filed a CQ to be able to use electron@15.1.2. I updated the pr description to reference the necessary CQs.

Accroding to
#9846

Our tester report crash on their M1 device. Currently it's our most significant problem to promotion。
Although there is problem of open-source project,I think theia community can provide interface or cli argument to users,to replace electron manually by downstream developers,so we can develop some internal test products,or open source without exact electron version dependence to avoid license conflict?

@vince-fugnitto
Copy link
Member

#9936 (comment)

@linhanyu unfortunately I do not believe it is possible, we must wait for the CQ to resolve before we can officially support the newer electron version. Upgrading electron is much more involving that simply updating the dependency (as you can see in this change) so the idea of passing a command line argument would likely not work well.

@paul-marechal
Copy link
Member Author

@linhanyu I am currently stuck with other work in addition to having to move this PR from Electron 14 to 15.

AFAIK it might be tricky for downstream projects to use an arbitrary version of Electron if the APIs between the supported version and the custom one changed to much, but maybe it should be possible using resolution and some rebindings?

Regarding the issue you mentioned on M1, is that when using Electron 9? Would it be fixed by moving to 15?

@linhanyu
Copy link
Contributor

linhanyu commented Oct 19, 2021 via email

@linhanyu
Copy link
Contributor

@linhanyu I am currently stuck with other work in addition to having to move this PR from Electron 14 to 15.

AFAIK it might be tricky for downstream projects to use an arbitrary version of Electron if the APIs between the supported version and the custom one changed to much, but maybe it should be possible using resolution and some rebindings?

Regarding the issue you mentioned on M1, is that when using Electron 9? Would it be fixed by moving to 15?

@vince-fugnitto @paul-marechal
Can we just merge the cli and electron encapsulate part to master branch?Just not change electorn and ffmpeg versions. It's seems work on current version of electron-9.4.4 and node12. So I can hack on more easily
I have tried hack in our downstream product。But it's made us hard to follow community changes (Good architecture that can be iterated continuously is the main reason for adopting theia for us.)

@paul-marechal
Copy link
Member Author

@linhanyu I'm sorry I don't understand. Are you saying that Electron 9.4.4 and Node 12 works? Those are the versions on master already.

We can't do much with this PR as long as the CQ is pending. Also now that I now what I need to do with this it should be done for the release after the one this week.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

The outstanding issues identified (bug from electron itself, and open folder) are resolved, and feedback from other reviews are addressed.

I confirmed that the following work well:

  • build and starting the app
  • searching (ripgrep)
  • debugging
  • scm
  • searching extensions
  • language features
  • electron specific functionality related to windows, menus and context-menus

If issues are identified after the release we can address them and think about patch releases if anything is critical (users can always use previous versions of theia).

@paul-marechal paul-marechal merged commit 82f152d into master Jan 27, 2022
@github-actions github-actions bot added this to the 1.22.0 milestone Jan 27, 2022
@JonasHelming
Copy link
Contributor

@paul-marechal Thank you very much for working on this! @msujew @vince-fugnitto @colin-grant-work : Thank you for your testing and review efforts!

@paul-marechal
Copy link
Member Author

paul-marechal commented Jan 28, 2022

I noticed a minor issue with the downloaded ffmpeg.dll file (Windows) which isn't in our .gitignore file:

image

We should probably add a ignore entry for that?

@msujew I haven't been able to reproduce that issue? Is it still happening?

edit: I don't see how this can happen now that the cache for the shared library is the following:

const ffmpegCachedPath = path.join(os.tmpdir(), `theia-cli/cache/electron-v${electronVersion}`, ffmpegName);

@JonasHelming
Copy link
Contributor

I had a similar issue with the Yeoman template, did not yet investigate, see eclipse-theia/generator-theia-extension#127.

Steps to reproduce:
npm install -g yo generator-theia-extension
yo theia-extension
select hello world
@paul-marechal
not sure whether this is related though

@JonasHelming
Copy link
Contributor

"Error: Cannot find module '../build/Debug/ffmpeg.node'"

@paul-marechal
Copy link
Member Author

@JonasHelming #10686

It's the kind of issue that I only learn about when actually publishing :(

We'll do a patch release with all the files properly published.

@JonasHelming
Copy link
Contributor

@paul-marechal I agree, it is not possible to find all potential errors before publishing! I would say the good news is that we have the infrastructure in place to catch these issues, the nightly of the generator failed. However, there was not enough time between the merge of this PR and the release so we were not able to react. The nightly failed after the release was already done. I believe this is something we can easily fix process wise.
I think this PR was important enough to justify the last minute merge and some regressions were expected. I will add he topic of how to make sure the nightlys run before release on our dev call agenda. Thank you for already working on a fix!

@JonasHelming
Copy link
Contributor

@paul-marechal
Copy link
Member Author

paul-marechal commented Jan 28, 2022

@JonasHelming the workflow you linked actually failed with the same known previous error, the reason being that theia/...@next is having trouble getting published as lately our CI fails before getting to the publish step... Don't know why yet.

We tried building our own test apps using 1.22.1 instead and we were successful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) dependencies pull requests that update a dependency file electron issues related to the electron target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants