-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Final bits to prepare electron distribtion: #2653
Conversation
* Remove the config: nobody else wants our update URL so we'll keep it separately. Don't copy the config. * Script to yell at you if you've build a package with auto update turned off. * s/vector/webapp/ when looking for config * Use different update URLs for the various platforms
It just has our update URL in it which only we care about
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.
Looks fine, but I have questions...
@@ -35,7 +35,7 @@ | |||
"build:compile": "babel --source-maps -d lib src", | |||
"build:bundle": "NODE_ENV=production webpack -p --progress", | |||
"build:bundle:dev": "webpack --optimize-occurence-order --progress", | |||
"build:electron": "npm run clean && npm run build && cpx electron/config.json webapp/ && build -lwm", | |||
"build:electron": "rimraf electron/dist && npm run clean && npm run build && build -wm --ia32 --x64 && scripts/check-electron.sh", |
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 add electron/dist
to the list of things cleaned by clean
?
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.
build -wm --ia32 --x64
no linux build?
Does this mean that we get 32-bit and 64-bit mac builds, or does --ia32 not apply to the mac build?
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.
There is now a linux build (deb). What this means is we get both 32 and 64 bit builds for windows and linux, but not mac because mac went 64 bit ages ago.
@@ -0,0 +1,54 @@ | |||
#!/bin/bash |
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.
Is this a script that will be run manually, or as part of the release process? Should it be run via npm run
?
@@ -1,72 +0,0 @@ | |||
{ |
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.
Is this related to https://github.com/matrix-org/internal-config/pull/57 ?
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.
Yep, I've moved this to our internal config repo given that nobody else cares what the config for our build of the electron app is.
* Remove squirrel hooks (the installing & uninstalling is now done by the, er, installer) * Switch to electron-auto-update * Shorten initial update delay because we no longer need to wait for squirrel to release a lock file * Change update URLs because windows is now one installer for both 32 & 64 bit. * Update electron-builder to 10 where NSIS is now the default target for Windows. * Add linux to the target list, building a deb. * Remove sqirrel-specific installation spinner * Remove redundant !**/* from files
Amalgamate the electron build packaging into one script. Use update_base_url so we can compute the actual URL in the script for windows (because we need to put it in the build) and at runtime for mac os.
@@ -134,14 +135,15 @@ | |||
"dereference": true, | |||
"//files": "We bundle everything, so we only need to include webapp/", | |||
"files": [ | |||
"!**/*", |
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.
This was just redundant, ftr.
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.
<shrug> lgtm
which it turns out is by far the lesser of two evils. * Auto-update works with a proxy * The update process is reasonably atomic & faster, rather than running the uninstaller then the installer, leaving you with a broken install if you shut down your machine at the wrong time * Gets the update URL the same way as on mac, rather than baking it into the app at build time from package.json. We don't want it in package.json because only our builds want our update URL.
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.
uh, still lgtm I think
keep it separately. Don't copy the config.
update turned off.