alacritty / alacritty

A cross-platform, OpenGL terminal emulator.
https://alacritty.org
Apache License 2.0
53.72k stars 2.94k forks source link

Don't execute `zsh` startup files as part of our default macOS wrapper #7950

Closed nixpulvis closed 2 weeks ago

nixpulvis commented 4 weeks ago

Given a ~/.zshenv with echo hit inside, I've tested with both chsh -s /bin/sh which does not print hit anymore, while chsh -s /bin/zsh continues to print hit as it does on master.

fixes #7886

nixpulvis commented 3 weeks ago

Have you considered any other zsh options? I'm curious if this includes --no-globalrcs.

I've considered it, but after reading the man page for the options I don't think it will have any effect at the moment for an average macOS user. The only file in /etc/ on my macOS system is /etc/zshrc (which was there by default, but doesn't seem to be used). I'm more that happy to add the --no-global-rcs option however, if we think it will help future proof the code.

It's worth mentioning that ~/.zshrc is also not run on current master. So this patch really is just effecting the run of ~/.zshenv as #7886 requests.

@Learath2 I'm curious if you have any input, or could verify this patch.

Learath2 commented 3 weeks ago

This patch is exactly what I'm running locally. I haven't hit any issues with it after running it for a month. As for adding -d/--no-global-rcs, I think it might be a good idea just to futureproof, but I also don't see it currently changing anything.

The only reason I didn't contribute this patch myself was that I was wondering whether we should be wrapping the users shell setting from the config the same way we wrap the one set by chsh. Then I got distracted by some other work.

nixpulvis commented 3 weeks ago

I was wondering this myself, but after a brief discussion in #alacritty, it seems like the stance is that users who run their own shell.program should do the wrapping themselves if they care about it.

I'm personally still not convinced, since I hardly understand the need for the wrapping, I assume most users are also unaware of the need and will therefor do something like shell.program = "/bin/bash". In fact this is evident if you scan through the comments of many users in our issue tracker and elsewhere online. I understand the need to wrap the shell in a call to login I think, but I do not understand the call to zsh still.

But that could be considered a separate issue to this PR either way.

kchibisov commented 3 weeks ago

The only reason I didn't contribute this patch myself was that I was wondering whether we should be wrapping the users shell setting from the config the same way we wrap the one set by chsh. Then I got distracted by some other work.

The point was to run login shell with - as prefix. iterm2 for example has a separate binary for that iirc, but it effectively does the same thing as zsh wrapping we're doing.

And I don't think it wraps user specified programs, since users can properly start their shells.

chrisduerr commented 3 weeks ago

Most importantly login will automatically change to the home directory. So if you change your shell and your working directory config option doesn't work anymore, you can fix that yourself. If you don't care about that, then you don't have to care about it, it's just that Alacritty's default shell has to care about it.

nixpulvis commented 2 weeks ago

@chrisduerr should be good to go now I think.