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

fix(app-shell): Update Electron and add macOS app notarization #4011

Merged
merged 5 commits into from
Sep 10, 2019

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Sep 6, 2019

overview

This PR updates a lot of our underlying Electron / app packaging tooling to ensure we are ready for new security requirements in the upcoming version of macOS (10.15 Catalina).

This Electron update will bring reliability and security improvements to all our users, but out of necessity, is going to introduce a little bit of user pain for our Linux and Windows users out of the gate in exchange for a better long term experience.

Closes #3997, closes #2567

the good

  • When macOS 10.15 is launched in the coming weeks, our users won't get scary security alerts
  • Our macOS distributable is switching from ZIP to DMG
    • Now, when users double-click their Mac download, they'll get instructions to drag the app to their /Applications folder
  • Windows users can now choose to install for all users (if they have admin permission, see Install the App for all users #2567)
  • The app will be on Node v12 and Chromium v76, which is always nice for us (e.g. built-in flatMap!)
  • Copy / paste works in the devtools now!

the bad

  • Support for macOS 10.9 Mavericks has been dropped
  • We need to recommend to Windows users that they uninstall 3.11.x before installing this update to avoid weird problems caused by installation location bugs fixed in electron-builder
  • Our macOS builds are going to take about 5 to 10 minutes longer because of the notarization process

wash

  • The installation experience of the Linux app is changing
    • Bad news: we need to direct them to install AppImageLauncher once, before downloading our app
    • Good news: once they've got AppImageLauncher installed, all they have to do to install the app is to double-click the download
    • In other words, no more messing with file permissions before launching; AppImageLauncher will just handle it

changelog

  • Update electron and electron-builder dependencies
  • Update electron-builder configuration to support macOS notarization

review requests

This PR is going to require stress testing on all of our OS's. Given that we don't have a choice on this update (because of macOS security), I think as long as we do various smoke tests with this PR, we should rely on our release QA to really make sure this is solid. Scariest areas that the Electron side of things deals with that we'll need to keep an eye on:

  • App self-update
  • App config
  • Robot discovery
  • Buildroot updates

@mcous mcous added app Affects the `app` project ready for review fix PR fixes a bug labels Sep 6, 2019
@codecov
Copy link

codecov bot commented Sep 7, 2019

Codecov Report

Merging #4011 into edge will increase coverage by 0.32%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #4011      +/-   ##
==========================================
+ Coverage   57.68%   58.01%   +0.32%     
==========================================
  Files         828      829       +1     
  Lines       23500    23713     +213     
==========================================
+ Hits        13557    13757     +200     
- Misses       9943     9956      +13
Impacted Files Coverage Δ
app-shell/electron-builder.config.js 0% <0%> (ø)
app-shell/src/main.js 0% <0%> (ø) ⬆️
app-shell/src/menu.js 0% <0%> (ø) ⬆️
opentrons/commands/commands.py 92.83% <0%> (-0.9%) ⬇️
opentrons/hardware_control/modules/thermocycler.py 90.53% <0%> (-0.84%) ⬇️
opentrons/protocol_api/contexts.py 86.23% <0%> (-0.12%) ⬇️
opentrons/commands/types.py 100% <0%> (ø) ⬆️
shared-data/js/labwareTools/index.js 94.11% <0%> (+2.01%) ⬆️
opentrons/server/rpc.py 93.07% <0%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c60457...3ebb9d0. Read the comment docs.

@mcous mcous mentioned this pull request Sep 9, 2019
20 tasks
@mcous mcous requested review from a team September 9, 2019 14:43
@Kadee80
Copy link
Contributor

Kadee80 commented Sep 9, 2019

Making a note about this here so it doesn't get forgotten:

OT App page on website currently lists supported OS as macOS 10.9 or later
Once this is merged and released we will need to update that to macOS 10.10 or later

@mcous please confirm i'm correct here with the new version support

@mcous
Copy link
Contributor Author

mcous commented Sep 9, 2019

@Kadee80 that is correct. Filed Opentrons/opentrons-dot-com#230 to track

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcous mcous merged commit 246d6db into edge Sep 10, 2019
@mcous mcous deleted the app_notarize-mac-builds branch September 10, 2019 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App release: Update macOS release process for notarization Install the App for all users
3 participants