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

Investigate preexec hook for bash/zsh #1294

Closed
jdx opened this issue Dec 29, 2023 · 12 comments · Fixed by #3373
Closed

Investigate preexec hook for bash/zsh #1294

jdx opened this issue Dec 29, 2023 · 12 comments · Fixed by #3373

Comments

@jdx
Copy link
Owner

jdx commented Dec 29, 2023

It might be possible to use rtx activate in scripts if I used preexec instead of precmd in zsh. It looks like this package provides preexec for bash too: https://github.com/rcaloras/bash-preexec

I think I might be able to get rid of the "not_found" handler with this too. rtx could perform the bin check before the command is executed.

Fish probably has a similar hook too

@jdx
Copy link
Owner Author

jdx commented Dec 29, 2023

It's unclear to me if preexec actually runs in non-interactive sessions or not, I'll need to experiment. If not, chpwd could work for zsh.

Unfortunately most people use bash for scripts so I'm not sure what solution could be used there.

@jdx
Copy link
Owner Author

jdx commented Dec 29, 2023

Possible the biggest benefit here would be not being able to run "scripts" but just for zshrc to be able to use tools. Of course zshrc is a "script" but it's a special kind since it prepares the interactive session and I think users would expect it to work.

@joshuataylor
Copy link
Contributor

I ran into this tonight, there is a slight difference to the way zsh and bash hooks seem to work currently with rtx.

2023.12.40 linux-x64 (460e547 2023-12-28)

zsh has much better support for chpwd, but bash is clunky.

Here's a good example when using bash:

Let's say we have a project at ~/foo/bar:

  1. Create a simple Python project with virtualenv, version of Python doesn't really matter:

.rtx.toml

[tools]
python = {version='3.12', virtualenv='.venv'}
  1. Change trace+debug.
export RTX_DEBUG=1
export RTX_LOG_LEVEL=trace

zsh

In zsh (5.9 for me, but should work with whatever), this works correctly:

  1. First, change to the directory so .venv etc is created.
cd ~/foo/bar
  1. Now, in a single line, cd to the parent directory, then delete the .venv directory, then cd back into the bar directory, then echo "hello".

In theory, this should first setup the virtualenv then echo hello.

cd ~/foo && rm -rf bar/.venv && cd bar && echo "hello"

Output (correct):

[DEBUG] ARGS: rtx hook-env -s zsh
[DEBUG] Config {
    Config Files: [
        "~/.config/rtx/config.toml",
    ],
    Installed Plugins: [
        "poetry",
    ],
}
[DEBUG] Toolset:
[DEBUG] ARGS: rtx hook-env -s zsh
[DEBUG] Config {
    Config Files: [
        "~/foo/bar/.rtx.toml",
        "~/.config/rtx/config.toml",
    ],
    Installed Plugins: [
        "poetry",
    ],
}
[DEBUG] Toolset: python@3.11
[INFO] rtx setting up virtualenv at: /home/josh/foo/bar/.venv
[DEBUG] $ ~/.local/share/rtx/installs/python/3.11/bin/python -m venv /home/josh/foo/bar/.venv
hello
[DEBUG] ARGS: rtx hook-env -s zsh
[DEBUG] Config {
    Config Files: [
        "~/foo/bar/.rtx.toml",
        "~/.config/rtx/config.toml",
    ],
    Installed Plugins: [
        "poetry",
    ],
}

bash

  1. But in bash, it seems this is never triggered:
hello
[DEBUG] ARGS: rtx hook-env -s bash
[DEBUG] Config {
    Config Files: [
        "~/foo/bar/.rtx.toml",
        "~/.config/rtx/config.toml",
    ],
    Installed Plugins: [
        "poetry",
    ],
}
[DEBUG] Toolset: python@3.11
[INFO] rtx setting up virtualenv at: /home/josh/foo/bar/.venv
[DEBUG] $ ~/.local/share/rtx/installs/python/3.11/bin/python -m venv /home/josh/foo/bar/.venv

@jdx
Copy link
Owner Author

jdx commented Dec 30, 2023

first, thanks for the writeup, it's helpful to get these kind of user stories. I wonder if chpwd only gets fired on a new prompt display? I would expect that line to work like you did. I'll have to do some experimentation here

https://github.com/jdx/rtx/blob/main/src/shell/zsh.rs#L49-L56

@joshuataylor
Copy link
Contributor

I'll have a play with it, Bash isn't my primary shell but agreed, looks like it only fires on prompt as bash doesn't have that hook.

IMHO, it's outside of the scope of rtx to fix if it's a bash issue, the current way is fine for most usecases.

I'll have more of a dig though.

@jdx
Copy link
Owner Author

jdx commented Nov 30, 2024

sounds like this was a valiant experiment but ultimately fruitless

@jdx jdx closed this as not planned Won't fix, can't repro, duplicate, stale Nov 30, 2024
@jdx
Copy link
Owner Author

jdx commented Nov 30, 2024

crap maybe I read it wrong, it sounds like it works in zsh but not bash? idk why I never considered this for zsh

@jdx jdx reopened this Nov 30, 2024
@joshuataylor
Copy link
Contributor

Yeah there is a noticeable delay with prompt rendering, it's because it's the only sane way to do it unless you're some kind of arcane wizard. I spent a while on this and couldn't get it to work reliably.

There are various things this would solve, as there are actions that could be fixed that can't be done with the current approach.

@jdx
Copy link
Owner Author

jdx commented Dec 1, 2024

looks like fish has this: fish-shell/fish-shell#583

I'm annoyed though. While this does fix issues and I get a lot of questions about hook-env in zshrc it creates a lot of new ones. I think I almost have to implement this but here are at least some of the issues I realized that will need to be addressed:

  • docs will need a lot of work. We need to clarify this and also clarify that zsh and fish work differently than other shells. The guidance around using shims in non-interactive environments will no longer apply but only using those shells. This requires changes in probably lots of places.
  • this code always causes problems when I modify it in unexpected ways. I am basically just expecting this to explode in a way that I won't be able to predict.
  • we can't remove the current behavior of running hook-env on prompt display because otherwise mise wouldn't notice changes to mise.toml if you don't change directory, that means we need both. However if we keep both in mise will run twice on cd, once for cd and once when the prompt is displayed. mise will be twice as slow in the most popular shell on cd.

It's the last one that makes me super concerned. This means I probably need to track some state or something to know if it ran on cd and if so then don't run on prompt display the prompt once. This means a lot more logic will need to be added. State management often has bugs. zsh code often has bugs. This is going to be buggy.

I think what I should do is gate this, not even behind MISE_EXPERIMENTAL but behind its own setting so intrepid people can iron the issues out with me. I suspect this will be a long road but needs to happen.

@joshuataylor
Copy link
Contributor

Agreed around how very tricky/unexpected to change because direnv does it the same way with prompts. I feel like mise works exceptionally well and this sort of change is kinda big.

@jdx
Copy link
Owner Author

jdx commented Dec 5, 2024

well that's interesting, reading the code it appears we're already doing this:

mise/src/shell/zsh.rs

Lines 52 to 54 in 2693f94

if [[ -z "${{chpwd_functions[(r)_mise_hook]+1}}" ]]; then
chpwd_functions=( _mise_hook ${{chpwd_functions[@]}} )
fi

@jdx
Copy link
Owner Author

jdx commented Dec 5, 2024

given we're already doing this, I'm inclined to punt on the performance issue for now since that's the hard part. We're already paying this cost, optimizing it is a separate issue.

I think if I made one simple change to automatically call hook-env it would mean I could greatly simplify the guidance around shims. You could just put mise activate right into your login shell and all the talk about non-interactive/interactive sessions would no longer apply, mise would run on cd and when the prompt is displayed so in theory nobody would need shims. I don't think I would remove shims since there are probably some esoteric edge-cases where they'd be useful.

We would need to address bash, but I'm thinking we do what rvm does and wrap cd/popd with this library. That's a bit scary, but given how old rvm is and that I don't see any issues related to them doing this it's probably fine for us too.

I'm not really sure what it would mean for other shells. I think nushell is already monitoring PWD and I gotta think any modern shell is going to have a way of doing that even if we're not doing it today for some reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants