purcell / envrc

Emacs support for direnv which operates buffer-locally
378 stars 35 forks source link

Overriding exec-path changes from other hooks #19

Closed davazp closed 3 years ago

davazp commented 3 years ago

This sounds familiar to some of the other issues reported.

In summary, envrc will override the changes that hooks might do to the exec-path.

In my case, I do use also add-node-modules-path, which will add some elements to the exec-path. But this seemed not to have any effect if envrc is enabled.

Looking at the code, envrc is very careful to merge emacs PATH and the value read from the .envrc file. But it is looking at the PATH environment variable, not the exec-path. So the exec-path is effectively reset.

https://github.com/purcell/envrc/blob/master/envrc.el#L288-L289

Should the merged environment consider exec-path too, or would it break things?

purcell commented 3 years ago

I can't find the other issue or PR where I discussed this with a contributor, but essentially, if your exec-path and $PATH are out of sync inside Emacs, then something is going to break or not work exactly. As an example, create-process might work, but shell-command might not. If the two values disagree, there's no good strategy for envrc.el to decide how to choose between them or merge them.

In this regard, then, add-node-modules-path is doing the wrong thing. I used to use add-node-modules-path myself, but it's completely redundant if you're using direnv. As you noticed, I have done a little work to try to ensure that envrc.el can work with other path-altering packages, but it's not a high priority.

davazp commented 3 years ago

That makes sense. I didn't think of how having both values out of sync could break things, when the executable tries to run other executables then.

In this particular case it would be fine. I would prefer not to extend the path in direnv, as my primary intention was just to let format-all-code find the executable.

I can workaround it though. Thanks!

zeorin commented 5 months ago

Oof, I just ran into this issue. I'm a Nix/NixOS user and I don't like installing binaries globally if I can avoid it. Of course, the Emacs packages I use sometimes require that certain binaries are installed. I have provided them to Emacs by wrapping Emacs using wrapProgram and prepending the various derivations binary paths to the PATH environment variable: https://github.com/zeorin/dotfiles/blob/ca602de8c6312c53f51fadd4cb97593fa3961271/home-manager/home.nix#L89-L101.

This works, and it's not caused any problems with your package. However, one thing that's always annoyed me about this approach is that when I then open a shell in Emacs, that shell inherits this Emacs-specific PATH, which means that this shell now has access to binaries that are only intended for Emacs, and not the shell environment. My Emacs shell environment is thus different from a shell environment opened in a normal terminal emulator. I would like for them to be the same.

Today I decided to try and find out whether there's a way to tell Emacs where to look for executables without inadvertently adding those to the PATH. I found this: http://xahlee.info/emacs/emacs/emacs_env_var_paths.html:

Difference between exec-path and PATH

The value of environment variable “PATH” is used by emacs when you are trying to call a linux command from a shell in emacs. The exec-path is used by emacs itself to find programs it needs for its features, such as spell checking, file compression, compiling, grep, diff, etc.

The value of (getenv "PATH") and exec-path do not need to be the same.

So instead of adding those paths to PATH I tried adding them to exec-path instead. This caused some things to no longer work correctly, and I eventually managed to follow the breadcrumbs here.

if your exec-path and $PATH are out of sync inside Emacs, then something is going to break or not work exactly

This confuses me somewhat. Seems to me that they serve similar (overlapping even, perhaps) but not exactly the same purposes, otherwise why would Emacs:

  1. have 2 different configuration options
  2. not just sync them automatically by force (this would make sense if e.g. exec-path existed before PATH did or something, way back in the sands of time), i.e. one be an alias for the other?

I think the correct behaviour here might be to apply the same changes that direnv applies to the PATH to exec-path, e.g. if direnv prepends /opt/foo/bin to the PATH, then IMO it makes sense to prepend /opt/foo/bin to exec-path, instead of clobbering exec-path with the new value of PATH.

For users that do keep PATH and exec-path in sync, this has the same result. But for users that intentionally want to take advantage of the nuance between PATH and exec-path, this is more considerate.

This reminds me a little of a similar issue I once had with nvm: https://github.com/nvm-sh/nvm/pull/1316

purcell commented 5 months ago

@zeorin are you looking for exec-path-from-shell perhaps?

zeorin commented 5 months ago

@purcell No, in my case I'm intentionally trying not to completely sync exec-path with PATH. What I was expecting is that envrc would apply the same changes to exec-path that were applied to PATH. E.g. like this:

      (let* ((prev-path (parse-colon-path (cdr (assoc "PATH" (cdr (assoc "p" result))))))
             (next-path (parse-colon-path (cdr (assoc "PATH" (cdr (assoc "n" result))))))
             (path-removed (cl-set-difference prev-path next-path))
             (path-added (cl-set-difference next-path prev-path)))
        (setq-local exec-path (append path-added (cl-set-difference (default-value 'exec-path) path-removed)))
purcell commented 5 months ago

Ah, that seems like an ~unusual~ unexpected use case. Generally I would expect exec-path and PATH to match up, otherwise things can get confusing.

I think the correct behaviour here might be to apply the same changes that direnv applies to the PATH to exec-path

I looked at your code, thanks! Isn't this equivalent to keeping exec-path entries that weren't mentioned in the original $PATH? ie. the new exec-path becomes set(new_PATH) + (set(exec-path) - set(original_PATH)). This way you don't need to track added/removed.

purcell commented 5 months ago

I would definitely consider merging a corresponding change. I wonder if we can assume that entries unique to exec-path entries should go at the end of that var, though: perhaps in some cases they should be preserved at the beginning.

zeorin commented 5 months ago

Isn't this equivalent to keeping exec-path entries that weren't mentioned in the original $PATH? ie. the new exec-path becomes set(new_PATH) + (set(exec-path) - set(original_PATH))

That could possibly re-order some things in rare cases.

I wonder if we can assume that entries unique to exec-path entries should go at the end of that var, though: perhaps in some cases they should be preserved at the beginning.

My implementation is a bit naïve, as there's no guarantee that the additions should go to the front, though I imagine that's what ~100% of the time is the intention. Ideally the changes should be merged like a patch apply (each entry in the PATH being a "line"), and when it's unclear where to insert an entry, fall back to prepending it. But my Emacs-fu is not quite up to the task. The code I wrote in my fork is my most significant Emacs Lisp effort to date. :smile:

EDIT:

and when it's unclear where to insert an entry, fall back to prepending it

More specifically, if there's a "conflict" for adding some "lines", accept both changes, but put the changes coming from envrc's changes to PATH in front of the differences the user has in their exec-path and PATH. "Conflicts" where both are removing something should just remove both.

If the user needs even more project-specific changes to exec-path beyond the changes they made to PATH with direnv, I reckon that's out of scope and eval/.dir-locals.el might allow them to do it (e.g. if they advise some envrc functions).

zeorin commented 5 months ago

I wonder if we can assume that entries unique to exec-path entries should go at the end of that var, though: perhaps in some cases they should be preserved at the beginning.

Thinking about this a little more, I would not assume this. Imagine the following scenario:

  1. exec-path is initialized by exec-path-from-shell
  2. after this the user prepends paths to exec-path, because for some reason they have a need for customized versions of some executables to be used by Emacs (e.g. ripgrep used by DOOM needs to be compiled with PCRE2 support).
  3. envrc is run, it moves the entries unique to exec-path to the end
  4. the result is that the customised executables are now being shadowed

This is why I took my approach of figuring out the differences instead. This preserves more of the order than the above approach, though as I mentioned, it's still a little naïve.

I know this all seems like splitting hairs, but:

(90% of Emacs users have unusual setups and the remaining 10% are really unusual) – https://www.gnu.org/software/emacs/manual/html_node/auth/Help-for-users.html

purcell commented 5 months ago

Nice. Well I'll see if I can find a little time to implement something similar.

(90% of Emacs users have unusual setups and the remaining 10% are really unusual)

Haha, so true!