From d00a37e47107355516497b64a9cec54b0b17cfb4 Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Thu, 15 Sep 2022 12:24:37 -0700 Subject: [PATCH 4/5] Improve handling of $PATH in Eshell for remote directories * lisp/eshell/esh-util.el (eshell-path-env, eshell-parse-colon-path): Make obsolete. (eshell-host-path-env): New variable. (eshell-get-path-assq, eshell-set-path): New functions. (eshell-get-path): Use 'eshell-get-path-assq'. * lisp/eshell/esh-var.el (eshell-variable-aliases-list): Add entry for $PATH. (eshell-var-initialize): Add 'eshell-host-path-env' to 'eshell-subcommand-bindings'. * lisp/eshell/esh-ext.el (eshell-search-path): Use 'file-name-concat' instead of 'concat'. (eshell/addpath): Use 'eshell-get-path' and 'eshell-set-path'. * lisp/net/tramp-integration.el: Only apply Eshell hooks when 'eshell-host-path-env' is unbound. * test/lisp/eshell/esh-var-tests.el (esh-var-test/path-var/local-directory) (esh-var-test/path-var/remote-directory, esh-var-test/path-var/set) (esh-var-test/path-var/set-locally): New tests. * test/lisp/eshell/esh-ext-tests.el: New file. * test/lisp/eshell/eshell-tests-helpers.el (eshell-tests-remote-accessible-p): New function. * doc/misc/eshell.texi (Variables): Document $PATH. * etc/NEWS: Announce this change (bug#57556). --- doc/misc/eshell.texi | 8 +++ etc/NEWS | 5 ++ lisp/eshell/esh-ext.el | 25 ++++---- lisp/eshell/esh-util.el | 65 ++++++++++++++++++-- lisp/eshell/esh-var.el | 12 +++- lisp/net/tramp-integration.el | 21 ++++--- test/lisp/eshell/esh-ext-tests.el | 77 ++++++++++++++++++++++++ test/lisp/eshell/esh-var-tests.el | 42 +++++++++++++ test/lisp/eshell/eshell-tests-helpers.el | 10 +++ 9 files changed, 235 insertions(+), 30 deletions(-) create mode 100644 test/lisp/eshell/esh-ext-tests.el diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi index 48edee59ab..dc1af16fcf 100644 --- a/doc/misc/eshell.texi +++ b/doc/misc/eshell.texi @@ -939,6 +939,14 @@ Variables directory ring via subscripting, e.g.@: @samp{$-[1]} refers to the working directory @emph{before} the previous one. +@vindex $PATH +@item $PATH +This specifies the directories to search for executable programs as a +string, separated by @code{":"} for Unix and GNU systems, and +@code{";"} for MS systems. This variable is connection-aware, so when +the current directory on a remote host, it will automatically update +to reflect the search path on that host. + @vindex $_ @item $_ This refers to the last argument of the last command. With a diff --git a/etc/NEWS b/etc/NEWS index 76e44965ba..7d1f9bf355 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -320,6 +320,11 @@ previous 'C-x ='. ** Eshell +*** Eshell's PATH is now derived from 'exec-path'. +For consistency with remote connections, Eshell now uses 'exec-path' +to determine the execution path on the local system, instead of using +the PATH environment variable directly. + --- *** 'source' and '.' no longer accept the '--help' option. This is for compatibility with the shell versions of these commands, diff --git a/lisp/eshell/esh-ext.el b/lisp/eshell/esh-ext.el index 98902fc6f2..4ec9241c22 100644 --- a/lisp/eshell/esh-ext.el +++ b/lisp/eshell/esh-ext.el @@ -77,7 +77,7 @@ eshell-search-path (let ((list (eshell-get-path)) suffixes n1 n2 file) (while list - (setq n1 (concat (car list) name)) + (setq n1 (file-name-concat (car list) name)) (setq suffixes eshell-binary-suffixes) (while suffixes (setq n2 (concat n1 (car suffixes))) @@ -231,6 +231,8 @@ eshell-external-command (eshell-gather-process-output (car interp) (append (cdr interp) args))))) +(defvar eshell-in-subcommand-p) ; Defined in esh-cmd.el. + (defun eshell/addpath (&rest args) "Add a set of paths to PATH." (eshell-eval-using-options @@ -239,17 +241,16 @@ eshell/addpath (?h "help" nil nil "display this usage message") :usage "[-b] PATH Adds the given PATH to $PATH.") - (if args - (progn - (setq eshell-path-env (getenv "PATH") - args (mapconcat #'identity args path-separator) - eshell-path-env - (if prepend - (concat args path-separator eshell-path-env) - (concat eshell-path-env path-separator args))) - (setenv "PATH" eshell-path-env)) - (dolist (dir (parse-colon-path (getenv "PATH"))) - (eshell-printn dir))))) + (let ((path (eshell-get-path t))) + (if args + (progn + (setq path (if prepend + (append args path) + (append path args))) + (eshell-set-path path eshell-in-subcommand-p) + (string-join path (path-separator))) + (dolist (dir path) + (eshell-printn dir)))))) (put 'eshell/addpath 'eshell-no-numeric-conversions t) (put 'eshell/addpath 'eshell-filename-arguments t) diff --git a/lisp/eshell/esh-util.el b/lisp/eshell/esh-util.el index 9258ca5e40..d7cb79830b 100644 --- a/lisp/eshell/esh-util.el +++ b/lisp/eshell/esh-util.el @@ -249,17 +249,70 @@ eshell-path-env It might be different from \(getenv \"PATH\"), when `default-directory' points to a remote host.") -(defun eshell-get-path () +(make-obsolete-variable 'eshell-path-env 'eshell-get-path "29.1") + +(defvar-local eshell-host-path-env nil + "An alist mapping local and remote hosts to their `exec-path' values. +These can be retrieved via `eshell-get-path' or updated via +`eshell-set-path'.") + +(defun eshell-get-path-assq (host &optional initialize copy-p) + "Return the path association for HOST from `eshell-host-path-env'. +If HOST is nil, use `localhost'. If the association already +exists, just return it; if COPY-P is non-nil, push a copy of the +association onto the list and return that. This is useful when +temporarily altering the path. + +If the association doesn't exisst and INITIALIZE is non-nil, +initialize it from `exec-path' first." + (if-let ((host-id (intern (or host "localhost"))) + (cached-path (assq host-id eshell-host-path-env))) + (if copy-p + (car (push (copy-tree cached-path) eshell-host-path-env)) + cached-path) + ;; If not already cached, get the path from `exec-path', removing + ;; the last element, which is `exec-directory'. + (car (push (cons host-id (when initialize (butlast (exec-path)))) + eshell-host-path-env)))) + +(defun eshell-get-path (&optional local-part) "Return $PATH as a list. -Add the current directory on MS-Windows." - (eshell-parse-colon-path - (if (eshell-under-windows-p) - (concat "." path-separator eshell-path-env) - eshell-path-env))) +If LOCAL-PART is non-nil, only return the local part of the path. +Otherwise, return the full, possibly-remote path. + +On MS-Windows, add the current directory as the first directory +in the path." + (let* ((remote (file-remote-p default-directory)) + (path (cdr (eshell-get-path-assq remote t)))) + (when (and (eshell-under-windows-p) + (not remote)) + (push "." path)) + (if (and remote (not local-part)) + (mapcar (lambda (x) (concat remote x)) path) + path))) + +(defun eshell-set-path (path &optional copy-p) + "Set the Eshell $PATH to PATH. +PATH can be either a list of directories or a string of +directories separated by `path-separator'. + +If COPY-P is non-nil, set this as a new entry in +`eshell-host-path-env'. This is useful for temporarily altering +the path." + (let* ((remote (file-remote-p default-directory)) + (path-entry (eshell-get-path-assq remote nil copy-p))) + (setcdr path-entry + (if (listp path) + path + ;; Don't use `parse-colon-path' here, since we don't + ;; want the additonal translations it does on each + ;; element. + (split-string path (path-separator)))))) (defun eshell-parse-colon-path (path-env) "Split string with `parse-colon-path'. Prepend remote identification of `default-directory', if any." + (declare (obsolete nil "29.1")) (let ((remote (file-remote-p default-directory))) (if remote (mapcar diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el index caf143e1a1..8a27ab747d 100644 --- a/lisp/eshell/esh-var.el +++ b/lisp/eshell/esh-var.el @@ -156,7 +156,14 @@ eshell-variable-aliases-list ("LINES" ,(lambda () (window-body-height nil 'remap)) t t) ("INSIDE_EMACS" eshell-inside-emacs t) - ;; for eshell-cmd.el + ;; for esh-ext.el + ("PATH" (,(lambda () (string-join (eshell-get-path t) (path-separator))) + . ,(lambda (_ value) + (eshell-set-path value eshell-in-subcommand-p) + value)) + t t) + + ;; for esh-cmd.el ("_" ,(lambda (indices quoted) (if (not indices) (car (last eshell-last-arguments)) @@ -249,7 +256,8 @@ eshell-var-initialize (setq-local eshell-subcommand-bindings (append '((process-environment (eshell-copy-environment)) - (eshell-variable-aliases-list eshell-variable-aliases-list)) + (eshell-variable-aliases-list eshell-variable-aliases-list) + (eshell-host-path-env eshell-host-path-env)) eshell-subcommand-bindings)) (setq-local eshell-special-chars-inside-quoting diff --git a/lisp/net/tramp-integration.el b/lisp/net/tramp-integration.el index 35c0636b1c..b054c7c7d0 100644 --- a/lisp/net/tramp-integration.el +++ b/lisp/net/tramp-integration.el @@ -136,16 +136,17 @@ tramp-eshell-directory-change (getenv "PATH")))) (with-eval-after-load 'esh-util - (add-hook 'eshell-mode-hook - #'tramp-eshell-directory-change) - (add-hook 'eshell-directory-change-hook - #'tramp-eshell-directory-change) - (add-hook 'tramp-integration-unload-hook - (lambda () - (remove-hook 'eshell-mode-hook - #'tramp-eshell-directory-change) - (remove-hook 'eshell-directory-change-hook - #'tramp-eshell-directory-change)))) + (unless (boundp 'eshell-host-path-env) + (add-hook 'eshell-mode-hook + #'tramp-eshell-directory-change) + (add-hook 'eshell-directory-change-hook + #'tramp-eshell-directory-change) + (add-hook 'tramp-integration-unload-hook + (lambda () + (remove-hook 'eshell-mode-hook + #'tramp-eshell-directory-change) + (remove-hook 'eshell-directory-change-hook + #'tramp-eshell-directory-change))))) ;;; Integration of recentf.el: diff --git a/test/lisp/eshell/esh-ext-tests.el b/test/lisp/eshell/esh-ext-tests.el new file mode 100644 index 0000000000..cf0910d797 --- /dev/null +++ b/test/lisp/eshell/esh-ext-tests.el @@ -0,0 +1,77 @@ +;;; esh-ext-tests.el --- esh-ext test suite -*- lexical-binding:t -*- + +;; Copyright (C) 2022 Free Software Foundation, Inc. + +;; This file is part of GNU Emacs. + +;; GNU Emacs 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. + +;; GNU Emacs 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 GNU Emacs. If not, see . + +;;; Commentary: + +;; Tests for Eshell's external command handling. + +;;; Code: + +(require 'ert) +(require 'esh-mode) +(require 'esh-ext) +(require 'eshell) + +(require 'eshell-tests-helpers + (expand-file-name "eshell-tests-helpers" + (file-name-directory (or load-file-name + default-directory)))) + +;;; Tests: + +(ert-deftest esh-ext-test/addpath/end () + "Test that \"addpath\" adds paths to the end of $PATH." + (with-temp-eshell + (let ((eshell-host-path-env '((localhost . ("/some/path" "/other/path")))) + (expected-path (string-join '("/some/path" "/other/path" "/new/path" + "/new/path2") + (path-separator)))) + (eshell-match-command-output "addpath /new/path /new/path2" + (concat expected-path "\n")) + (eshell-match-command-output "echo $PATH" + (concat expected-path "\n"))))) + +(ert-deftest esh-ext-test/addpath/begin () + "Test that \"addpath -b\" adds paths to the beginning of $PATH." + (with-temp-eshell + (let ((eshell-host-path-env '((localhost . ("/some/path" "/other/path")))) + (expected-path (string-join '("/new/path" "/new/path2" "/some/path" + "/other/path") + (path-separator)))) + (eshell-match-command-output "addpath -b /new/path /new/path2" + (concat expected-path "\n")) + (eshell-match-command-output "echo $PATH" + (concat expected-path "\n"))))) + +(ert-deftest esh-ext-test/addpath/set-locally () + "Test adding to the path temporarily in a subcommand." + (let* ((original-path-list '("/some/path" "/other/path")) + (eshell-host-path-env `((localhost . ,original-path-list))) + (original-path (string-join original-path-list (path-separator))) + (local-path (string-join (append original-path-list '("/new/path")) + (path-separator)))) + (with-temp-eshell + (eshell-match-command-output + "{ addpath /new/path; env }" + (format "PATH=%s\n" (regexp-quote local-path))) + ;; After the last command, the previous $PATH value should be restored. + (eshell-match-command-output "echo $PATH" + (concat original-path "\n"))))) + +;; esh-ext-tests.el ends here diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el index a7ac52ed24..6f4b0b9994 100644 --- a/test/lisp/eshell/esh-var-tests.el +++ b/test/lisp/eshell/esh-var-tests.el @@ -23,6 +23,7 @@ ;;; Code: +(require 'tramp) (require 'ert) (require 'esh-mode) (require 'esh-var) @@ -610,6 +611,47 @@ esh-var-test/inside-emacs-var-split-indices (eshell-match-command-output "echo $INSIDE_EMACS[, 1]" "eshell"))) +(ert-deftest esh-var-test/path-var/local-directory () + "Test using $PATH in a local directory." + (let ((expected-path (string-join (eshell-get-path t) (path-separator)))) + (with-temp-eshell + (eshell-match-command-output "echo $PATH" (regexp-quote expected-path)) + (should (equal (mapcar #'car eshell-host-path-env) + '(localhost)))))) + +(ert-deftest esh-var-test/path-var/remote-directory () + "Test using $PATH in a remote directory." + (skip-unless (eshell-tests-remote-accessible-p)) + (let* ((default-directory ert-remote-temporary-file-directory) + (expected-path (string-join (eshell-get-path t) (path-separator)))) + (with-temp-eshell + (eshell-match-command-output "echo $PATH" (regexp-quote expected-path)) + (should (equal (mapcar #'car eshell-host-path-env) + (list (intern (file-remote-p default-directory)))))))) + +(ert-deftest esh-var-test/path-var/set () + "Test setting $PATH." + (let* ((path-to-set-list '("/some/path" "/other/path")) + (path-to-set (string-join path-to-set-list (path-separator)))) + (with-temp-eshell + (eshell-match-command-output (concat "set PATH " path-to-set) + (concat path-to-set "\n")) + (eshell-match-command-output "echo $PATH" (concat path-to-set "\n")) + (should (equal (eshell-get-path) path-to-set-list))))) + +(ert-deftest esh-var-test/path-var/set-locally () + "Test setting $PATH temporarily for a single command." + (let* ((path-to-set-list '("/some/path" "/other/path")) + (path-to-set (string-join path-to-set-list (path-separator)))) + (with-temp-eshell + (eshell-match-command-output (concat "set PATH " path-to-set) + (concat path-to-set "\n")) + (eshell-match-command-output "PATH=/local/path env" + "PATH=/local/path\n") + ;; After the last command, the previous $PATH value should be restored. + (eshell-match-command-output "echo $PATH" (concat path-to-set "\n")) + (should (equal (eshell-get-path) path-to-set-list))))) + (ert-deftest esh-var-test/last-status-var-lisp-command () "Test using the \"last exit status\" ($?) variable with a Lisp command" (with-temp-eshell diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el index e713e162ad..0067216f70 100644 --- a/test/lisp/eshell/eshell-tests-helpers.el +++ b/test/lisp/eshell/eshell-tests-helpers.el @@ -36,6 +36,16 @@ eshell-test--max-subprocess-time "The maximum amount of time to wait for a subprocess to finish, in seconds. See `eshell-wait-for-subprocess'.") +(defun eshell-tests-remote-accessible-p () + "Return if a test involving remote files can proceed. +If using this function, be sure to load `tramp' near the +beginning of the test file." + (ignore-errors + (and + (file-remote-p ert-remote-temporary-file-directory) + (file-directory-p ert-remote-temporary-file-directory) + (file-writable-p ert-remote-temporary-file-directory)))) + (defmacro with-temp-eshell (&rest body) "Evaluate BODY in a temporary Eshell buffer." `(save-current-buffer -- 2.25.1