greshake / i3status-rust

Very resourcefriendly and feature-rich replacement for i3status, written in pure Rust
GNU General Public License v3.0
2.88k stars 475 forks source link

Use $SHELL for on_click #158

Closed lausek closed 6 years ago

lausek commented 6 years ago

At the moment, sh is hardcoded for execution of custom commands

https://github.com/greshake/i3status-rust/blob/881979bd7f356465d5e84fe898db08705309bab5/src/blocks/custom.rs#L118

I think that the $SHELL variable should be used here. That would also allow calling user defined commands like fish functions.

ammgws commented 6 years ago

Second this. Currently forced to explicitly call fish like so:

[[block]]
block = "custom"
command = "fish --command=ime_status"
on_click = "fish --command=ibus-toggle"
atheriel commented 6 years ago

On the face of things this makes sense to me, but I'd like to verify that $SHELL will always be what users expect it to be, and handle cases where the variable is empty.

lausek commented 6 years ago

@atheriel How should those cases be handled?

atheriel commented 6 years ago

Ok, so how would a user set the $SHELL environment variable? Would you expect users to run something like SHELL=/bin/fish i3status-rs in their i3 config? Or just rely on whatever is in their login configuration? Under what conditions are you (and others that are interested in this feature) setting $SHELL at the moment?

atheriel commented 6 years ago

Secondly, if we default to using $SHELL by default, are you aware of any user configurations that might break (e.g. because they are now executing under bash instead of sh)?

lovesegfault commented 6 years ago

@atheriel $SHELL is set, at least on Linux, with chsh, and is visible to all user processes, as far as I know. The env::var() call used in @lausek's implementation should work flawlessly, at least on Linux. I'm going to test it on FreeBSD and see what happens.

lausek commented 6 years ago

@atheriel As @bemeurer said, you can use chsh to set the $SHELL variable.

Regarding your second comment, are you thinking of something specific that sh can do but e.g. bash can't?

atheriel commented 6 years ago

I'm happy enough with the chsh answer (TIL!), but to clarify on the second, yes, I am wondering if assuming bash/fish will act like sh is a mistake. On doing some research I think this is probably OK:

According to this Stackoverflow answer, bash is not a POSIX-compliant implementation of sh, but the behaviour of bash is so similar as to be symlinked in most distributions. I assume the same is true of fish. So it's not unreasonable that users who set $SHELL can be expected to ensure that their shell choice is compatible with sh, and we can safely merge #159!

lovesegfault commented 6 years ago

@atheriel Watch out, although bash is quasi-POSIX compliant, the same is not true for fish. Fish is extremely non-POSIX, it's perhaps the most non-POSIX shell that is actually used by people, and most, if not all, of your csh/bash/POSIX-shell scripts will not run on Fish without considerable patching.

Thus, assuming sh compatibility is troublesome, if the person changes shell to fish it is very likely to cause issues.

c.f. https://github.com/fish-shell/fish-shell/issues/2382

lausek commented 6 years ago

@atheriel A thing I totally forgot about: standard blocks still use sh so they need to be switched too. Wouldn't it be better if we introduced a new variable in Config which holds the configured shell?

lovesegfault commented 6 years ago

Another good reference: http://hyperpolyglot.org/unix-shells

atheriel commented 6 years ago

@lausek There are lots of places where we shell out, but I don't think they all need to be changed. This particular substitution makes a lot of sense, because users expect to be able to call their own functions -- but when we shell out to a program (e.g. curl or alsamonitor), simply using sh seems more sensible.