mgalgs / gnome-shell-system-monitor-applet

Display system informations in gnome shell status bar, such as memory usage, cpu usage, network rates…
https://extensions.gnome.org/extension/3010/system-monitor-next/
GNU General Public License v3.0
220 stars 19 forks source link

GPU: Un-hardcode /bin/bash location #74

Closed HeroBrine1st closed 6 months ago

HeroBrine1st commented 6 months ago

What I tested

Changes

This PR changes hardcoded /bin/bash to /usr/bin/env bash in GPU module, fixing non-working GPU module for non-FHS distributions such as NixOS.

Actually tested with direct patch to NixOS package, but this PR is so small it doesn't need throrough testing.

The NixOS override I use for now ```nix {gnomeExtensions}: gnomeExtensions.system-monitor-next.overrideAttrs (old: { patches = old.patches ++ [ ./usr-bin-env.patch ]; }) ``` Mentioned patch is (the same as in this PR): ```diff --- a/extension.js +++ b/extension.js @@ -2161,7 +2161,7 @@ const Gpu = class SystemMonitor_Gpu extends ElementBase { // Run asynchronously, to avoid shell freeze try { let path = this.extension.path; - let script = ['/bin/bash', path + '/gpu_usage.sh']; + let script = ['/usr/bin/env', 'bash', path + '/gpu_usage.sh']; // Create subprocess and capture STDOUT let proc = new Gio.Subprocess({argv: script, flags: Gio.SubprocessFlags.STDOUT_PIPE}); ```

Also tested this patch on Arch Linux (FHS distribution) - GPU module works as expected.

Warning: I did not test this patch on master branch, I did only on latest (66) version from e.g.o


This change is Reviewable

HeroBrine1st commented 6 months ago

Another (and probably better) option is to not use /bin/bash in command and instead place it in shebang. /bin/sh is already there and is actually patched by nix builder, but I don't think zip tarball can include file permissions, so it would be a regression for e.g.o users.

HeroBrine1st commented 6 months ago

I forgot to check other PRs. This PR conflicts with #71, but those two can be easily merged as both are small. IDK how to make a PR that depends on another PR so I leave this as-is.

If you want me to merge this with #71, just tell me.