From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
To: Antero Mejr <antero@mailbox.org>
Cc: ludo@gnu.org, 62848@debbugs.gnu.org
Subject: [bug#62848] [PATCH] environment: Add --remote option and emacsclient-eshell backend.
Date: Fri, 01 Sep 2023 09:26:03 -0400 [thread overview]
Message-ID: <877cpa3tzo.fsf@gmail.com> (raw)
In-Reply-To: <20230415014430.15466-1-antero@mailbox.org> (Antero Mejr's message of "Sat, 15 Apr 2023 01:44:30 +0000")
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
next prev parent reply other threads:[~2023-09-01 13:27 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-15 1:44 [bug#62848] [PATCH] environment: Add --remote option and emacsclient-eshell backend Antero Mejr via Guix-patches via
2023-09-01 13:26 ` Maxim Cournoyer [this message]
2023-11-07 22:30 ` [bug#62848] [PATCH v2] " Antero Mejr via Guix-patches via
2023-11-08 5:29 ` Liliana Marie Prikler
2023-11-08 15:19 ` [bug#62848] [PATCH 1/2] guix: Rename white-list to allow-list Antero Mejr via Guix-patches via
2023-11-08 15:21 ` [bug#62848] [PATCH 2/2] environment: Add --remote option and emacsclient-eshell backend Antero Mejr via Guix-patches via
2023-11-08 19:32 ` Liliana Marie Prikler
2023-11-08 15:34 ` [bug#62848] [PATCH v2] " Antero Mejr via Guix-patches via
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=877cpa3tzo.fsf@gmail.com \
--to=maxim.cournoyer@gmail.com \
--cc=62848@debbugs.gnu.org \
--cc=antero@mailbox.org \
--cc=ludo@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.