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

Why is direnv messing with existing variables? #82

Closed
pdf opened this issue Dec 10, 2013 · 12 comments
Closed

Why is direnv messing with existing variables? #82

pdf opened this issue Dec 10, 2013 · 12 comments
Labels

Comments

@pdf
Copy link

pdf commented Dec 10, 2013

I have a .envrc:

PATH_add bin

So, I would expect direnv to modify nothing but PATH when loading that file. Instead I get:

direnv: loading .envrc
direnv export: -COMP_WORDBREAKS ~PATH

Why is it deleting COMP_WORDBREAKS?

And worse, when I'm in tmux:

direnv: reloading
direnv: loading .envrc
direnv export: -COMP_WORDBREAKS -TMUX -TMUX_COLUMNS_0 -TMUX_COLUMNS_1 -TMUX_COLUMNS_2 -TMUX_COLUMNS_3 -TMUX_PANE -TMUX_PWD_0 -TMUX_PWD_1 -TMUX_PWD_2 -TMUX_PWD_3 -rvm_delete_flag -rvm_sticky_flag ~GEM_HOME ~GEM_PATH ~PATH ~TERM ~rvm_env_string

Now it's deleting all the TMUX vars (I need to know if I'm inside TMUX!) and worse still, it's overwriting my TERM, which breaks drawing via ncurses, amongst other nastiness.

I don't understand what's going on here - why is direnv messing with anything that's not in the .envrc?

@zimbatm
Copy link
Member

zimbatm commented Dec 10, 2013

Hi @pdf, thanks for reporting.

It's two different issues that issue from the same direnv mechanism. direnv serializes the old environment in the DIRENV_BACKUP environment variable, runs a bash sub-shell to load the .envrc and exports the diff between the old and new environment in the shell. Unloading the environment just means restoring the content that's in the DIRENV_BACKUP.

In the first case it's an unidentified issue with bash that I also saw on #81. In some condition bash seems to be changing the COMP_WORDBREAKS variable in the subshell. It shouldn't because the subshell bash is executed with --noprofile and --norc. I don't know why it's doing it but I have added COMP_WORDBREAKS in the blacklist of ignored environment variables since it's causing segfaults on some versions of bash when unset (see 403c34e ). Next release it's going to be fixed.

I believe the second case only happens when you start tmux from withing a loaded .envrc environment. The backup has already happened before tmux is started. The tmux+bash subshell is now loaded with both the old DIRENV_BACKUP and the new TMUX_* variables introduced by tmux. When you cd .. outside of your project inside tmux it will restore the DIRENV_BACKUP environment and remove the TMUX_* variables who are not part of it.
I don't know what's the best way to deal with that yet ; either a tmux wrapper that unload the environment before executing tmux, or add the TMUX_ variables to the blacklist.

@zimbatm
Copy link
Member

zimbatm commented Dec 10, 2013

One solution for tmux would be to use a wrapper.

Add this script somewhere in your PATH under direnv-tmux:

#!/usr/bin/env bash
pwd=$PWD
cd /
eval "$(direnv export bash)"
cd $pwd
exec tmux "$@"

chmod +x direnv-tmux

Then alias tmux=direnv-tmux

@pdf
Copy link
Author

pdf commented Dec 11, 2013

Thanks @zimbatm. I believe you're right about the problem only occurring when starting tmux from within an existing .envrc environment. The workaround is essentially working, but tmux likes to call itself a lot, so direnv does a lot of reloading with the alias in place.

I may be missing some understanding here, but why is direnv doing anything with variables that it's not being told to modify? That just seems wrong to me.

@pwaller
Copy link
Member

pwaller commented Dec 11, 2013

@pdf the problem is that since we use bash to run the .envrc files we have no way of knowing what has been explicitly modified. The best we can do is to invoke a "fresh bash" and then contaminate it with the .envrc, then to look at the difference between environments.

@pdf
Copy link
Author

pdf commented Dec 12, 2013

@pwaller I'm not entirely convinced that's the best that can be done. An example implementation that would behave correctly might be to parse the .envrc and perform individual actions that could be tracked inside direnv. There may be other ways of tracking stuff, too.

With that aside, why is DIRENV_BACKUP exported? Why do we ever want that in subshells? This particular issue (and I suspect others of similar provenance) wouldn't occur if it wasn't exported.

@pwaller
Copy link
Member

pwaller commented Dec 12, 2013

@pdf, I didn't mean to imply it was the best one could theoretically achieve, however, given the constraints on the project and the fact we currently use bash in the way it is used I would say it is (probably) unreasonable to implement, hook or otherwise hack a shell interpreter to tell us whenever an environment variable is set.

That's a good thought about DIRENV_BACKUP. However, if you started a subshell in $PWD with some environment already exported and then move to $PWD/.., what should happen?

@zimbatm
Copy link
Member

zimbatm commented Dec 12, 2013

The reason why direnv works like that is because it was simple to implement
and this issue was never raised before. I'm going to make a branch where
only the changing variables are backed-up and see if I like it better like
that.

@pdf if DIRENV_BACKUP is unset it's not possible to revert the env changes.
That might lead to having inconsistent states.
On Dec 12, 2013 4:26 AM, "Peter Fern" notifications@github.com wrote:

@pwaller https://github.com/pwaller I'm not entirely convinced that's
the best that can be done. An example implementation that would behave
correctly might be to parse the .envrc and perform individual actions that
could be tracked inside direnv. There may be other ways of tracking stuff,
too.

With that aside, why is DIRENV_BACKUP exported? Why do we ever want that
in subshells? This particular issue (and I suspect others of similar
provenance) wouldn't occur if it wasn't exported.


Reply to this email directly or view it on GitHubhttps://github.com/zimbatm/direnv/issues/82#issuecomment-30388601
.

@pdf
Copy link
Author

pdf commented Dec 12, 2013

@pdf if DIRENV_BACKUP is unset it's not possible to revert the env changes.
That might lead to having inconsistent states.

I wasn't suggesting unsetting it, just not exporting it... however, I see below why that is not possible.

That's a good thought about DIRENV_BACKUP. However, if you started a subshell in $PWD with some environment already exported and then move to $PWD/.., what should happen?

That is indeed problematic. I've tried having a quick browse of the code, but I'm struggling to put all the pieces together on how this works.

Perhaps this can be attacked from another angle - what is it about tmux that triggers an unload, even though DIRENV_DIR hasn't changed?

@zimbatm
Copy link
Member

zimbatm commented Dec 24, 2013

I'm going to change direnv to only track and restore variables that have been changed in the .envrc. That should fix the issue with tmux (unless you set a TMUX_ variable in the .envrc).

zimbatm added a commit that referenced this issue Dec 24, 2013
This commit changes how direnv works and the terminal needs to be restarted.
Previously any changed environment would be reverted when exiting a folder
that contains an .envrc. This changes the behavior to only restore variables
that where changed inside the .envrc.
@zimbatm
Copy link
Member

zimbatm commented Dec 24, 2013

^ this commit should fix the TMUX issue. All tests are passing but since it changes how direnv works you need to restart your shell just to be sure. @pdf if you can test it it would be great. I'm going to merge it in a week or so.

zimbatm added a commit that referenced this issue Dec 24, 2013
This commit changes how direnv works and the terminal needs to be restarted.
Previously any changed environment would be reverted when exiting a folder
that contains an .envrc. This changes the behavior to only restore variables
that where changed inside the .envrc.
@pdf
Copy link
Author

pdf commented Dec 29, 2013

Sorry for the delay in testing - this time of year is hectic. This fix looks great, thanks!

@zimbatm
Copy link
Member

zimbatm commented Jan 11, 2014

Ok merged into master, direnv release following soon.

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

No branches or pull requests

3 participants