Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Update Electron to 0.36.12 #11348

Merged
merged 9 commits into from Apr 30, 2016
Merged

Update Electron to 0.36.12 #11348

merged 9 commits into from Apr 30, 2016

Conversation

50Wliu
Copy link
Contributor

@50Wliu 50Wliu commented Apr 2, 2016

Continuation of #10983.

Closes #10983.
Fixes #11198.

/cc @drewmnoel

@joefitzgerald
Copy link
Contributor

Bump up to 0.36.12?

@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 2, 2016

Bump up to 0.36.12?

Nice catch! No idea that existed.

@50Wliu 50Wliu changed the title Update Electron to 0.36.11 Update Electron to 0.36.12 Apr 2, 2016
@joshaber joshaber added the atom label Apr 4, 2016
@Nikki1993
Copy link

Archlinux, builds fine and atom operates as usual.

@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 4, 2016

The failing spec seems to be flaky on master so it can probably be ignored.

@joefitzgerald
Copy link
Contributor

Friends don't let friends let flakey tests stay flakey 😆

@Nikki1993
Copy link

Atom is still broken on master without this PR. Same as in another PR, can we get this merged asap?

@n0f3
Copy link

n0f3 commented Apr 6, 2016

what are the failing checks referring to?

@awnumar
Copy link

awnumar commented Apr 9, 2016

Months of waiting simply to update electron.

@n0f3
Copy link

n0f3 commented Apr 10, 2016

@50Wliu you mentioned that the failing spec seems to be flaky on master as well, does this need to be rebuilt, or can we just approve this PR back to master? I'm wondering if there is anything that we can do to speed this process up

@lee-dohm
Copy link
Contributor

What can be done to help speed the process up is if people can give us an idea of what testing they have done to ensure that this doesn't create any regressions. We all want to update to the latest version but only the Atom team is on the hook if a regression is introduced. Since Electron can affect literally anything from Atom's UI to how it interacts with the underlying OS to what JavaScript features are available, any Electron upgrade is potentially a breaking change.

So, if people want to help, here's what you can do:

  1. Build the branch
  2. Run the entire test suite (including all built-in packages) by executing script/test
  3. Use Atom in a wide variety of tasks (preferably the same tasks on different operating systems)

If you run into any problems, get steps to reproduce the problem and verify that the problem does not exist on the latest master. If you can do that, report it here.

If you do not run into any problems, add a link to a detailed report here (you can store the report in a Gist) of the things you did including:

  1. The Atom version including the change number you ran
  2. What OS, OS version and machine specs you run Atom on
  3. What kinds of tasks you performed
  4. What packages you have installed
  5. Your entire config.cson (feel free to redact anything you consider sensitive, but don't just remove the content ... indicate that something was redacted)
  6. Anything else you consider relevant

@Nikki1993
Copy link

I went as far as updating, locally, electron to 0.37.5 (however all details/findings apply to current PR 0.36.12) and in my 2 days of testing I noticed 0 regressions on ArchLinux, Gnome-Shell running under X. Main workflow consists of developing websites with AngularJS with a couple of extra community packages.

Building latest from master, running 1.9.0 DEV atom version and the build is successful. Only changes made were edits to package.json to upgrade electron to 37.5 and changes listed in this PR.

script/test shows no problems apart from

Running "mkdeb" task
>> /usr/bin/fakeroot: line 178: dpkg-deb: command not found
Warning: null Use --force to continue.
Error: null
  at ChildProcess.<anonymous> (/home/nikki/git/atom/build/tasks/task-helpers.coffee:61:37)
  at emitTwo (events.js:100:13)
  at ChildProcess.emit (events.js:185:7)
  at maybeClose (internal/child_process.js:850:16)
  at Process.ChildProcess._handle.onexit (internal/child_process.js:215:5)

But that's 99% because of the fact that I am using Arch and not a debian based system hence the mkdeb issue.

My Confg.cson


"*":
  "activate-power-mode":
    screenShake: {}
  "atom-beautify":
    css:
      newline_between_rules: true
      no_lead_zero: true
      preserve_newlines: true
      selector_separator_newline: true
    general:
      _analyticsUserId: "0db11db5-33b9-49c0-898e-6cde6f599b50"
    html:
      indent_size: 2
      wrap_attributes_indent_size: 2
    scss:
      indent_size: 2
      newline_between_rules: true
      no_lead_zero: true
      preserve_newlines: true
  "atom-material-ui":
    colors:
      abaseColor:
        red: 21
        green: 101
        blue: 192
        alpha: 1
      accentColor:
        red: 255
        green: 0
        blue: 0
        alpha: 1
    tabs:
      tintedTabBar: true
    treeView:
      blendTabs: true
      compactList: true
    ui:
      panelContrast: true
      panelShadows: true
  "autoclose-html":
    legacyMode: true
  core:
    autoHideMenuBar: true
    themes: [
      "atom-material-ui"
      "atom-material-syntax"
    ]
  editor:
    backUpBeforeSaving: true
    fontFamily: "Iosevka"
    fontSize: 16
    softWrap: true
  "exception-reporting":
    userId: "c3cbdaa4-0055-9ead-7c6d-707da5f15280"
  "linter-scss-lint":
    executablePath: "/home/nikki/.gem/ruby/2.3.0/bin/scss-lint"
  welcome:
    showOnStartup: false

Packages installed:

  • activate-power-mode
  • atom-beautify
  • atom-material-syntax
  • atom-material-ui
  • autoclose-html
  • emmet
  • file-icons
  • git-plus
  • git-time-machine
  • linter
  • linter-scss-lint
  • minimap
  • pigments

I hope this helps, I do understand that the electron in question is 0.36.12 but I do believe there is no active regressions (at least in general sense of use) to update electron to 0.37.5.

@awnumar
Copy link

awnumar commented Apr 13, 2016

I've been running this patched branch for a few days now. Running smoothly so far. I'll attach config and logs after some more testing.

@amytruong
Copy link

*Mac and Windows
Basic Smoke Test

  • Run through Welcome Guide
  • Install and update Packages - fails to delete/uninstall with a message but closing/re-opening app fixes the problem
    screen shot 2016-04-11 at 10 58 21 am
  • Install and update Themes (same as ☝️ with themes)
  • Verify speed and response time
  • Verify scrolling is fast
  • Verify File menu bars/navigation is functioning

@50Wliu not sure if these are related to Electron at all but on stable, packages and themes are fine. Just giving you a heads up just in case 😄

@amytruong
Copy link

Also, noticed something a little odd. If you select/unselect options, the "Default" will go in & out. Again, I can't be sure about it being related to Electron or not but just documenting to be safe. 😄 (Windows)
weird

@lee-dohm
Copy link
Contributor

@Nikki1993 Please stick to the branch as implemented. We're not looking at updating to Electron v0.37.x yet. Let's constrain testing reports on this PR to ones that actually test this PR.

@Nikki1993
Copy link

@lee-dohm understand. I was testing this PR for the past week and no issues arise, using 0.36.12 electron. Everything what I wrote for 0.37.5 electron applies to 0.36.12 no regressions that I can find that would disturb the workflow.

@lee-dohm
Copy link
Contributor

no regressions that I can find that would disturb the workflow.

As someone who used to work in QA, the bolded text doesn't fill me with confidence that no regressions were actually found 😉

@Nikki1993
Copy link

@lee-dohm again, there is only so much that can be found. I tested atom from the point I am using it and everything is flawless. I try to go above and beyond, but priority atm is to get atom in a working condition for Arch users. I always leave a seed of doubt, hence the

no regressions that I can find that would disturb the workflow.

Naturally, I tend to test software extensively when the time allows but presently atom is broken on Arch based systems and causes headaches to update. I feel like if no problems are found that would break things even further, it's safe to merge and deploy. This PR is a successor to different PR which has been around for a few weeks already.

P.S. I am interested in QA testing and I support that a great deal of tests should be executed before merging framework changes, however when things are broken I believe they can be sped up a little bit :P

@lee-dohm
Copy link
Contributor

I appreciate where you're coming from and, in general, agree with your point of view. What we're looking for is "regressions". My apologies for assuming that everyone is familiar with the term. A regression is defined as something that used to work that no longer does. So yes, we're on the same page.

What confused me about your phrasing is that it sounded like you had found regressions (again, things that once worked that no longer do) but had discounted them as not disruptive to some workflow. Since exhaustive testing is generally not possible in any non-trivial system (and definitely not possible in one of Atom's size), any regression is cause for concern. But from your latest message, it sounds like you didn't find any regressions ... just things that haven't ever worked on Arch?

@lee-dohm
Copy link
Contributor

When you say "latest master", are you referring to Atom's latest master? Or the atom-editor Arch Linux package's latest master?

@Nikki1993
Copy link

@lee-dohm latest Atom's master from git. I build atom-editor myself, not through Arch AUR system.

@Nikki1993
Copy link

However, julia-client is NOT crashing on Atom 1.6.2 Windows 10 64bit.

@lee-dohm
Copy link
Contributor

Ok, since it is from the latest master, please open a separate issue for it.

@alyst
Copy link

alyst commented Apr 13, 2016

@lee-dohm atom-editor AUR package uses 1.7.0 release + the patch based on this PR as a hotfix. So it could be a regression in the stable atom 1.7.0 (unless it's some julia-client internal bug).

@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 13, 2016

The fact that atom-editor is (only partially) patching in this PR in is extremely concerning.
As described in a couple of places, the filepaths for common Electron modules were changed, and Atom's module cache needs to be updated to reflect that (as has been done in this PR). However based on @Nikki1993 and @alyst's reports, it appears that atom-editor failed to include the entire PR diff and only updated the Electron version, which will lead to many packages failing to activate, like julia-client or travis-ci-status.

@amytruong I've actually been experiencing that for a while now (at least for two packages) on master.
Can definitely reproduce the Default flickering though (it seems to also extend a bit to the right).

@alyst
Copy link

alyst commented Apr 13, 2016

@50Wliu Sorry for the confusion, I've checked the patch that the AUR package is actually using and it does not look like the whole PR (however, I have really no idea how it should look). OTOH @Nikki1993 reports the independent build has the same issue.

@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 13, 2016

Upon looking closer at @Nikki1993's error, that one doesn't seem to be due to module require problems and seems to me like something is wrong with atom.asar.

@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 14, 2016

This test should have failed prior to the path updates, but it didn't, maybe because this expect doesn't have an assertion (eg .toBeTruthy())? I'll see if I can get it working properly.

@amytruong I'm seeing the default flicker even on Atom 1.9.0-dev-043982c.

@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 14, 2016

The aforementioned spec has been fixed and I found a bug as a result 😄.

@alyst
Copy link

alyst commented Apr 14, 2016

I was trying to build AUR package using the patch from this branch (not including the changes from the last hour) instead of the package.patch, but then Atom was freezing again at startup even in the safe mode (although AFAIR non-patched atom 1.7.0 was not showing the file contents at all, the patched one showed it).
When I additionally included the changes from the package.patch (nodegit is updated to 0.12.2), everything was fine, including the julia-client package.
However, there are currently difficulties with APM packages downloading and I had to restart the atom-edit package build several times, so I'm not entirely sure if the freezing is really related to nodegit.

@sjug
Copy link

sjug commented Apr 14, 2016

@alyst The 1.7.0-4 package now contains all of this PR which went up about 15 minutes ago.

The nodegit update is a i686 specfic issue patch unrelated to this PR, you can remove that change and see if it helps your issues.

@alyst
Copy link

alyst commented Apr 14, 2016

@sjug Thanks, 1.7.0-4 so far is working fine for me.

@ryanpcmcquen
Copy link

All works here. Everything in script/test passes (except for mkdeb, which I cannot test on Slackware):

~/Downloads/atom-wl-drewmnoel-electron:$ script/test
Node: v4.4.3
npm: v2.13.3
Installing build modules...
=> Took 2764ms.

Installing apm...
=> Took 841ms.

Deleting old packages...
Removing .atom-ci-fingerprint ✓
=> Took 1116ms.

Installing modules ✓
=> Took 3167ms.

Deduping modules ✓
=> Took 3445ms.

Running "output-disk-space" task

Running "download-electron" task

Running "download-electron-chromedriver" task

Running "build" task

Running "babel:dist" (babel) task

Running "coffee:glob_to_multiple" (coffee) task
>> 334 files created.

Running "prebuild-less:src" (prebuild-less) task
>> 0 files compiled, 1891 files reused

Running "cson:glob_to_multiple" (cson) task
>> 172 files compiled to JSON.

Running "peg:glob_to_multiple" (peg) task
Generating parser from "node_modules/snippets/lib/snippet-body.pegjs"...
Parser "~/Downloads/atom-wl-drewmnoel-electron/out/Atom/resources/app/node_modules/snippets/lib/snippet-body.js" generated in 26ms.

Running "generate-license:save" (generate-license) task

Running "generate-module-cache" task

Running "compile-packages-slug" task

Running "fingerprint" task
Wrote ci fingerprint: ~/Downloads/atom-wl-drewmnoel-electron/node_modules/.atom-ci-fingerprint 9c072ab8dc1aa60545b237c27666af2e0e74cd14

Running "set-version" task

Running "check-licenses" task

Running "standard:src" (standard) task

Linting files...

✓ No Problems

Running "coffeelint:src" (coffeelint) task
>> 118 files lint free.

Running "coffeelint:build" (coffeelint) task
>> 29 files lint free.

Running "coffeelint:test" (coffeelint) task
>> 73 files lint free.

Running "csslint:src" (csslint) task
>> 0 files lint free.

Running "lesslint:src" (lesslint) task
>> 29 files lint free.

Running "generate-asar" task

Running "mktar" task
>> Created ~/Downloads/atom-wl-drewmnoel-electron/out/atom-1.8.0-dev-amd64.tar.gz

Running "publish-build" task

Running "upload-assets" task

Done, without errors.


Execution Time (2016-04-19 14:35:44 UTC)
loading tasks             1.9s  ▇▇ 3%
build                     6.4s  ▇▇▇▇▇ 12%
babel:dist                1.2s  ▇ 2%
coffee:glob_to_multiple   7.1s  ▇▇▇▇▇ 13%
prebuild-less:src         1.1s  ▇ 2%
generate-license:save    821ms  ▇ 1%
generate-module-cache     1.2s  ▇ 2%
check-licenses           566ms  ▇ 1%
standard:src              1.2s  ▇ 2%
coffeelint:src            5.2s  ▇▇▇▇ 9%
coffeelint:build         557ms  ▇ 1%
coffeelint:test           8.3s  ▇▇▇▇▇▇ 15%
lesslint:src              1.5s  ▇ 3%
generate-asar             6.3s  ▇▇▇▇▇ 11%
mktar                    11.5s  ▇▇▇▇▇▇▇▇ 21%
Total 55.2s

~/Downloads/atom-wl-drewmnoel-electron:$

It is great to have a working Atom again!

@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 19, 2016

There's no need to run script/test on Linux as no tests are actually run.

@ryanpcmcquen
Copy link

@50Wliu what is all the output then?

@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 19, 2016

As I briefly described in #11516, that's just Atom building itself and then making sure it can publish a release. Currently only OS X runs the specs when script/test is run.

@as-cii as-cii merged commit 0050225 into master Apr 30, 2016
@as-cii as-cii deleted the wl-drewmnoel-electron branch April 30, 2016 07:19
@as-cii
Copy link
Contributor

as-cii commented Apr 30, 2016

Thanks @50Wliu and @drewmnoel! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet