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

Jump to bottom

discord: 0.0.5 -> 0.0.8 [backport to release-18.09] #54018

Merged
merged 4 commits into from Jan 17, 2019
Merged

discord: 0.0.5 -> 0.0.8 [backport to release-18.09] #54018

merged 4 commits into from Jan 17, 2019

Conversation

MP2E
Copy link

@MP2E MP2E commented Jan 15, 2019 •
edited by worldofpeace

discord now depends on gtk3 instead of gtk2. Unfortunately there is no public changelog to the Linux client specifically, so other changes are unknown.

Motivation for this change

Fixes #54003

Older versions of Discord become unusable every update that comes out, so it'd be nice to make sure our stable NixOS users don't lose the ability to use it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • other Linux distributions
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace
Copy link
Contributor

It would be great if we could accomplish #54003 (comment)

@samueldr
Copy link
Member

Hi! Thanks for the backport.

We prefer using git cherry-pick -x with backported commits. This is helpful in tracking the source of the changes.

@MP2E
Copy link
Author

MP2E commented Jan 16, 2019

I have just force pushed over this branch with a commit that uses git cherry-pick -x and I will look into the proper channels to notify Discord about this, since it has been causing us issues since the early versions of Discord

@worldofpeace
Copy link
Contributor

This fails to build with

these derivations will be built:
  /nix/store/gdm2cq422502a1cw9bmm84fzzkajmd1b-discord-0.0.7.drv
building '/nix/store/gdm2cq422502a1cw9bmm84fzzkajmd1b-discord-0.0.7.drv'...
unpacking sources
unpacking source archive /nix/store/s6f97am593xs281kvjdncgcfai481y02-discord-0.0.7.tar.gz
source root is Discord
setting SOURCE_DATE_EPOCH to timestamp 1547577376 of file Discord/postinst.sh
patching sources
configuring
no configure script, doing nothing
building
no Makefile, doing nothing
installing
warning: working around a Linux kernel bug by creating a hole of 675840 bytes in ‘/nix/store/snj0p53ar8rr3v7wmk9dyaqxkc7vsfi3-discord-0.0.7/opt/discord/Discord’
file /nix/store/snj0p53ar8rr3v7wmk9dyaqxkc7vsfi3-discord-0.0.7/opt/discord/Discord is not a valid ELF executable (invalid SHT_ entry:1)
builder for '/nix/store/gdm2cq422502a1cw9bmm84fzzkajmd1b-discord-0.0.7.drv' failed with exit code 1
error: build of '/nix/store/gdm2cq422502a1cw9bmm84fzzkajmd1b-discord-0.0.7.drv' failed

@MP2E
Copy link
Author

MP2E commented Jan 16, 2019 •
edited

Hmm that is strange, it built on my machine, but I'm based off of current master. I'll build the release branch and see what I can find.

EDIT: Yep, seeing the exact same issue when I try to build release-18.09 with this commit! I'm not entirely sure how to proceed with fixing this, but will keep looking

@worldofpeace
Copy link
Contributor

Removing paxmark m $out/opt/discord/Discord should fix this.

@worldofpeace
Copy link
Contributor

Looking at the git history paxmark was removed from stdenv.

But that's not on 18.09. Shouldn't matter however if we remove it since it's not actually useful.

@MP2E
Copy link
Author

MP2E commented Jan 16, 2019

Thanks @worldofpeace ! Right before posting this comment version 0.0.8 came out, breaking Discord yet again... Hurray... :(

But I have quickly tested some things and everything appears to be working even without paxmark, and removing it makes it build for me on release-18.09. So I have pushed a commit bumping discord and removing the paxmark line to master and backported that to this PR

@worldofpeace
Copy link
Contributor

Thanks @worldofpeace ! Right before posting this comment version 0.0.8 came out, breaking Discord yet again... Hurray... :(

🤣 Yeah that isn't very friendly for us

and removing the paxmark line to master and backported that to this PR

The paxmark line with removed in 1b146a8#diff-ec90eee065c44b72c6000faa6d9078c2 so you'll have to add a commit here to remove it.

@MP2E
Copy link
Author

MP2E commented Jan 16, 2019

Thanks for the catch! I'm wondering if we should maybe wait on merging this into stable, just in case discord gives us another update over the next 24 hours. In the meantime, I'll try to find a means of contacting them.

The reason I mention this, is that while a quick test seemed to be fine, I noticed that if I go to any conversation or server with significant history, there appear to be severe performance problems where there were none with 0.0.7. And because of Discord's update policy, we can't simply revert...

worldofpeace
worldofpeace previously approved these changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace 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, builds. Though I haven't tested the function with 18.09, it should be fine 🤞

@worldofpeace
Copy link
Contributor

Thanks for the catch! I'm wondering if we should maybe wait on merging this into stable, just in case discord gives us another update over the next 24 hours. In the meantime, I'll try to find a means of contacting them.

Just now reading this after i approved, and I agree that we should wait just in case they do a sneaky update again.

The reason I mention this, is that while a quick test seemed to be fine, I noticed that if I go to any conversation or server with significant history, there appear to be severe performance problems where there were none with 0.0.7. And because of Discord's update policy, we can't simply revert...

Indeed, that might also be something to contact them with as well.

@worldofpeace worldofpeace dismissed their stale review January 16, 2019 01:59

should be cautious of sneaky updates

@MP2E
Copy link
Author

MP2E commented Jan 16, 2019 •
edited

Argh, I feel a bit silly. The 'performance issues' were due to me trying to build and run the package from release-18.09 on my system running NixOS master. So that's a non-issue, as the package build off master works with no performance issues on my system.

I'm guessing release-18.09 will also be just fine if I do a full system rebuild based off of it, will try that tonight and report back.

@MP2E MP2E changed the title discord: 0.0.5 -> 0.0.7 [backport to release-18.09] discord: 0.0.5 -> 0.0.8 [backport to release-18.09] Jan 16, 2019
@worldofpeace
Copy link
Contributor

I'm guessing release-18.09 will also be just fine if I do a full system rebuild based off of it, will try that tonight and report back.

I'd recommend building a vm like

NIX_PATH=nixpkgs=**path to nixpkgs checkout**:nixos-config="$PWD"/configuration.nix nixos-rebuild build-vm

@MP2E
Copy link
Author

MP2E commented Jan 16, 2019

Thanks again, that is a really useful command! I had to bump up the default amount of RAM the VM uses by default to get Discord to work, but I see no problems at all. I haven't found a good channel to contact Discord yet (they recommend just pinging their Discordapp account on Twitter..) but I am still looking. In the meantime, I think if there are no updates in the next few hours, this should be good to merge.

Cray Elliott added 4 commits January 16, 2019 17:34
discord: 0.0.5 -> 0.0.7
a76438c 
discord now depends on gtk3 instead of gtk2, unfortunately
there is no public changelog, so other changes are unknown

(cherry picked from commit 4e4a9ba)
discord: 0.0.7 -> 0.0.8
01f54e8 
also fixes evaluation on release-18.09

(cherry picked from commit d5d5453)
discord: remove line referencing paxmark
c2eb7d7
discord: fix gsettings crash when selecting file
8a00a45 
(cherry picked from commit ff2341a)
@MP2E
Copy link
Author

MP2E commented Jan 17, 2019

Found that Discord was crashing when trying to upload a file, due to not being able to find gsettings-schema. Fixed! Haven't found any other issues, and things seem to have slowed down. With that, I think this is ready to be merged, given it passes a review :)

@Mic92 Mic92 merged commit 8139daa into NixOS:release-18.09 Jan 17, 2019
@MP2E MP2E deleted the discord_stable_bump branch January 19, 2019 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MP2E @worldofpeace @samueldr @Mic92 @GrahamcOfBorg