asdf-community / asdf-direnv

direnv plugin for the asdf version manager
https://github.com/asdf-vm/asdf
Apache License 2.0
559 stars 38 forks source link

bug: cached env file gets created when there are errors #156

Closed jfly closed 2 years ago

jfly commented 2 years ago

To reproduce:

$ asdf direnv local python 3.8.10

Switch to a version of python we don't have installed:

$ echo "python 3.8.11" > .tool-versions
direnv: loading ~/tmp/demo/.envrc
direnv: using asdf
direnv: Creating env file /home/jeremy/.cache/asdf-direnv/env/877384399-54258750-301089837-2116979671
direnv: python 3.8.11 not installed. Run 'asdf install' and then 'direnv reload'.
direnv: loading ~/.cache/asdf-direnv/env/877384399-54258750-301089837-2116979671
direnv: using asdf python 3.8.11

Now cd out of the directory and back in. Note how there are no errors:

$ cd ..; cd -
direnv: unloading
~/tmp/demo
direnv: loading ~/tmp/demo/.envrc
direnv: using asdf
direnv: loading ~/.cache/asdf-direnv/env/877384399-54258750-301089837-2116979671
direnv: using asdf python 3.8.11

No errors, even though I don't actually have python 3.8.11 installed! Now, it doesn't even matter if I even if I install python 3.8.11, I've got a cached env file that is useless:

$ cat ~/.cache/asdf-direnv/env/877384399-54258750-301089837-2116979671
### Do not edit. This was autogenerated by 'asdf direnv envrc' from /home/jeremy/tmp/demo/.tool-versions ###
log_status using asdf python 3.8.11

This is a regression, we should not even generate a cached env file in these situations. (We worked carefully to get this working in https://github.com/asdf-community/asdf-direnv/pull/88)

I believe this broke with 6610b47515ef011d08f996fdf90280f7980fa4b7, which changed use asdf from calling $(asdf direnv _asdf_cached_envrc "$@") to calling $(_asdf_cached_envrc "$@"). This change feels like it should be a no-op, but it has a really important, subtle difference because asdf direnv ... is an actual subprocess invocation, whereas the _asdf_cached_envrc is a function call in our current bash script, which behaves differently with set -e. This SO question talks about this. My bash-fu is not very strong, so I'm not too sure what the best fix is.

Given that we've broken this once, it also seems like a good idea to add a unittest to prevent further regressions.