Re: tramp (2.6.1 HEAD/3ff676c2f98cb6c47fecb37f31a589a910dd3876); docker-container ssh multi-hop support

[Michael Albinus]

Gene Goykhman <gene@indigo1.com> writes:

Hi Gene & Kristofer,

Sorry for the delay, but bug fixing has priority ...

> I've made some progress and have published a gist with my current
> approach to multi-hop Docker container completion in TRAMP. I'd
> appreciate comments and suggestions, and whether this is a reasonable
> direction to take or if I'm missing something important.

As said already, I like the idea. It has potential to be used for other
Tramp methods as well, so it might be added to Tramp proper.

However, this would require to assign the copyright of the code to the
FSF, as all non-trivial contributions to Emacs require. Would you be
willing to sign this?

>> A very simple change I tried when previously trying to get multihop
>> completion working
>> with another backend was that if I changed the variable
>> `tramp-compat-temporary-file-directory'
>> for the function `temporary-file-directory' in the completion
>> functions (like `tramp-container--completion-function'), then at least
>
> I'm advising :before tramp-completion-handle-file-name-all-completions
> and parsing out the required remote directory from which we need to
> call the docker program to get remote completions. Then, I'm adding a
> custom completion function my-tramp-container-completion-function that
> takes that directory (if it has been parsed) and sets it as the
> default-directory.

I guess we don't need to advice 
tramp-completion-handle-file-name-all-completions.
The code should be added directly there.

>> Another issue that I found, which maybe is unavoidable, is that some
>> of the completion functions in `tramp-container' requires that the
>> container
>> program is installed on the local host, but sometimes I only have
>
> I /think/ the approach I've taken only requires that the container program be 
> available on the remote host on which we are trying to get the completion 
> list.

Yep. On every hop of a multi-hop file name only the program for that hop
must be available. That is, for "/ssh:host|docker:container:" we need
ssh on the local host, and docker on the host "host".

> ;; Disable TRAMP caching so we don't see unavailable containers
> (setq tramp-completion-use-cache nil)

Well, you disable this for all completions. I don't know whether we want
this. It is useful for the very first hop, and not only for the docker
method, but for all methods.

However, we might need a marker in the cache, whether the entry belongs
to the very first hop, or not. This we can implement later.

>   (when-let ((default-directory (if tramp-last-hop-directory
>                                     tramp-last-hop-directory
>                                   tramp-compat-temporary-file-directory))

I would write (or tramp-last-hop-directory 
tramp-compat-temporary-file-directory)

> (add-to-list 'tramp-completion-function-alist
>              `("docker"
>                (tramp-container--completion-function ,tramp-docker-program)
>                (my-tramp-container-completion-function 
> ,tramp-docker-program)))

So you have two entries. Do you still need tramp-container--completion-function?

> (defun my-tramp-docker-host-path (orig)
>   ;; Strips off "|docker" suffix from ORIG allowing
>   ;; it to be converted to a tramp-file using TRAMP's path functions.
>   (let ((regexp "\\(.*\\)|docker"))
>     (if (string-match regexp orig)
>         (concat (match-string 1 orig) ":/")
>       orig)))

Well, this is docker related. The proper Tramp functions would be

(tramp-make-tramp-file-name (tramp-dissect-hop-name (tramp-file-name-hop 
(tramp-dissect-file-name FILENAME))))

Hmm, perhaps this is worth an own function. Or perhaps we have it
already, and I don't remember. We have a similar logic in computing the
previous hop in function tramp-compute-multi-hops, but not in the same
sense as we need it here.

And this also does not handle the case of more than two hops, like in 
"/ssh:host1|ssh:host2|docker:container::"

> ;; When TRAMP handles completions, first set tramp-last-hop-directory
> ;; so that my completion function can set it before running the Docker 
> executable
>
> (defun my-tramp-set-docker-default-directory (filename directory)
>   (setq tramp-last-hop-directory nil)
>   (let ((my-host-path (my-tramp-docker-host-path directory)))
>     (when (tramp-tramp-file-p my-host-path)
>       (setq tramp-last-hop-directory (tramp-make-tramp-file-name
>                                       (tramp-dissect-file-name 
> my-host-path))))))
>
> (advice-add #'tramp-completion-handle-file-name-all-completions :before 
> #'my-tramp-set-docker-default-directory)

As said, I propose to integrate it in 
tramp-completion-handle-file-name-all-completions.
And I don't understand, why you call (tramp-make-tramp-file-name 
(tramp-dissect-file-name ...))
my-host-path is already a proper file name, isn't it?

Best regards, Michael.