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 Segfault on Ubuntu 18.10. Fixes 3391# #3400

Merged

Conversation

@zerkz
Copy link
Contributor

@zerkz zerkz commented Oct 22, 2018

Fixes #3391

  • Upgrade Electron to 2.0.8 (solves https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/1790966)
  • Upgrade discord presence module to 0.7. 0.5 was 404ing when getting the dependency. you can see this in previous test runs/etc. Not sure what's up.
  • Built/tested using node 9.4.0 (same node as gpmdp/build-64 image)
  • Adds - instead of spaces in the product name, to get rid of node-gyp build warnings about having a builddir with spaces.
- Upgrade Electron to 2.0.8
- Upgrade discord presence module to 0.7 (0.5 was 404ing?)
- Built/tested using node 9.4.0 (same node as gpmdp/build-64 image)
@zerkz zerkz changed the title Fix Segfault on Ubuntu 18.10. Fixes #3391 Fix Segfault on Ubuntu 18.10. Fixes 3391# Oct 22, 2018
@jostrander
Copy link
Collaborator

@jostrander jostrander commented Oct 22, 2018

Does the spaces change cause the packaging to build with dashes?

Loading

@zerkz
Copy link
Contributor Author

@zerkz zerkz commented Oct 22, 2018

@jostrander It does (pulls productName from package.json), but the installers seem unaffected (they use a different naming template). Dashes are legal in Windows filenames as well.

Should the version be bumped for this by the way? Just realized I didn't do anything with that.

Loading

@jostrander
Copy link
Collaborator

@jostrander jostrander commented Oct 22, 2018

No, versioning happens after it's pulled into master.

Loading

@zerkz
Copy link
Contributor Author

@zerkz zerkz commented Oct 22, 2018

@jostrander 👍 , let me know if I need to make any changes.

It looks like the CircleCi Darwin tests have been failing for a bit - https://circleci.com/gh/MarshallOfSound/Google-Play-Music-Desktop-Player-UNOFFICIAL-/2754

Loading

@jostrander
Copy link
Collaborator

@jostrander jostrander commented Oct 22, 2018

With the exception of the dash change I think this PR is good to go. At the very least we should change that in a different issue/PR/commit and review it's impact separately as to keep this in scope about making it build work on 18.10.

Loading

@zerkz
Copy link
Contributor Author

@zerkz zerkz commented Oct 22, 2018

@jostrander Good points. I was grasping at anything whenever I was debugging this, so definitely can be handled in another PR. I reverted the change with another commit.

Loading

Copy link
Collaborator

@jostrander jostrander left a comment

LGTM 👍

Loading

@jostrander jostrander requested a review from MarshallOfSound Oct 22, 2018
@MarshallOfSound MarshallOfSound merged commit c0c9e51 into MarshallOfSound:master Oct 23, 2018
8 of 10 checks passed
Loading
@welcome
Copy link

@welcome welcome bot commented Oct 23, 2018

Congrats on merging your first pull request! Have a

Loading

@Steampunkery
Copy link

@Steampunkery Steampunkery commented Oct 23, 2018

Thanks for the snappy fix! Could one of you compile the deb for 64 bit Ubuntu 18.10? I assume someone here has it given the thread. I would do it myself, but I'm not a node guy and I keep getting errors when I try. Funny part is I can npm start to run the app after building but I can't compile it.

Loading

@jostrander
Copy link
Collaborator

@jostrander jostrander commented Oct 23, 2018

Loading

@zerkz
Copy link
Contributor Author

@zerkz zerkz commented Oct 23, 2018

@Steampunkery I have a deb built but I've been slapped on the wrists before for linking biniaries/etc (see #3278 (comment))

@jostrander provided a good solution though!

Loading

@Steampunkery
Copy link

@Steampunkery Steampunkery commented Oct 23, 2018

No, @jostrander that one is the broken one. It would seem that circleCI hasn't built the new version. But the tests passed so that's good.

Loading

@jostrander
Copy link
Collaborator

@jostrander jostrander commented Oct 23, 2018

CircleCI has definitely built the new version:

https://circleci.com/gh/MarshallOfSound/Google-Play-Music-Desktop-Player-UNOFFICIAL-/2838

This is the latest build on master, if you login to CircleCI you can view the artifacts tab where you can download the latest build. It's still labeled 4.6.1 because the package version hasn't been incremented for release.

image

Also confirmed inside the deb you can view the version of electron in the version file which is v2.0.8. Previously it was 2.0.2 from the version the releases tab.

Loading

@Steampunkery
Copy link

@Steampunkery Steampunkery commented Oct 23, 2018

Wow thanks! I was looking for something like a downloads tab. As you can tell, I'm new to CircleCI. It works flawlessly now.

Loading

@christofdamian
Copy link

@christofdamian christofdamian commented Oct 30, 2018

FYI: this problem also happens on Fedora 29, which will be released today.
And the build 2838 from above fixes it.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants