Re: Review wanted for method for accessing Mock chroots

[Tim Landscheidt]

Michael Albinus <michael.albinus@gmx.de> wrote:

>> Is there anything blatantly wrong/not backward-compatible/
>> not foreward-compatible about it?  I saved on (customizable)
>> variables as it is hard to predict what other users might
>> want/need to change.  Any advice is appreciated.

> Some few comments below.

> However: I don't know Mock chroots in detail, but it looks to me like it
> is something what could live in tramp-container.el. What do you think a bout?

It probably fits quite nicely, but due to my preference for
sticking to distribution packages, I'd need an interim solu-
tion until Emacs 30 makes its way to my system. :-)

> Here are some few comments from roughly scanning the code:

>> ;;; mock-tramp.el --- TRAMP integration for Mock chroots  -*- 
>> lexical-binding: t; -*-

> Tramp is spelled out “Tramp”. See the manual.

Ha!  In "my" manual (2.5.4-pre) it is spelled "TRAMP" be-
cause 079625d3c618188fc76b89f8d942f0e7004d0312 was not com-
mitted yet. :-)

>> ;; Package-Requires: ((tramp "2.7.1-pre"))

> Why "2.7.1-pre"? This isn't a released version; I would depend on "2.7.1".

Because I copied the version string from the wrong file; I
changed it to "2.5.4-pre" (as that is what was and is
shipped on Fedora 38).

>> ;;;###autoload
>> (defcustom mock-tramp-method "mock"
>>   "TRAMP method to connect to Mock chroots."
>>   :type 'string
>>   :group 'mock-tramp)

> You can keep the :group out; the last declared defgroup is taken by
> default.

Thanks.

>> ;;;###autoload
>> (defun mock-tramp--list-chroots (directory)
>>   "Return a list of chroots defined in DIRECTORY.

> Why ;;;###autoload?

>> ;;;###autoload
>> (with-eval-after-load 'tramp
>>   (add-to-list 'tramp-methods

> Same here.

To mimic docker-tramp's (IMHO sensible) behaviour:
Installing the package sets up the Tramp method.

>>                  (tramp-login-program "mock")

> I would declare and use `mock-tramp-program'.

I'd rather not.  Mock is very specialized, and it is unlike-
ly that a regular user will ever want or need to change
tramp-login-program, but nothing else.  If I introduce a
customization for this now, it is thus unlikely that is
(ever) used, but it takes screen/mental space nonetheless.

>>                  (tramp-login-args (("-r") ("%h")
>>                                     ("--shell"
>>                                      "--"
>>                                      "/usr/bin/env"
>>                                      "PROMPT_COMMAND="
>>                                      "/bin/sh"

> I would use `tramp-default-remote-shell'.

>>                                      "-l")))
>>                  (tramp-remote-shell "/bin/sh")

> Same here.

Thanks.

I have attached a new revision of the package.

Tim

;;; mock-tramp.el --- Tramp integration for Mock chroots  -*- lexical-binding: 
t; -*-

;; Copyright (C) 2024  Tim Landscheidt

;; Author: Tim Landscheidt <tim@tim-landscheidt.de>
;; Version: 1.0rc7
;; Package-Requires: ((tramp "2.5.4-pre"))
;; Keywords: comm, processes

;; This program is free software; you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; This program is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with this program.  If not, see <https://www.gnu.org/licenses/>.

;;; Commentary:

;; This package provides a Tramp method to access Mock chroots
;; (https://rpm-software-management.github.io/mock/).  For example,
;; after running "fedpkg mockbuild --no-cleanup-after", one can edit
;; the files in the chroot by visiting "/mock:default:/path/to/file"
;; and execute commands in this chroot with M-x shell-command RET.

;;; Code:

(require 'subr-x)
(require 'tramp)

(defgroup mock-tramp nil
  "Tramp integration for Mock chroots."
  :prefix "mock-tramp-"
  :group 'applications
  :link '(emacs-commentary-link "mock-tramp"))

;;;###autoload
(defcustom mock-tramp-method "mock"
  "Tramp method to connect to Mock chroots."
  :type 'string
  :package-version '(mock-tramp . "1.0rc2"))

;;;###autoload
(defun mock-tramp--list-chroots (directory)
  "Return a list of chroots defined in DIRECTORY.

The list consists of (\"\" chroot) sublists suitable for Tramp
completion."
  ;; Unfortunately, mock provides no machine-readable list of chroots,
  ;; therefore here the logic in mock/py/mockbuild/config.py and
  ;; mock/etc/bash_completion.d/mock needs to be duplicated.  An RFE
  ;; for such an option to mock is tracked at
  ;; https://github.com/rpm-software-management/mock/issues/1294.
  (if (file-directory-p directory)
      (seq-reduce
       (lambda (r filename)
         (if (not (and (string= directory "/etc/mock")
                       (member filename '("chroot-aliases.cfg"
                                          "site-defaults.cfg"))))
             (append (list (list nil (string-remove-suffix ".cfg" filename))) r)
           r))
       (directory-files directory nil "^.*\\.cfg$" t)
       '())))

;;;###autoload
(with-eval-after-load 'tramp
  (add-to-list 'tramp-methods
               `(,mock-tramp-method
                 (tramp-login-program "mock")
                 (tramp-login-args (("-r") ("%h")
                                    ("--shell"
                                     "--"
                                     "/usr/bin/env"
                                     "PROMPT_COMMAND="
                                     ,tramp-default-remote-shell
                                     "-l")))
                 (tramp-remote-shell ,tramp-default-remote-shell)
                 (tramp-remote-shell-args ("-i" "-c"))))
  (tramp-set-completion-function
   mock-tramp-method
   '((mock-tramp--list-chroots "/etc/mock")
     (mock-tramp--list-chroots "~/.config/mock"))))

(provide 'mock-tramp)
;;; mock-tramp.el ends here