kwrooijen / cargo.el

Emacs Minor Mode for Cargo, Rust's Package Manager.
GNU General Public License v3.0
170 stars 67 forks source link

Can't run cargo commands on TRAMP buffers. #113

Closed bjc closed 3 years ago

bjc commented 3 years ago

Overview

I have a rust install on a remote server, and I'm using TRAMP to do rust development there. Unfortunately, it looks like cargo-minor-mode assumes files are always local, so it breaks when trying to issue commands like cargo-process-run:

  signal(error ("No such directory found via CDPATH environment var..."))
  error("No such directory found via CDPATH environment var...")
  cd("/home/bjc/src/std-async")
  compilation-start("/home/bjc/.cargo/bin/cargo run --manifest-path /ns..." cargo-process-mode #f(compiled-function (_) #<bytecode -0x1042492df03d0bd0>))
  cargo-process--start("Run" "run")
  cargo-process-run()
  funcall-interactively(cargo-process-run)
  call-interactively(cargo-process-run nil nil)
  command-execute(cargo-process-run)

To reproduce

  1. have a rust/cargo install on a remote server
  2. edit a remote rust file via TRAMP on a local emacs (e.g., C-x C-f /ssh:user@remote:src/rust-test/src/main.rs)
  3. call M-x cargo-process-run

Suggested fix

cargo-minor-mode should become TRAMP-aware, as TRAMP is a major feature of Emacs that is widely used.

My environment

emacs 28.0.50 cargo version 20210124.1516 from melpa (git SHA e66beb9b2f1f8dce48aa31f42cb5c384328554c6)

bjc commented 3 years ago

Looking into this further, the problem appears to be here: https://github.com/kwrooijen/cargo.el/blob/master/cargo-process.el#L349

    (let ((default-directory (or (cargo-process--workspace-root)
                                 default-directory)))

default-directory is computed properly (/ssh:user@host:/some/path), but cargo-process--workspace-root is returning just the host-local portion (/some/path), and because it comes first in the or, that's what's being used by the call to compilation-start.

So I swapped the order of the arguments, which lead to an issue in manifest-path-argument, where it outputs the TRAMP version of a file for use by cargo itself (which obviously isn't TRAMP aware). The issue is here: https://github.com/kwrooijen/cargo.el/blob/master/cargo-process.el#L321

(defun manifest-path-argument (name)
  (let ((manifest-filename (cargo-process--project-root "Cargo.toml")))
    (when (and manifest-filename
               (not (member name cargo-process--no-manifest-commands)))
      (concat "--manifest-path " (shell-quote-argument manifest-filename)))))

I surrounded the call to cargo-process--project-root with a call to tramp-file-local-name:

(defun manifest-path-argument (name)
  (let ((manifest-filename (tramp-file-local-name (cargo-process--project-root "Cargo.toml"))))
    (when (and manifest-filename
               (not (member name cargo-process--no-manifest-commands)))
      (concat "--manifest-path " (shell-quote-argument manifest-filename)))))

And now it returns the system-local version of the file name, suitable for use with cargo itself.

After making both of these changes, I can now run cargo-minor-mode commands in TRAMP buffers.

kwrooijen commented 3 years ago

So I swapped the order of the arguments

You mean you changed it to? :

(let ((default-directory (or default-directory
                             (cargo-process--workspace-root))))

Doesn't that mean default-directory will never change, and therefore breaking the root workspace lookup?

bjc commented 3 years ago

That is the change I made. I figured it was not the right thing to do, but I wanted to test it to see if I could get cargo-minor-mode to work at all, and I wasn't sure what the rationale of the existing order was (besides it probably being important).

I'm not sure what you mean by "root workspace lookup". If you could explain why it's ordered the way it is in the current code, and how to test that its working properly, I'll try to fix it the right way. I just don't know enough about how things are supposed to work to attempt that right now.

kwrooijen commented 3 years ago

It's ordered this way because we only want to change the default-directory if the root can be found using cargo-process--workspace-root. If no result is found just use default-directory (Note that the binding is the same name as the value). I think the cargo-process--workspace-root function needs to be changed to take TRAMP into consideration.

bjc commented 3 years ago

I'm looking into doing it this way, but it's trickier, because cargo-process--workspace-root parses the path directly from cargo output, so I'm going to have to add the current buffer's TRAMP prefix to it, but I can't see anything in TRAMP right now that will give me that prefix easily.

I could just take the current file name from the buffer and remove what's returned from (tramp-file-local-name buffer-file-name) but that seems pretty cllumsy, so I'll keep looking for a better way.