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

Upgrade to Electron 4 #6307

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Upgrade to Electron 4 #6307

merged 1 commit into from
Oct 9, 2019

Conversation

thegecko
Copy link
Member

@thegecko thegecko commented Oct 2, 2019

Signed-off-by: thegecko rob.moran@arm.com

What it does

Upgrades the electron version used to be the latest version 4 (4.2.11) to align with VS Code usage of electron.

This also means the static versions of packaged tools available within Electron are also upgraded:

Node: 10.11.0
Chromium: 69.0.3497.106

As discussed in the dev meeting, this is a step towards adopting Electron 6. We should consider doing that once VS Code have stabilised their use of it.

@marcdumais-work Will this require CQs to be raised for the new dependency version?

How to test

Build application locally and run

Review checklist

Reminder for reviewers

cc @benoitf @akosyakov

@akosyakov akosyakov added the electron issues related to the electron target label Oct 2, 2019
@marcdumais-work
Copy link
Contributor

marcdumais-work commented Oct 2, 2019

Hi @thegecko,

Generally, for production dependency updates, "running" this process is good enough, assuming we do not find surprises. Can you give this a try and ping @eclipse-theia/ip-help if you have questions?

For Electron, we need to pay special attention since we went through a lot of trouble last spring to "license certify" its usage despite the bundled LGPL-licensed libffmpeg that also can include proprietary codecs. I used Gitpod to quickly test your PR for that aspect, and it looks like our mechanism, that replaces libffmpeg with a version that does not contain proprietary codecs, still works.

gitpod /workspace/theia $ cd dev-packages/electron/
gitpod /workspace/theia/dev-packages/electron $ node ./electron-codecs-test.js 
"/workspace/theia/node_modules/electron/dist/libffmpeg.so" does not contain proprietary codecs (16 found).

So I think we're good for Electron.

Ref: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19939

@marcdumais-work
Copy link
Contributor

Update: to be safe I sent an email to Sharon (with @thegecko in cc), to confirm if we should take further steps wr to the Electron update or if the above is satisfactory.

@kittaakos
Copy link
Contributor

I do not mind this update, it is excellent, but I cannot verify it. What would you expect from a reviewer?

@thegecko
Copy link
Member Author

thegecko commented Oct 7, 2019

What would you expect from a reviewer

I guess all we can do is check it works. Things like menus, dialogs, etc.

@kittaakos
Copy link
Contributor

I guess all we can do is check it works. Things like menus, dialogs, etc.

Got it, thanks! So no bundled electron app verification is required. I will check it soon.

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have verified the electron example on macOS. I have checked:

  • the main application menu,
  • the context menus in the editor and the navigator.

I have tried the bare-minimum TS LS capabilities with

  • validation,
  • CA, and
  • the outline.

I started a few new terminals and executed a few Git commands from the Git view. It worked as expected.

The Node.js version also looks good to me:

npx electron -i
> console.log(process.versions.node)
10.11.0
undefined
> .exit

Thank you! 👍

@thegecko
Copy link
Member Author

thegecko commented Oct 7, 2019

Thanks @kittaakos I'll consider merging this once I've completed the process above.

@thegecko
Copy link
Member Author

thegecko commented Oct 7, 2019

Thanks @marcdumais-work, I've followed the process outlined above for this PR.

All licences listed for electron 4 are permitted in the Approved Third Party Content Licenses

  • Apache-2.0
  • BSD-2-Clause
  • BSD-3-Clause
  • CC-BY-3.0
  • CCO-1.0
  • ISC
  • MIT

Is this enough to merge or do you want to complete the email thread with Sharon?

@marcdumais-work
Copy link
Contributor

All licences listed for electron 4 are permitted in the Approved Third Party Content Licenses

Thanks Rob

Is this enough to merge or do you want to complete the email thread with Sharon?

Yes, I think you can go ahead and merge

@thegecko thegecko merged commit 46a3bf3 into eclipse-theia:master Oct 9, 2019
@thegecko thegecko deleted the electron-4 branch October 9, 2019 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants