[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#62848] [PATCH] environment: Add --remote option and emacsclient-esh
From: |
Maxim Cournoyer |
Subject: |
[bug#62848] [PATCH] environment: Add --remote option and emacsclient-eshell backend. |
Date: |
Fri, 01 Sep 2023 09:26:03 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
Hi Antero,
Antero Mejr <antero@mailbox.org> writes:
> * guix/scripts/environment.scm (launch-environment/eshell): New procedure.
> (guix-environment*): Add logic for remote backend switching.
> (%options): Add --remote and --list-remote-backends options.
> (show-environment-options-help): Add help text for new options.
> * guix/profiles.scm (load-profile)[getenv-proc, setenv-proc, unsetenv-proc]:
> New optional keyword arguments.
> (purify-environment)[unsetenv-proc]: New argument.
> * doc/guix.texi(Invoking guix shell): Document --remote and
> --list-remote-backends options.
This looks interesting!
[...]
> +(define* (launch-environment/eshell args command profile manifest
> + #:key pure? (white-list '()))
Maybe use the modern allow-list / block-list terminology.
> + "Create an new eshell buffer with an environment containing PROFILE,
> +with the search paths specified by MANIFEST. When PURE?, pre-existing
> +environment variables are cleared before setting the new ones, except those
> +matching the regexps in WHITE-LIST."
> +
> + (define (escape in)
> + (string-append "\"" (string-replace-substring in "\"" "\\\"") "\""))
> +
> + (define* (ec cmd #:optional (check? #f))
^
Shouldn't we always check for errors? When is it useful to let them
through?
> + (let* ((cmd (string-append "emacsclient " args " -e " (escape cmd)))
Instead of having to escape stuff, it'd be better to avoid using a shell
for the invocation by opening the pipe with (open-pipe* OPEN_READ prog
arg ...). There's an example if you grep for
'with-input-pipe-to-string' in the update-guix-package.scm file. This
should make the rest of the code more readable.
> + (port (open-input-pipe cmd))
> + (str (read-line port))
> + (code (status:exit-val (close-pipe port))))
> + (if (and check? (or (not (eqv? code 0)) (eof-object? str)))
^
(zero? code)
> + (leave
> + (G_ "Emacs server connection failed. Is the server running?~%")))
> + str))
> +
> + (let ((buf (ec "(buffer-name (eshell t))" #t)))
> + (define (ec-buf cmd)
> + (ec (string-append "(with-current-buffer " buf " " cmd ")")))
> +
> + (load-profile
> + profile manifest #:pure? pure? #:white-list-regexps white-list
> + #:setenv-proc (lambda (var val)
> + (ec-buf
> + (if (string=? var "PATH")
> + ;; TODO: TRAMP support?
> + (string-append "(eshell-set-path " (escape val)
> ")")
> + (string-append "(setenv " (escape var) " "
> + (escape val) ")"))))
> + #:unsetenv-proc (lambda (var)
> + (ec-buf
> + (string-append "(setenv " (escape var) ")"))))
> +
> + (match command
> + ((program . args)
> + (ec-buf
> + (string-append
> + "(eshell-command "
> + (escape (string-append program " " (string-join args)))")"))))))
> +
> (define* (launch-environment/container #:key command bash user user-mappings
> profile manifest link-profile?
> network?
> map-cwd? emulate-fhs? nesting?
> @@ -1081,7 +1140,10 @@ (define (guix-environment* opts)
> '("/bin/sh")
> (list %default-shell))))
> (mappings (pick-all opts 'file-system-mapping))
> - (white-list (pick-all opts 'inherit-regexp)))
> + (white-list (pick-all opts 'inherit-regexp))
> + (remote (match (string-split (assoc-ref opts 'remote) #\=)
> + ((x) (cons x ""))
Why is this needed?
> + ((x . y) (cons x (string-join y))))))
If using open-pipe* as suggested above, all the arguments could be kept
as a list.
>
> (define store-needed?
> ;; Whether connecting to the daemon is needed.
> @@ -1119,6 +1181,10 @@ (define-syntax-rule (with-store/maybe store exp ...)
> (when (pair? symlinks)
> (leave (G_ "'--symlink' cannot be used without '--container~%'"))))
>
> + (when (and remote (not (member (car remote) '("emacsclient-eshell"))))
> + (leave
> + (G_ "Invalid remote backend, see --list-remote-backends for
> options.~%'")))
This code and the --list-remote-backends associated procedure could
share a same %supported-backends list containing currently just
'("emacsclient-eshell").
> (with-store/maybe store
> (with-status-verbosity (assoc-ref opts 'verbosity)
> (define manifest-from-opts
> @@ -1172,15 +1238,23 @@ (define manifest
>
> (mwhen (assoc-ref opts 'check?)
> (return
> - (if container?
> + (if (or container? remote)
> (warning (G_ "'--check' is unnecessary \
> -when using '--container'; doing nothing~%"))
> +when using '--container' or '--remote'; doing nothing~%"))
> (validate-child-shell-environment profile manifest))))
>
> (cond
> ((assoc-ref opts 'search-paths)
> (show-search-paths profile manifest #:pure? pure?)
> (return #t))
> + (remote
> + (match (car remote)
> + ("emacsclient-eshell"
> + (return
> + (launch-environment/eshell (cdr remote)
> + command profile manifest
> + #:white-list white-list
> + #:pure? pure?)))))
> (container?
> (let ((bash-binary
> (if bootstrap?
It'd be nice to have a functional test for it; some inspiration could be
taken from tests/build-emacs-utils.scm, which skips the test if emacs is
not available.
Thanks for this contribution! Could you please send a v2 taking into
account the above comments?
--
Thanks,
Maxim
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- [bug#62848] [PATCH] environment: Add --remote option and emacsclient-eshell backend.,
Maxim Cournoyer <=