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

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

Merged
merged 3 commits into from May 15, 2024

Conversation

nixpulvis
Copy link
Contributor

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

Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nixpulvis
Copy link
Contributor Author

nixpulvis commented May 6, 2024

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
Copy link

Learath2 commented May 6, 2024

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
Copy link
Contributor Author

nixpulvis commented May 6, 2024

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
Copy link
Member

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
Copy link
Member

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.

Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the only thing that gets sourced now is /etc/zsh/zshenv and I didn't see an easy solution to fix that. Should be fine as-is, probably isn't going to make things worse at least.

Could you add a changelog entry documenting these changes?

@nixpulvis
Copy link
Contributor Author

@chrisduerr should be good to go now I think.

@chrisduerr chrisduerr merged commit 3cd35df into alacritty:master May 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

ZSH pollutes environment on macOS even with $SHELL set to bash
4 participants