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. try affinity flag #12210

Merged
merged 15 commits into from Jun 11, 2018
Merged

Conversation

chrisnojima
Copy link
Contributor

@chrisnojima chrisnojima commented Jun 5, 2018

This updates electron to 2.0.2 and updates the packager and some packager deps.
This also turns on the affinity flag: electron/electron#11501 which reduces the overhead when we have multiple remote windows going

  • window
  • linux
  • darwin
  • update webpack preset env

@@ -15,7 +15,12 @@ const desktopPath = (...args) => path.join(__dirname, ...args)
const copySyncFolder = (src, target, onlyExts) => {
const srcRoot = desktopPath(src)
const dstRoot = desktopPath(target)
const files = klawSync(srcRoot, {filter: item => onlyExts.includes(path.extname(item.path))})
const files = klawSync(srcRoot, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

klaw passes the folders to us so if we reject it it won't keep going down

const comment: string = argv.comment || ''
// $FlowIssue // flow-typed libdef is pretty weak, thinks this might be a boolean
const outDir: string = argv.outDir || ''
const shouldBuildAnArch: string = (argv.arch: any)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets just cast instead of flowissue

dir: desktopPath('./build'),
electronVersion: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

flow doesn't like us assigning a var later to a key that doesn't exist

@@ -173,15 +182,16 @@ function pack(plat, arch, cb) {
}
}

packager(opts, cb)
return packager(opts)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we get deprecation warnings if we don't use the promise version

@chrisnojima
Copy link
Contributor Author

@oconnor663
@zanderz can you do test builds on windows/linux and test that this works. please ensure the menu widget and trackers work as expected

@oconnor663
Copy link
Contributor

@chrisnojima it seems ok to me when I do a local build.

@chrisnojima
Copy link
Contributor Author

has to be what is a production type build and not any kind of hot server etc

@codecov
Copy link

codecov bot commented Jun 5, 2018

Codecov Report

Merging #12210 into master will decrease coverage by 0.08%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12210      +/-   ##
==========================================
- Coverage   86.03%   85.94%   -0.09%     
==========================================
  Files        1232     1200      -32     
  Lines       61490    61215     -275     
==========================================
- Hits        52902    52614     -288     
- Misses       8588     8601      +13
Impacted Files Coverage Δ
shared/menubar/remote-proxy.desktop.js 92.85% <ø> (ø) ⬆️
shared/desktop/app/menu-bar.desktop.js 53.4% <100%> (+1.64%) ⬆️
...ared/desktop/remote/sync-browser-window.desktop.js 89.47% <100%> (+0.43%) ⬆️
shared/desktop/webpack.config.babel.js 82.45% <100%> (ø) ⬆️
shared/desktop/app/user-data.desktop.js 50% <33.33%> (ø) ⬆️
shared/desktop/package.desktop.js 66.66% <60%> (-6.14%) ⬇️
shared/desktop/app/main-window.desktop.js 79.01% <83.33%> (+1.38%) ⬆️
shared/common-adapters/header.js.flow
...d/login/register/select-other-device/index.js.flow
... and 50 more

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 7f81c3d...598565c. Read the comment docs.

@chrisnojima chrisnojima changed the base branch from nojima/DESKTOP-update-dev-deps to master June 6, 2018 14:43
@chrisnojima chrisnojima changed the title WIP: update to electron. try affinity flag Update to electron. try affinity flag Jun 6, 2018
@chrisnojima
Copy link
Contributor Author

@keybase/react-hackers we can look at this while waiting on a windows build

@chrisnojima
Copy link
Contributor Author

@zanderz when you have a chance can we test a windows build of this

@chrisnojima
Copy link
Contributor Author

@zanderz lets prioritize this monday

@zanderz
Copy link
Contributor

zanderz commented Jun 8, 2018 via email

@chrisnojima
Copy link
Contributor Author

@keybase/react-hackers can someone review. i'm going to kick admin builds off this commit so we can play with it today

@chrisnojima
Copy link
Contributor Author

@oconnor663 / @zanderz can you double check notifications work

@chrisnojima
Copy link
Contributor Author

@keybase/react-hackers lets review this and get it in

@adamjspooner
Copy link
Contributor

I'm guessing the insecure CSP warning is expected.

@chrisnojima
Copy link
Contributor Author

thats only in dev

@chrisnojima
Copy link
Contributor Author

@keybase/react-hackers bump

@adamjspooner
Copy link
Contributor

I ran this branch for most of the morning with no issues, but that was with hot-server and start-hot. Everything seems fine to me. Is that enough to approve it?

@chrisnojima
Copy link
Contributor Author

chrisnojima commented Jun 11, 2018 via email

@adamjspooner
Copy link
Contributor

How do I do that?

@zanderz
Copy link
Contributor

zanderz commented Jun 11, 2018

Sorry, I have missing notifications with build https://prerelease.keybase.io/windows/Keybase_2.1.0-20180607094851%2B399affe0c6.386.exe, which I think is supposed to contain the windows fix. Still looking.

@zanderz
Copy link
Contributor

zanderz commented Jun 11, 2018

building a new windows one off this branch now

@adamjspooner
Copy link
Contributor

I'm not sure it's relevant, but I get this error when I set an exploding message time: Error: ERROR CODE 218 - method 'updateCategory' not found in protocol 'keybase.1.gregor' in method keybase.1.gregor.updateCategory

@chrisnojima
Copy link
Contributor Author

unrelated, i just merged master back in so that kind of stuff won't show up

Copy link
Contributor

@adamjspooner adamjspooner left a comment

Choose a reason for hiding this comment

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

Looks good on macOS!

@zanderz
Copy link
Contributor

zanderz commented Jun 11, 2018

@chrisnojima
Copy link
Contributor Author

k i'm merging

@chrisnojima chrisnojima merged commit 3d3e869 into master Jun 11, 2018
@chrisnojima chrisnojima deleted the nojima/DESKTOP-electron-2 branch November 26, 2018 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants