purcell / envrc

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

Add TRAMP support #29

Closed siddharthverma314 closed 5 days ago

siddharthverma314 commented 2 years ago

Hi,

This PR adds tramp support and closes #27 . It has only been tested by me, so I would appreciate it if others could try it out and see if it works for them!

purcell commented 2 years ago

I tried this out by adding a .envrc containing export FOO=bar on a remote host, then connecting with dired to the directory via an /ssh: TRAMP path. The *envrc* buffer showed the var was parsed, but I don't see any effect on tramp-remote-process-environment, or the expected output when running M-! echo $FOO. What did you try in order to test this out?

siddharthverma314 commented 2 years ago

Hey, thanks for your feedback! In my initial implementation, I wrongly assumed that the variables must be set in the tramp connection buffer. I've updated my PR to reflect this. Most normal operations should now work. I also fixed all of your suggestions.

However, it seems like shell programs still don't work. Using the test you suggested, tramp-remote-process-environment is updated, but M-! echo $FOO does not output anything, and eshell does not have the correct environment variable set. I'm currently working on resolving this.

siddharthverma314 commented 2 years ago

It seems like tramp-remote-process-environment must be set on the connection buffer in order for (exec-path) to work. I've updated the PR to set the variables on both the connection buffer and local buffer, although ideally the code should use connection-local-variables to update the environment for all tramp-related buffers. Changing the PR to a draft as it needs more work!

purcell commented 2 years ago

Thanks for this, I haven't worked much with the tramp machinery, so I appreciate you tackling the issue.

andir commented 1 year ago

Sorry for bumping this almost a year old conversation but as a recent migrant into Emacs I would love this feature :-)

@siddharthverma314 did you continue working the version posted here? Is there something I can do to help testing this?

purcell commented 1 year ago

Hey Andi, yeah, I was wondering about this recently too.

siddharthverma314 commented 1 year ago

Thanks for your interest! Back when I was writing this, the tramp implementation (to my knowledge) did not support buffer local tramp-remote-process-environment, making it impossible to implement an envrc-style approach. This might have changed recently -- I'll need to look into it more. Meanwhile, I have a PR open on emacs-direnv for TRAMP support that should work.

d-miketa commented 1 year ago

Hi, just wanted to say I'd love this feature too! :) Happy to help test it.

sellout commented 1 year ago

@siddharthverma314 I haven’t done extensive testing yet, but tramp-remote-process-environment can be set buffer-locally (which I’m guessing you know). However, given your comment:

It seems like tramp-remote-process-environment must be set on the connection buffer in order for (exec-path) to work.

if that’s still the case, then buffer-local doesn’t help, because there’s only one relevant buffer per connection.

I had another thought that might help – do what you’re currently doing, but add advice to the various TRAMP functions like

(advice-add 'some-tramp-function
            :before
            (lambda (orig-fn &rest args)
              ;; get the possibly buffer-local variables from the current (non-connection) buffer
              (let ((local-process-env tramp-remote-process-env)
                    (local-path tramp-remote-path))
                (with-current-buffer (tramp-get-connection-buffer default-directory)
                  ;; assign the non-connection buffer values to the connection buffer
                  (setq-local tramp-remote-process-env local-process-env
                              tramp-remote-path local-path)))))

which would hopefully then use the appropriate environment for each command (and buffers that don’t have buffer-local values for these vars would just end up setting the connection buffer’s values to to defaults. (There are coarser variants, like updating the connection-buffer’s variables whenever the selected window changes, like via window-state-change-hook, but that could maybe lead to race conditions?)

dcunited001 commented 1 year ago

:+1: if possible, that would be awesome

bbigras commented 1 year ago

@andir did you find a workaround to use tramp with a remote nix shell?

siddharthverma314 commented 1 year ago

Hey everyone, I've been working on this locally and I think I have an implementation that works ~20% of the time 🤣. I'd love to gather feedback about whether it works or not, but try at your own risk. Or if you find bugs, let me know and I can fix them.

Also, thank you @sellout for the inspiration, I ended up implementing an advice around tramp-get-connection-buffer and tramp-get-remote-path.

Note that in order to enable direnv over tramp, you will need to set envrc-remote to t. This is the config I'm using locally:

(use-package envrc
  :straight (envrc :host github :repo "purcell/envrc"
                   :fork (:host github :repo "siddharthverma314/envrc"))
  :config
  (setq envrc-remote t)
  (envrc-global-mode))

Thanks for your patience!

sellout commented 1 year ago

Thanks, @siddharthverma314! I should be able to give this a go tomorrow.

purcell commented 1 year ago

Nice! In what way does it fail to work the rest of the time?

hso commented 1 year ago

Thanks, @siddharthverma314. I've been looking for something like this for some time. I can't get it working yet, though. When I use your sample config I get envrc/:config: Symbol's value as variable is void: vec when I try to load a file over TRAMP or if I manually call envrc-global-mode. What could I be missing?

siddharthverma314 commented 1 year ago

@hso Thanks for the feedback! I'm assuming you tried the obvious, like restarting emacs, running M-x straight-pull-package and reloading envrc.el. Can you provide the following info:

  1. Which version of tramp are you using? For me, M-x tramp-version outputs 2.7.0-pre, so maybe there is minimum tramp version that is required.
  2. Could you provide a stack trace for when this happens? Easy way of doing that is to M-x toggle-debug-on-error and try to enable envrc-global-mode. I'm mainly curious whether this error happens within envrc-global-mode or envrc-get-remote-path.
siddharthverma314 commented 1 year ago

@purcell In my testing, things got slow sometimes or the remote path got messed up, but these might have been problems with earlier versions of the code. For now it seems to run fine, but I definitely haven't tested all cases yet.

mrunhap commented 1 year ago

Is this ready to merge?

purcell commented 12 months ago

Symbol's value as variable is void: vec when I try to load a file over TRAMP or if I manually call envrc-global-mode. What could I be missing?

Perhaps it's the with-parsed-tramp-file-name call — there's no (eval-when-compile (require 'tramp)) present in the changes, so if that macro is unknown, Emacs would produce an error message like that.

Is this ready to merge?

Not yet. I want to understand how it works and what the trade-offs are, and I haven't had the time for a deep dive. For example, the changes apparently rely on copying buffer-local environments into the connection buffer "just in time" for commands to be executed there, so I worry about collisions or unexpected lingering effects. e.g. If you have multiple envrc-mode-enabled buffers on a tramp connection, and some buffers on that same connection that do not have envrc-mode activated, those latter buffers would presumably see the environment left by the last command run in one of the former buffers.

But I think this is promising.

hso commented 11 months ago

@siddharthverma314 yes, I tried restarting emacs, running straight-pull-package and reloading envrc.el, but I still get that error.

1. Which version of tramp are you using? For me, `M-x tramp-version` outputs `2.7.0-pre`, so maybe there is minimum tramp version that is required.

I don't have that command available, I suppose I have never installed a package for tramp, I'm just using the one that comes with my emacs version: GNU Emacs 29.1 (build 2, x86_64-suse-linux-gnu, GTK+ Version 3.24.38, cairo version 1.17.8)

2. Could you provide a stack trace for when this happens? Easy way of doing that is to `M-x toggle-debug-on-error` and try to enable `envrc-global-mode`. I'm mainly curious whether this error happens within `envrc-global-mode` or `envrc-get-remote-path`.

Sure:

Debugger entered--Lisp error: (void-variable vec)
  envrc-global-mode(toggle)
  funcall-interactively(envrc-global-mode toggle)
  #<subr call-interactively>(envrc-global-mode record nil)
  call-interactively@ido-cr+-record-current-command(#<subr call-interactively> envrc-global-mode record nil)
  apply(call-interactively@ido-cr+-record-current-command #<subr call-interactively> (envrc-global-mode record nil))
  call-interactively(envrc-global-mode record nil)
  command-execute(envrc-global-mode record)
  execute-extended-command(nil "envrc-global-mode" nil)
  funcall-interactively(execute-extended-command nil "envrc-global-mode" nil)
  #<subr call-interactively>(execute-extended-command nil nil)
  call-interactively@ido-cr+-record-current-command(#<subr call-interactively> execute-extended-command nil nil)
  apply(call-interactively@ido-cr+-record-current-command #<subr call-interactively> (execute-extended-command nil nil))
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)
bbigras commented 7 months ago

Any progress on this? or maybe a workaround.

bbigras commented 6 months ago

with 5c1d1eb64de927785e9fefa3c6fdbf023d7cfc4d, I got env: ‘direnv’: No such file or directory.

I tried with /ssh: and /sshx: with (setq envrc-remote t).

sgrb commented 1 month ago

Is there any progress on merging this PR? At least for me it works well with remote direnv + nix.

purcell commented 1 month ago

Is there any progress on merging this PR?

Unsure of the current status, and I don't use the changes myself. The merge conflict looks trivial, but @bbigras reported issues above, maybe due to not having direnv installed on the target machine?

sgrb commented 5 days ago

@bbigras reported issues above, maybe due to not having direnv installed on the target machine?

It looks plausible (or maybe PATH issues on the remote end). At least I use envrc with this PR applied almost every day (both locally and remotely over TRAMP), and didn't have any issues so far.

sellout commented 5 days ago

I use envrc with this PR applied almost every day (both locally and remotely over TRAMP), and didn't have any issues so far.

Same. In fact, I recently switched to my own fork to test out some unrelated changes, and was immediately affected by the loss of TRAMP support 😆

purcell commented 5 days ago

I've resolved this conflict and merged this PR into the main branch. Couple of questions for you all though:

  1. Do you find that shell-command-to-string etc. work okay from remote buffers that use ? I'm wondering if changes to inheritenv might be necessary too.
  2. Any reason to keep the envrc-remote custom var added by this PR? I feel like tramp support should just always be enabled, or am I missing something?
siddharthverma314 commented 5 days ago

Hey! Thanks for merging this PR. Regarding the concerns you raised above:

  1. inheritenv changes may be necessary because I was experiencing odd behavior with magit and couldn't track down why. I think this PR should do it.
  2. We should keep envrc-remote disabled by default because the functionality does not have test coverage 😄. Once I figure out how to do that, it can be removed. As a side note, I have no clue if this works with connection local variables.

Sorry for the delay in pushing this through.

purcell commented 5 days ago

We should keep envrc-remote disabled by default because the functionality does not have test coverage 😄. Once I figure out how to do that, it can be removed. As a side note, I have no clue if this works with connection local variables.

Even the existing tests have been a bit broken for a while tbh. :( I'd be inclined to remove the var anyway if that's the only technical reason for it — worst case would be that we get a few helpful bug reports.

purcell commented 5 days ago

I'll close this PR to avoid confusion, but feel free to comment further, or open a follow-up issue or PR.