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

betterdiscordctl: init at 1.7.0 #86943

Open
wants to merge 1 commit into
base: master
from

Conversation

@IvarWithoutBones
Copy link
Contributor

IvarWithoutBones commented May 5, 2020

Motivation for this change

Adds betterdiscordctl, a pretty useful tool for customizing Discord. Tested with stable.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link

ObserverOfTime left a comment

I'll add the package to the betterdiscordctl readme once this is merged. 👍

pkgs/tools/misc/betterdiscordctl/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/betterdiscordctl/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/betterdiscordctl/default.nix Outdated Show resolved Hide resolved
@bb010g
Copy link
Member

bb010g commented May 6, 2020

I wouldn't mind being marked as a maintainer (maintainers.bb010g).

@IvarWithoutBones IvarWithoutBones force-pushed the IvarWithoutBones:betterdiscord branch from b10b9a9 to cbb3b2c May 6, 2020
@ofborg ofborg bot requested a review from bb010g May 6, 2020
@IvarWithoutBones IvarWithoutBones force-pushed the IvarWithoutBones:betterdiscord branch from cbb3b2c to 630b470 May 6, 2020
@IvarWithoutBones IvarWithoutBones changed the title betterdiscordctl: init at 1.6.1 betterdiscordctl: init at 1.7.0 May 6, 2020
@IvarWithoutBones
Copy link
Contributor Author

IvarWithoutBones commented May 6, 2020

The ptb and canary branches now work as well if they're selected.

Also adressed other review comments, and updated to 1.7.0.

@ObserverOfTime
Copy link

ObserverOfTime commented May 6, 2020

Does this create a separate betterdiscordctl package for each version of Discord?

@IvarWithoutBones
Copy link
Contributor Author

IvarWithoutBones commented May 6, 2020

It does not, it allows you to select which branch to use with the branch variable (default is stable).
That sets the discord variable to the version of Discord selected, which gets used in the patchPhase.

@ObserverOfTime
Copy link

ObserverOfTime commented May 6, 2020

I'm not familiar with nix packaging. Can you change branches on the fly or do you have to reinstall it in order to use it on another Discord branch?

@IvarWithoutBones
Copy link
Contributor Author

IvarWithoutBones commented May 6, 2020

You'd have to reinstall as it needs to repatch. Not sure of a better solution. However as 99.99% of users just use 1 branch of Discord i dont think it should be much of an issue.

@ObserverOfTime
Copy link

ObserverOfTime commented May 6, 2020

How about patching it so that it prefixes /opt with ${pkgs.discord} or the corresponding flavor selected by the --flavor flag on runtime instead? This should probably be handled upstream by adding a --nix flag (akin to bb010g/betterdiscordctl#59) and enabling it by default in the nix package.

@IvarWithoutBones
Copy link
Contributor Author

IvarWithoutBones commented May 6, 2020

Sure, sounds good. If someone merges this (The installation derivation from that PR wouldn't be required as we handle that here) we can just wrap it to automatically use that flag here. Would indeed be a bit cleaner.

Edit: Oh, looks like that would only support the stable branch, as other binaries are named differently. Would require a bit more than just merging that.

@ObserverOfTime
Copy link

ObserverOfTime commented May 6, 2020

The PR implementation doesn't seem like it'll work with PTB or Canary either.
Anyway, I'll let @bb010g handle the rest since I'm way out of my depth here.

@IvarWithoutBones
Copy link
Contributor Author

IvarWithoutBones commented Jun 1, 2020

Any update on this? If not i think what I've done currently should be fine. I forgot to add documentation on how to switch discord versions in the package, so I'll add that now.

@IvarWithoutBones IvarWithoutBones force-pushed the IvarWithoutBones:betterdiscord branch 2 times, most recently from 7ade7fe to 919c55b Jun 1, 2020
@ObserverOfTime
Copy link

ObserverOfTime commented Jun 2, 2020

I made an attempt over at bb010g/betterdiscordctl#67.

If it's merged, patchPhase can be replaced with:

substituteInPlace betterdiscordctl --replace "^DISABLE_UPGRADE=$" "DISABLE_UPGRADE=yes"
substituteInPlace betterdiscordctl --replace "^nix=$" "nix=yes"

(Assuming regular expressions are supported.)

Also, the version should be changed to 1.7.1 and the branch derivation won't be needed.

@IvarWithoutBones IvarWithoutBones force-pushed the IvarWithoutBones:betterdiscord branch from 919c55b to 28f4483 Jul 4, 2020
@IvarWithoutBones
Copy link
Contributor Author

IvarWithoutBones commented Jul 4, 2020

Since it's taking a while for that PR to get merged upstream, I've just patched it in here. Current derivation works just fine, and is a lot cleaner than before IMO :)

Should be ready to merge.

Also, the version should be changed to 1.7.1 and the branch derivation won't be needed.

That release does not seem to exist?

@IvarWithoutBones IvarWithoutBones force-pushed the IvarWithoutBones:betterdiscord branch from 28f4483 to be23046 Jul 4, 2020
@ObserverOfTime
Copy link

ObserverOfTime commented Jul 4, 2020

That release does not seem to exist?

Yeah, I was going to publish it once the PR was merged.

--replace "DISABLE_UPGRADE=" "DISABLE_UPGRADE=1" \
--replace "nix=" "nix=yes"
Comment on lines 23 to 24

This comment has been minimized.

Copy link
@ObserverOfTime

ObserverOfTime Jul 4, 2020

Please use yes instead of 1 for consistency.

Also, substituting nix= with nix=yes will affect the following line as well:

@@ -207,7 +207,7 @@ while :; do
       die_non_empty '--flatpak'
       ;;
     --nix)
-      nix=yes
+      nix=yesyes
       ;;
     --upgrade-url)
       if [[ ${2+x} ]]; then upgrade_url=$2; shift

It doesn't particularly matter in this case but I'd like to avoid it if possible.

This comment has been minimized.

Copy link
@IvarWithoutBones

IvarWithoutBones Jul 4, 2020

Author Contributor

Mhm, I'm not sure if that's possible, sadly. Should indeed not really matter though.

This comment has been minimized.

Copy link
@ObserverOfTime

ObserverOfTime Jul 4, 2020

Are regular expressions not supported? Using ^nix=$ should fix it.

This comment has been minimized.

Copy link
@IvarWithoutBones

IvarWithoutBones Jul 5, 2020

Author Contributor

Looks like it doesn't.

@IvarWithoutBones IvarWithoutBones force-pushed the IvarWithoutBones:betterdiscord branch from be23046 to cb369cf Jul 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.