bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 142 bytes --]

Severity: minor
Tags: patch

With pylsp and rust-analyzer installed, I have been seeing some or all
of these failures for quite a while now:


[-- Attachment #2: eglot-tests.txt.gz --]
[-- Type: application/gzip, Size: 6551 bytes --]

[-- Attachment #3: Type: text/plain, Size: 807 bytes --]


The main problem is that:
0. test/Makefile.in sets HOME=/nonexistent
1. lisp/emacs-lisp/ert-x.el sets HOME=/tmp :(
2. eglot--call-with-fixture tries to detect when HOME is nonexistent,
   but /tmp exists, so that's left unchanged
3. The Rust tools look under HOME=/tmp for which toolchain to use, but
   the answer is under ~USER, so they give up

The following patch temporarily sets HOME=~USER in only those tests that
need it.  The patch consolidates some let-bindings into a single form to
minimise collateral indentation changes.  It also bumps the
rust-analyzer client/registerCapability timeout from 1 second to 3,
because sometimes 1 second was too short a wait.  Finally, it updates
the expected autopep8 reformatting results to what I see locally, and
skips a test that fails without YASnippet.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Fix-eglot-tests.el-that-need-HOME-USER.patch --]
[-- Type: text/x-diff, Size: 9652 bytes --]

From 825d0781522f87c6a2cc954a85b2c48a499d4857 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sun, 19 Feb 2023 17:41:48 +0000
Subject: [PATCH 1/2] Fix eglot-tests.el that need HOME=~USER

* test/lisp/progmodes/eglot-tests.el (eglot-tests--real-home-env):
New helper function.
(eglot-test-rust-analyzer-watches-files)
(eglot-test-diagnostic-tags-unnecessary-code)
(eglot-test-rust-analyzer-hover-after-edit)
(eglot-test-rust-on-type-formatting)
(eglot-test-project-wide-diagnostics-rust-analyzer): Use it to run
cargo and rust-analyzer under HOME=~USER.

(eglot-test-python-autopep-formatting): Update expected formats for
latest autopep8.
(eglot-test-json-basic): Skip when YASnippet is missing.
---
 test/lisp/progmodes/eglot-tests.el | 108 +++++++++++++++++------------
 1 file changed, 63 insertions(+), 45 deletions(-)

diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
index 4b6528351b2..4e1b025e63f 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -86,6 +86,14 @@ eglot--make-file-or-dir
           (t
            (eglot--error "Expected a string or a directory spec")))))
 
+(defun eglot-tests--real-home-env ()
+  "Return a `process-environment' with user's real HOME directory.
+This uses `user-login-name' to work around cases where the HOME
+environment variable has been set to something like
+\"/nonexistent\" or `temporary-file-directory'."
+  (cons (concat "HOME=" (expand-file-name (concat "~" (user-login-name))))
+        process-environment))
+
 (defun eglot--call-with-fixture (fixture fn)
   "Helper for `eglot--with-fixture'.  Run FN under FIXTURE."
   (let* ((fixture-directory (make-nearby-temp-file "eglot--fixture" t))
@@ -394,27 +402,27 @@ eglot-test-rust-analyzer-watches-files
   "Start rust-analyzer.  Notify it when a critical file changes."
   (skip-unless (executable-find "rust-analyzer"))
   (skip-unless (executable-find "cargo"))
-  (let ((eglot-autoreconnect 1))
-    (eglot--with-fixture
-        '(("watch-project" . (("coiso.rs" . "bla")
-                              ("merdix.rs" . "bla"))))
-      (with-current-buffer
-          (eglot--find-file-noselect "watch-project/coiso.rs")
+  (eglot--with-fixture
+      '(("watch-project" . (("coiso.rs" . "bla")
+                            ("merdix.rs" . "bla"))))
+    (with-current-buffer
+        (eglot--find-file-noselect "watch-project/coiso.rs")
+      (let ((eglot-autoreconnect 1)
+            ;; Cargo doesn't work under `temporary-file-directory'.
+            (process-environment (eglot-tests--real-home-env))
+            register-id)
         (should (zerop (shell-command "cargo init")))
-        (eglot--sniffing (
-                          :server-requests s-requests
-                          :client-notifications c-notifs
-                          :client-replies c-replies
-                          )
+        (eglot--sniffing ( :server-requests s-requests
+                           :client-notifications c-notifs
+                           :client-replies c-replies)
           (should (eglot--tests-connect))
-          (let (register-id)
-            (eglot--wait-for (s-requests 1)
-                (&key id method &allow-other-keys)
-              (setq register-id id)
-              (string= method "client/registerCapability"))
-            (eglot--wait-for (c-replies 1)
-                (&key id error &allow-other-keys)
-              (and (eq id register-id) (null error))))
+          (eglot--wait-for (s-requests 3)
+              (&key id method &allow-other-keys)
+            (setq register-id id)
+            (string= method "client/registerCapability"))
+          (eglot--wait-for (c-replies 1)
+              (&key id error &allow-other-keys)
+            (and (eq id register-id) (null error)))
           (delete-file "Cargo.toml")
           (eglot--wait-for
               (c-notifs 3 "waiting for didChangeWatchedFiles notification")
@@ -453,7 +461,9 @@ eglot-test-diagnostic-tags-unnecessary-code
            "fn main() -> () { let test=3; }"))))
     (with-current-buffer
         (eglot--find-file-noselect "diagnostic-tag-project/main.rs")
-      (let ((eglot-server-programs '((rust-mode . ("rust-analyzer")))))
+      (let ((eglot-server-programs '((rust-mode . ("rust-analyzer"))))
+            ;; Cargo doesn't work under `temporary-file-directory'.
+            (process-environment (eglot-tests--real-home-env)))
         (should (zerop (shell-command "cargo init")))
         (eglot--sniffing (:server-notifications s-notifs)
           (eglot--tests-connect)
@@ -496,21 +506,20 @@ eglot-test-rust-analyzer-hover-after-edit
            "fn test() -> i32 { let test=3; return te; }"))))
     (with-current-buffer
         (eglot--find-file-noselect "hover-project/main.rs")
-      (should (zerop (shell-command "cargo init")))
-      (eglot--sniffing (
-                        :server-replies s-replies
-                        :client-requests c-reqs
-                        )
-        (eglot--tests-connect)
-        (goto-char (point-min))
-        (search-forward "return te")
-        (insert "st")
-        (progn
-          ;; simulate these two which don't happen when buffer isn't
+      (let (;; Cargo doesn't work under `temporary-file-directory'.
+            (process-environment (eglot-tests--real-home-env))
+            pending-id)
+        (should (zerop (shell-command "cargo init")))
+        (eglot--sniffing ( :server-replies s-replies
+                           :client-requests c-reqs)
+          (eglot--tests-connect)
+          (goto-char (point-min))
+          (search-forward "return te")
+          (insert "st")
+          ;; Simulate these two which don't happen when buffer isn't
           ;; visible in a window.
           (eglot--signal-textDocument/didChange)
-          (eglot--eldoc-on-demand))
-        (let (pending-id)
+          (eglot--eldoc-on-demand)
           (eglot--wait-for (c-reqs 2)
               (&key id method &allow-other-keys)
             (setq pending-id id)
@@ -683,18 +692,22 @@ eglot-test-python-autopep-formatting
     (with-current-buffer
         (eglot--find-file-noselect "project/something.py")
       (should (eglot--tests-connect))
-      ;; Try to format just the second line
+      ;; Try to format just the second line.
       (search-forward "b():pa")
       (eglot-format (line-beginning-position) (line-end-position))
-      (should (looking-at "ss"))
-      (should
-       (or (string= (buffer-string) "def a():pass\n\n\ndef b(): pass\n")
-           ;; autopep8 2.0.0 (pycodestyle: 2.9.1)
-           (string= (buffer-string) "def a():pass\n\ndef b(): pass")))
-      ;; now format the whole buffer
+      (should (looking-at-p "ss"))
+      (should (member (buffer-string)
+                      '(;; autopep8 2.0.1 (pycodestyle: 2.10.0)
+                        "def a():pass\n\ndef b():\n    pass\n"
+                        ;; autopep8 2.0.0 (pycodestyle: 2.9.1)
+                        "def a():pass\n\ndef b(): pass"
+                        "def a():pass\n\n\ndef b(): pass\n")))
+      ;; Now format the whole buffer.
       (eglot-format-buffer)
-      (should
-       (string= (buffer-string) "def a(): pass\n\n\ndef b(): pass\n")))))
+      (should (member (buffer-string)
+                      '(;; autopep8 2.0.1 (pycodestyle: 2.10.0)
+                        "def a():\n    pass\n\n\ndef b():\n    pass\n"
+                        "def a(): pass\n\n\ndef b(): pass\n"))))))
 
 (ert-deftest eglot-test-python-yapf-formatting ()
   "Test formatting in the pylsp python LSP."
@@ -728,12 +741,14 @@ eglot-test-rust-on-type-formatting
            "fn main() -> () {\n  foo\n    .bar()\n  "))))
     (with-current-buffer
         (eglot--find-file-noselect "on-type-formatting-project/main.rs")
-      (let ((eglot-server-programs '((rust-mode . ("rust-analyzer")))))
+      (let ((eglot-server-programs '((rust-mode . ("rust-analyzer"))))
+            ;; Cargo doesn't work under `temporary-file-directory'.
+            (process-environment (eglot-tests--real-home-env)))
         (should (zerop (shell-command "cargo init")))
         (eglot--sniffing (:server-notifications s-notifs)
           (should (eglot--tests-connect))
           (eglot--wait-for (s-notifs 10) (&key method &allow-other-keys)
-             (string= method "textDocument/publishDiagnostics")))
+            (string= method "textDocument/publishDiagnostics")))
         (goto-char (point-max))
         (eglot--simulate-key-event ?.)
         (should (looking-back "^    \\."))))))
@@ -808,7 +823,9 @@ eglot-test-project-wide-diagnostics-rust-analyzer
           ("other-file.rs" .
            "fn foo() -> () { let hi=3; }"))))
     (eglot--make-file-or-dir '(".git"))
-    (let ((eglot-server-programs '((rust-mode . ("rust-analyzer")))))
+    (let ((eglot-server-programs '((rust-mode . ("rust-analyzer"))))
+          ;; Cargo doesn't work under `temporary-file-directory'.
+          (process-environment (eglot-tests--real-home-env)))
       ;; Open other-file, and see diagnostics arrive for main.rs
       (with-current-buffer (eglot--find-file-noselect "project/other-file.rs")
         (should (zerop (shell-command "cargo init")))
@@ -829,6 +846,7 @@ eglot-test-project-wide-diagnostics-rust-analyzer
 (ert-deftest eglot-test-json-basic ()
   "Test basic autocompletion in vscode-json-languageserver."
   (skip-unless (executable-find "vscode-json-languageserver"))
+  (skip-unless (fboundp 'yas-minor-mode))
   (eglot--with-fixture
       '(("project" .
          (("p.json" . "{\"foo.b")
-- 
2.39.1


[-- Attachment #5: Type: text/plain, Size: 490 bytes --]


While putting this together, I also noticed some minor housekeeping
opportunities.  The following patch mainly removes the symbol-value pair
syntax from eglot--with-fixture.  The setting and unsetting of these
variables happened quite far away from the body, so they could not be
used to change, say, process-environment, since eglot--with-fixture
already binds that.  I think requiring the body to let-bind its own
variables is not only sufficient, but more general and less surprising.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0002-Minor-housekeeping-in-ert-x-and-eglot-tests.patch --]
[-- Type: text/x-diff, Size: 9869 bytes --]

From 731c39a27b9a21754cbee73e7199366a7cc38d82 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sun, 19 Feb 2023 18:09:06 +0000
Subject: [PATCH 2/2] Minor housekeeping in ert-x and eglot-tests

* lisp/emacs-lisp/ert-x.el (ert-remote-temporary-file-directory):
Don't add trailing slash to HOME.

* test/lisp/progmodes/eglot-tests.el: Move footer line to EOF.
(eglot--with-fixture): Don't allow fixture elements to be
symbol-value pairs.  This feature was used in only one test.  The
same effect can be achieved in a simpler way, and closer to the
body, with let-bindings.
(eglot--make-file-or-dir): Ensure default-directory ends with a
slash.  Simplify using with-temp-file.
(eglot--call-with-fixture): Ensure default-directory ends with a
slash.  Remove symbol setting and restoring logic.  Modify
process-environment only when needed, and simplify using
eglot-tests--real-home-env.  Leave XDG_CONFIG_HOME alone; it is
already unexported in test/Makefile.in.  Fix let-binding syntax.
(eglot--cleanup-after-test): Remove symbol restoring logic.
(eglot--eldoc-on-demand, eglot--tests-force-full-eldoc): Fix
commentary typos.
(eglot-test-ensure): Adapt c-mode-hook binding to
eglot--with-fixture changes.
---
 lisp/emacs-lisp/ert-x.el           |  2 +-
 test/lisp/progmodes/eglot-tests.el | 97 ++++++++++++------------------
 2 files changed, 39 insertions(+), 60 deletions(-)

diff --git a/lisp/emacs-lisp/ert-x.el b/lisp/emacs-lisp/ert-x.el
index 98a017c8a8e..3bf9b59f874 100644
--- a/lisp/emacs-lisp/ert-x.el
+++ b/lisp/emacs-lisp/ert-x.el
@@ -563,7 +563,7 @@ ert-remote-temporary-file-directory
         ;; Emacs's Makefile sets $HOME to a nonexistent value.  Needed
         ;; in batch mode only, therefore.
         (when (and noninteractive (not (file-directory-p "~/")))
-          (setenv "HOME" temporary-file-directory))
+          (setenv "HOME" (directory-file-name temporary-file-directory)))
         (format "/mock::%s" temporary-file-directory))))
     "Temporary directory for remote file tests.")
 
diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
index 4e1b025e63f..cce5b572521 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -60,28 +60,25 @@
 ;;; Helpers
 
 (defmacro eglot--with-fixture (fixture &rest body)
-  "Setup FIXTURE, call BODY, teardown FIXTURE.
+  "Set up FIXTURE, call BODY, tear down FIXTURE.
 FIXTURE is a list.  Its elements are of the form (FILE . CONTENT)
 to create a readable FILE with CONTENT.  FILE may be a directory
 name and CONTENT another (FILE . CONTENT) list to specify a
-directory hierarchy.  FIXTURE's elements can also be (SYMBOL
-VALUE) meaning SYMBOL should be bound to VALUE during BODY and
-then restored."
+directory hierarchy."
   (declare (indent 1) (debug t))
-  `(eglot--call-with-fixture
-    ,fixture #'(lambda () ,@body)))
+  `(eglot--call-with-fixture ,fixture (lambda () ,@body)))
 
 (defun eglot--make-file-or-dir (ass)
   (let ((file-or-dir-name (car ass))
         (content (cdr ass)))
     (cond ((listp content)
            (make-directory file-or-dir-name 'parents)
-           (let ((default-directory (concat default-directory "/" file-or-dir-name)))
+           (let* ((subdir (expand-file-name file-or-dir-name default-directory))
+                  (default-directory (file-name-as-directory subdir)))
              (mapcan #'eglot--make-file-or-dir content)))
           ((stringp content)
-           (with-temp-buffer
-             (insert content)
-             (write-region nil nil file-or-dir-name nil 'nomessage))
+           (with-temp-file file-or-dir-name
+             (insert content))
            (list (expand-file-name file-or-dir-name)))
           (t
            (eglot--error "Expected a string or a directory spec")))))
@@ -96,43 +93,27 @@ eglot-tests--real-home-env
 
 (defun eglot--call-with-fixture (fixture fn)
   "Helper for `eglot--with-fixture'.  Run FN under FIXTURE."
-  (let* ((fixture-directory (make-nearby-temp-file "eglot--fixture" t))
-         (default-directory fixture-directory)
-         file-specs created-files
-         syms-to-restore
+  (let* ((fixture-directory (make-nearby-temp-file "eglot--fixture-" t))
+         (default-directory (file-name-as-directory fixture-directory))
+         created-files
          new-servers
          test-body-successful-p)
-    (dolist (spec fixture)
-      (cond ((symbolp spec)
-             (push (cons spec (symbol-value spec)) syms-to-restore)
-             (set spec nil))
-            ((symbolp (car spec))
-             (push (cons (car spec) (symbol-value (car spec))) syms-to-restore)
-             (set (car spec) (cadr spec)))
-            ((stringp (car spec)) (push spec file-specs))))
     (unwind-protect
-        (let* ((home (getenv "HOME"))
-               (process-environment
-                (append
-                 `(;; Set XDF_CONFIG_HOME to /dev/null to prevent
-                   ;; user-configuration to have an influence on
-                   ;; language servers. (See github#441)
-                   "XDG_CONFIG_HOME=/dev/null"
-                   ;; ... on the flip-side, a similar technique by
-                   ;; Emacs's test makefiles means that HOME is set to
-                   ;; /nonexistent.  This breaks some common
-                   ;; installations for LSP servers like pylsp, making
-                   ;; these tests mostly useless, so we hack around it
-                   ;; here with a great big hack.
-                   ,(format "HOME=%s"
-                            (if (file-exists-p home) home
-                              (format "/home/%s" (getenv "USER")))))
-                 process-environment))
-               ;; Prevent "Can't guess python-indent-offset ..." messages.
-               (python-indent-guess-indent-offset-verbose . nil)
-               (eglot-server-initialized-hook
-                (lambda (server) (push server new-servers))))
-          (setq created-files (mapcan #'eglot--make-file-or-dir file-specs))
+        (let ((process-environment
+               (if (file-exists-p "~")
+                   process-environment
+                 ;; `test/Makefile' sets `HOME=/nonexistent' (and
+                 ;; `ert-remote-temporary-file-directory' sets it to
+                 ;; `temporary-file-directory').  This breaks some
+                 ;; common installations for LSP servers like pylsp,
+                 ;; making these tests mostly useless, so we hack
+                 ;; around it here with a great big hack.
+                 (eglot-tests--real-home-env)))
+              ;; Prevent "Can't guess python-indent-offset ..." messages.
+              (python-indent-guess-indent-offset-verbose nil)
+              (eglot-server-initialized-hook
+               (lambda (server) (push server new-servers))))
+          (setq created-files (mapcan #'eglot--make-file-or-dir fixture))
           (prog1 (funcall fn)
             (setq test-body-successful-p t)))
       (eglot--message
@@ -165,18 +146,15 @@ eglot--call-with-fixture
                         (t
                          (eglot--message "Preserved for inspection: %s"
                                          (mapconcat #'buffer-name buffers ", "))))))))
-        (eglot--cleanup-after-test fixture-directory created-files syms-to-restore)))))
+        (eglot--cleanup-after-test fixture-directory created-files)))))
 
-(defun eglot--cleanup-after-test (fixture-directory created-files syms-to-restore)
+(defun eglot--cleanup-after-test (fixture-directory created-files)
   (let ((buffers-to-delete
-         (delete nil (mapcar #'find-buffer-visiting created-files))))
-    (eglot--message "Killing %s, wiping %s, restoring %s"
+         (delq nil (mapcar #'find-buffer-visiting created-files))))
+    (eglot--message "Killing %s, wiping %s"
                     buffers-to-delete
-                    fixture-directory
-                    (mapcar #'car syms-to-restore))
-    (cl-loop for (sym . val) in syms-to-restore
-             do (set sym val))
-    (dolist (buf buffers-to-delete) ;; have to save otherwise will get prompted
+                    fixture-directory)
+    (dolist (buf buffers-to-delete) ;; Have to save otherwise will get prompted.
       (with-current-buffer buf (save-buffer) (kill-buffer)))
     (delete-directory fixture-directory 'recursive)
     ;; Delete Tramp buffers if needed.
@@ -476,11 +454,11 @@ eglot-test-diagnostic-tags-unnecessary-code
           (should (eq 'eglot-diagnostic-tag-unnecessary-face (face-at-point))))))))
 
 (defun eglot--eldoc-on-demand ()
-  ;; Trick Eldoc 1.1.0 into accepting on-demand calls.
+  ;; Trick ElDoc 1.1.0 into accepting on-demand calls.
   (eldoc t))
 
 (defun eglot--tests-force-full-eldoc ()
-  ;; FIXME: This uses some Eldoc implementation defatils.
+  ;; FIXME: This uses some ElDoc implementation details.
   (when (buffer-live-p eldoc--doc-buffer)
     (with-current-buffer eldoc--doc-buffer
       (let ((inhibit-read-only t))
@@ -898,9 +876,9 @@ eglot-test-ensure
   (skip-unless (executable-find "clangd"))
   (eglot--with-fixture
       `(("project" . (("foo.c" . "int foo() {return 42;}")
-                      ("bar.c" . "int bar() {return 42;}")))
-        (c-mode-hook (eglot-ensure)))
-    (let (server)
+                      ("bar.c" . "int bar() {return 42;}"))))
+    (let ((c-mode-hook '(eglot-ensure))
+          server)
       ;; need `ert-simulate-command' because `eglot-ensure'
       ;; relies on `post-command-hook'.
       (with-current-buffer
@@ -1331,8 +1309,9 @@ eglot-test-same-server-multi-mode
         (should (eq (eglot-current-server) server))))))
 
 (provide 'eglot-tests)
-;;; eglot-tests.el ends here
 
 ;; Local Variables:
 ;; checkdoc-force-docstrings-flag: nil
 ;; End:
+
+;;; eglot-tests.el ends here
-- 
2.39.1


[-- Attachment #7: Type: text/plain, Size: 3431 bytes --]


WDYT?  And should any fixes go to emacs-29 or master?

Thanks,

-- 
Basil

$ pylsp --version
pylsp v1.7.1
$ autopep8 --version
autopep8 2.0.1 (pycodestyle: 2.10.0)
$ cargo --version
cargo 1.67.1 (8ecd4f20a 2023-01-10)
$ rust-analyzer --version
rust-analyzer 0.3.1402-standalone

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2023-02-19 built on tia
Repository revision: 8fba4cff1bd0b953af9e950e872e1eaecff179d7
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure 'CFLAGS=-Og -ggdb3' -C --prefix=/home/blc/.local
 --enable-checking=structs --with-file-notification=yes
 --with-x-toolkit=lucid --with-x'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XAW3D XDBE XIM XINPUT2 XPM
LUCID ZLIB

Important settings:
  value of $LANG: en_IE.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 36554 8763)
 (symbols 48 5169 0)
 (strings 32 13853 1554)
 (string-bytes 1 377508)
 (vectors 16 9296)
 (vector-slots 8 147812 13111)
 (floats 8 23 25)
 (intervals 56 245 0)
 (buffers 984 10))

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Hi Basil,

Both patches look generally good, though I haven't tested them,
so I'm trusting you did and they generally add robustness.

If that's the case, go ahead and push (to emacs-29, presumably
since this is fixing bugs in the test suite, but master isn't that
bad either).

The only nit I would point out is that there seems to be some
unrelated housekeeping already in the first patch that could
be moved to the second patch, or maybe a separate commit.

The yasnippet-related fix could also be its own commit.  But
again, that's only a minor nit.

Thanks,
João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

João Távora [2023-02-19 19:38 +0000] wrote:

> Both patches look generally good, though I haven't tested them,
> so I'm trusting you did and they generally add robustness.

Thanks, indeed 'make check' now/still succeeds locally.

> If that's the case, go ahead and push (to emacs-29, presumably
> since this is fixing bugs in the test suite, but master isn't that
> bad either).

The small change in ert-x.el, and the proximity to an emacs-29 RC, are
the reasons I wanted to double check with Eli about where these changes
should go.

Perhaps I should split out the ert-x.el change for master, and the rest
can go to emacs-29?

> The only nit I would point out is that there seems to be some
> unrelated housekeeping already in the first patch that could
> be moved to the second patch, or maybe a separate commit.

You mean, the indentation and commentary fixes?  The impression I got is
that these kinds of changes are more welcome in emacs.git when the
surrounding code is already being touched, as opposed to making small
whitespace-only changes to functions that are not otherwise being
changed.

I have little personal preference either way.

> The yasnippet-related fix could also be its own commit.  But
> again, that's only a minor nit.

Sure, I can break out the autopep8 and YASnippet changes if that's
preferred.

Thanks,

-- 
Basil

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

On Mon, Feb 20, 2023 at 9:22 AM Basil L. Contovounesios <contovob@tcd.ie> wrote:
>
> João Távora [2023-02-19 19:38 +0000] wrote:

> > The only nit I would point out is that there seems to be some
> > unrelated housekeeping already in the first patch that could
> > be moved to the second patch, or maybe a separate commit.
>
> You mean, the indentation and commentary fixes?  The impression I got is
> that these kinds of changes are more welcome in emacs.git when the
> surrounding code is already being touched, as opposed to making small
> whitespace-only changes to functions that are not otherwise being
> changed.

Those cosmetic changes are most welcome.  But my personal
preference is to do them in a separate commit, so that when you
inspect and try to understand a "functional" commit, you are
presented with only just behavior-changing changes, which
makes the commit easier to understand.

But this is just a nit.

> I have little personal preference either way.
>
> > The yasnippet-related fix could also be its own commit.  But
> > again, that's only a minor nit.
>
> Sure, I can break out the autopep8 and YASnippet changes if that's
> preferred.

Again, this is just a nit.

João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Eli Zaretskii]

> Cc: 61637@debbugs.gnu.org
> Date: Mon, 20 Feb 2023 09:22:36 +0000
> From:  "Basil L. Contovounesios" via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> João Távora [2023-02-19 19:38 +0000] wrote:
> 
> > Both patches look generally good, though I haven't tested them,
> > so I'm trusting you did and they generally add robustness.
> 
> Thanks, indeed 'make check' now/still succeeds locally.
> 
> > If that's the case, go ahead and push (to emacs-29, presumably
> > since this is fixing bugs in the test suite, but master isn't that
> > bad either).
> 
> The small change in ert-x.el, and the proximity to an emacs-29 RC, are
> the reasons I wanted to double check with Eli about where these changes
> should go.
> 
> Perhaps I should split out the ert-x.el change for master, and the rest
> can go to emacs-29?

Please install on master, but please let's first discuss the solution,
as I don't like us touch the user's home directory during tests.

Thanks.

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Hi Eli, Basil

I've recently bumped into this problem myself.  As I've been doing more
development on Eglot recently, running the test suite becomes a real
necessity.  

I installed some language servers like clangd, rust-analyzer and pylsp,
and they need to a valid HOME to be able to function.  Bare 'make check'
always fails for me as of late.  If one happens to have the servers
installed, the problem is twofold:

* For Eglot developers, it's impossible to batch-test Eglot

* For other devs who are doing changes elsewhere and looking for
  breakage, they'll have to reason about the 'make check' failure.

Another option is to have eglot-tests.el read a new environment var:

diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
index 5d5de59a19a..ad994915c52 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -116,9 +116,7 @@ eglot--call-with-fixture
                    ;; installations for LSP servers like pylsp, making
                    ;; these tests mostly useless, so we hack around it
                    ;; here with a great big hack.
-                   ,(format "HOME=%s"
-                            (if (file-exists-p home) home
-                              (format "/home/%s" (getenv "USER")))))
+                   ,(format "HOME=%s" (or (getenv "EGLOT_REAL_HOME") (getenv "HOME"))))
                  process-environment))
                ;; Prevent "Can't guess python-indent-offset ..." messages.
                (python-indent-guess-indent-offset-verbose . nil)

This is acceptable for Eglot devs like me, but other devs that don't
know about this variable will still be confused. WDYT?

> Why not do this the other way around: trick Eglot to think the home
> directory doesn't exist (as I understand it already knows how to
> handle this)?

I don't understand what you mean: this is entirely constrained to how
certain language servers work, so eglot.el itself can't handle it.

João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,  61637@debbugs.gnu.org
> Date: Sat, 04 Mar 2023 01:04:45 +0000
> 
> I installed some language servers like clangd, rust-analyzer and pylsp,
> and they need to a valid HOME to be able to function.

Please tell more about this: what exactly do these servers need from
the home directory?

> diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
> index 5d5de59a19a..ad994915c52 100644
> --- a/test/lisp/progmodes/eglot-tests.el
> +++ b/test/lisp/progmodes/eglot-tests.el
> @@ -116,9 +116,7 @@ eglot--call-with-fixture
>                     ;; installations for LSP servers like pylsp, making
>                     ;; these tests mostly useless, so we hack around it
>                     ;; here with a great big hack.
> -                   ,(format "HOME=%s"
> -                            (if (file-exists-p home) home
> -                              (format "/home/%s" (getenv "USER")))))
> +                   ,(format "HOME=%s" (or (getenv "EGLOT_REAL_HOME") (getenv "HOME"))))
>                   process-environment))
>                 ;; Prevent "Can't guess python-indent-offset ..." messages.
>                 (python-indent-guess-indent-offset-verbose . nil)
> 
> This is acceptable for Eglot devs like me, but other devs that don't
> know about this variable will still be confused. WDYT?

As I said, I don't like the idea of using the user's real home
directory for test purposes.  We could end up clobbering precious
files there.  We could also have tests fail because some user setting
in the home directory makes the test results unpredictable.

> > Why not do this the other way around: trick Eglot to think the home
> > directory doesn't exist (as I understand it already knows how to
> > handle this)?
> 
> I don't understand what you mean: this is entirely constrained to how
> certain language servers work, so eglot.el itself can't handle it.

In the original report, Basil sais:

> The main problem is that:
> 0. test/Makefile.in sets HOME=/nonexistent
> 1. lisp/emacs-lisp/ert-x.el sets HOME=/tmp :(
> 2. eglot--call-with-fixture tries to detect when HOME is nonexistent,
>    but /tmp exists, so that's left unchanged
> 3. The Rust tools look under HOME=/tmp for which toolchain to use, but
>    the answer is under ~USER, so they give up

I interpreted item 2 as meaning that eglot--call-with-fixture can cope
with the situation where the user's home directory doesn't exist, and
suggested to trick Eglot into thinking it doesn't exist in the cases
in point, as the means of solving the problem.

If my understanding is incorrect, or if what you describe here is a
different problem (thus item 2 is no longer relevant), I'd like to
better understand the details, so we could see if there are better
alternatives than to use the real home directory of the user running
the tests.

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,  61637@debbugs.gnu.org
>> Date: Sat, 04 Mar 2023 01:04:45 +0000
>> 
>> I installed some language servers like clangd, rust-analyzer and pylsp,
>> and they need to a valid HOME to be able to function.
>
> Please tell more about this: what exactly do these servers need from
> the home directory?

Every server is different.  This is the complain for a pylsp installed
with 'pip install pylsp' for the regular user, which you can find in the
output of e.g.

   make -C test lisp/progmodes/eglot-tests SELECTOR='"basic-completions"'

This are the relevant parts of the output.  Here, it looks like the
pylsp trampoline in ~/.local/bin/pylsp can't find the main supporing
library, which probably lives around the corner in
~/.local/lib/python3.10/site-packages.

   [stderr] PWD: /tmp/eglot--fixturelfcOUE/project
   [stderr] XDG_CONFIG_HOME: /dev/null
   [stderr] HOME: /tmp/
   ....
   [stderr] EMACS_TEST_DIRECTORY: /home/capitaomorte/Source/Emacs/emacs/test
   [stderr] Traceback (most recent call last):
   [stderr]   File "/home/capitaomorte/.local/bin/pylsp", line 14, in <module>
   [stderr]     from pylsp.__main__ import main
   [stderr] ModuleNotFoundError: No module named 'pylsp'
   [internal] Sat Mar  4 11:31:48 2023:
   (:message "Connection state changed" :change "exited abnormally with code 1\n")
    
   ----------b---y---e---b---y---e----------
   [eglot] Killing (something.py), wiping /tmp/eglot--fixturelfcOUE, restoring nil
   Test eglot-test-basic-completions backtrace:
     signal(error ("[eglot] -1: Server died"))

I suppose basil found something similar for rust-analyzer, though my
installation of it is system-wide, so I don't get that particular
problem.

>> This is acceptable for Eglot devs like me, but other devs that don't
>> know about this variable will still be confused. WDYT?
>
> As I said, I don't like the idea of using the user's real home
> directory for test purposes.  We could end up clobbering precious
> files there.  We could also have tests fail because some user setting
> in the home directory makes the test results unpredictable.

As I understand it, the concern of cloberring user customizations is
mostly related to Emacs' own packages like ido or auto-save-mode, some
of them do write files in ~/.emacs.d and similar.  That is reasonable.

But this is different IMO.  We're talking about user-installed language
servers, which presumably these users are already using (because they
installed them).  Only for the specific invocations of these servers is
HOME spoofed.  Overall I think the risk is low.  Eglot has had these
types of tests since practically the beginning and I've never had
complains of clobbered files.

>> > Why not do this the other way around: trick Eglot to think the home
>> > directory doesn't exist (as I understand it already knows how to
>> > handle this)?
>> 
>> I don't understand what you mean: this is entirely constrained to how
>> certain language servers work, so eglot.el itself can't handle it.
>
> In the original report, Basil sais:
>
>> The main problem is that:
>> 0. test/Makefile.in sets HOME=/nonexistent
>> 1. lisp/emacs-lisp/ert-x.el sets HOME=/tmp :(
>> 2. eglot--call-with-fixture tries to detect when HOME is nonexistent,
>>    but /tmp exists, so that's left unchanged
>> 3. The Rust tools look under HOME=/tmp for which toolchain to use, but
>>    the answer is under ~USER, so they give up
>
> I interpreted item 2 as meaning that eglot--call-with-fixture can cope
> with the situation where the user's home directory doesn't exist

Ah, I understand.  Indeed it could "cope", but only by doing exactly the
same kind of spoof (or rather, unspoof) that Basil was proposing.  But
at a certain point in Emacs-29 that unspoof stopped working.

In fact, I missed this in Basil's patch, but I think the correct fix is
actually to fix eglot--call-with-fixture directly, with Basil's better
technique of re-constructing $HOME.

diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
index 5d5de59a19a..5f5adc8e5b3 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -103,22 +103,21 @@ eglot--call-with-fixture
              (set (car spec) (cadr spec)))
             ((stringp (car spec)) (push spec file-specs))))
     (unwind-protect
-        (let* ((home (getenv "HOME"))
-               (process-environment
+        (let* ((process-environment
                 (append
                  `(;; Set XDF_CONFIG_HOME to /dev/null to prevent
-                   ;; user-configuration to have an influence on
+                   ;; user's configuration to have an influence on
                    ;; language servers. (See github#441)
                    "XDG_CONFIG_HOME=/dev/null"
                    ;; ... on the flip-side, a similar technique by
                    ;; Emacs's test makefiles means that HOME is set to
-                   ;; /nonexistent.  This breaks some common
-                   ;; installations for LSP servers like pylsp, making
-                   ;; these tests mostly useless, so we hack around it
-                   ;; here with a great big hack.
-                   ,(format "HOME=%s"
-                            (if (file-exists-p home) home
-                              (format "/home/%s" (getenv "USER")))))
+                   ;; /nonexistent or /tmp/.  This breaks some common
+                   ;; installations for LSP servers like pylsp, and
+                   ;; rust-analyzer, making these tests mostly
+                   ;; useless, so we hack around it here with a great
+                   ;; big hack.
+                   ,(format "HOME=%s" (expand-file-name (concat
+                                                         "~" (user-login-name)))))
                  process-environment))
                ;; Prevent "Can't guess python-indent-offset ..." messages.
                (python-indent-guess-indent-offset-verbose . nil)

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: contovob@tcd.ie,  61637@debbugs.gnu.org
> Date: Sat, 04 Mar 2023 11:48:27 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: João Távora <joaotavora@gmail.com>
> >> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,  61637@debbugs.gnu.org
> >> Date: Sat, 04 Mar 2023 01:04:45 +0000
> >> 
> >> I installed some language servers like clangd, rust-analyzer and pylsp,
> >> and they need to a valid HOME to be able to function.
> >
> > Please tell more about this: what exactly do these servers need from
> > the home directory?
> 
> Every server is different.  This is the complain for a pylsp installed
> with 'pip install pylsp' for the regular user, which you can find in the
> output of e.g.
> 
>    make -C test lisp/progmodes/eglot-tests SELECTOR='"basic-completions"'
> 
> This are the relevant parts of the output.  Here, it looks like the
> pylsp trampoline in ~/.local/bin/pylsp can't find the main supporing
> library, which probably lives around the corner in
> ~/.local/lib/python3.10/site-packages.

Is it reasonable to have an LSP installed in the HOME directory when
running the Emacs test suite, when we clearly document that HOME is
ignored for these tests?

How about if we ask users who install LSP servers under their home
directory to somehow specify the exact location of that installation,
so that the LSP will find its components, but Emacs won't access any
files in the user's HOME via the "~" shortcut?

> > As I said, I don't like the idea of using the user's real home
> > directory for test purposes.  We could end up clobbering precious
> > files there.  We could also have tests fail because some user setting
> > in the home directory makes the test results unpredictable.
> 
> As I understand it, the concern of cloberring user customizations is
> mostly related to Emacs' own packages like ido or auto-save-mode, some
> of them do write files in ~/.emacs.d and similar.  That is reasonable.
> 
> But this is different IMO.  We're talking about user-installed language
> servers, which presumably these users are already using (because they
> installed them).  Only for the specific invocations of these servers is
> HOME spoofed.  Overall I think the risk is low.  Eglot has had these
> types of tests since practically the beginning and I've never had
> complains of clobbered files.

You disregarded the second part of my reasoning, which has to do with
the test results being non-deterministic once the user's real home
directory is accessible to Emacs.  How do we overcome that?

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: contovob@tcd.ie,  61637@debbugs.gnu.org
>> Date: Sat, 04 Mar 2023 11:48:27 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: João Távora <joaotavora@gmail.com>
>> >> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,  61637@debbugs.gnu.org
>> >> Date: Sat, 04 Mar 2023 01:04:45 +0000
>> >> 
>> >> I installed some language servers like clangd, rust-analyzer and pylsp,
>> >> and they need to a valid HOME to be able to function.
>> >
>> > Please tell more about this: what exactly do these servers need from
>> > the home directory?
>> 
>> Every server is different.  This is the complain for a pylsp installed
>> with 'pip install pylsp' for the regular user, which you can find in the
>> output of e.g.
>> 
>>    make -C test lisp/progmodes/eglot-tests SELECTOR='"basic-completions"'
>> 
>> This are the relevant parts of the output.  Here, it looks like the
>> pylsp trampoline in ~/.local/bin/pylsp can't find the main supporing
>> library, which probably lives around the corner in
>> ~/.local/lib/python3.10/site-packages.
>
> Is it reasonable to have an LSP installed in the HOME directory when
> running the Emacs test suite, when we clearly document that HOME is
> ignored for these tests?

It's rather two questions from my perspective:

* Is there an easy way to install the pylsp server that _isn't_ this
  HOME directory installation so that Eglot devs can run the tests?
  There doesn't seem to be, at least not easy.  Furthermore I (and I
  suppose many others) prefer installing toolchains in unpriviliged
  places rather than priviliged system ones.

* If people do have this installed in HOME, it it reasonable to
  downright fail in their faces.

I think that, to be consistent with this HOME spoofing (which I find a
bit draconian in this case) then Emacs should also spoof PATH so that
these HOME programs aren't found in the first place.  

It comes down to what are the intended semantics of finding external
executable during Emacs 'make check'.  Only system-wide executables?
Should 'make check' chroot itself or something?  Should we be trying to
solve the very hard problems of determinism at all?  Shouldn't that be
left to a broader sandboxing solution, or a chrooting solution like
docker?

> How about if we ask users who install LSP servers under their home
> directory to somehow specify the exact location of that installation,
> so that the LSP will find its components, but Emacs won't access any
> files in the user's HOME via the "~" shortcut?

How do you propose we do that?  What users exactly?  Eglot-interested
devs?  What about the others working on libraries and trying not to
break things?  When should users do that specifying?  Isn't it different
for each server and each installation method?

>> > As I said, I don't like the idea of using the user's real home
>> > directory for test purposes.  We could end up clobbering precious
>> > files there.  We could also have tests fail because some user setting
>> > in the home directory makes the test results unpredictable.
>> 
>> As I understand it, the concern of cloberring user customizations is
>> mostly related to Emacs' own packages like ido or auto-save-mode, some
>> of them do write files in ~/.emacs.d and similar.  That is reasonable.
>> 
>> But this is different IMO.  We're talking about user-installed language
>> servers, which presumably these users are already using (because they
>> installed them).  Only for the specific invocations of these servers is
>> HOME spoofed.  Overall I think the risk is low.  Eglot has had these
>> types of tests since practically the beginning and I've never had
>> complains of clobbered files.
>
> You disregarded the second part of my reasoning, which has to do with
> the test results being non-deterministic once the user's real home
> directory is accessible to Emacs.  How do we overcome that?

Sorry I didn't mean to disregard, I just missed it.  Well, it's not
accessible to Emacs, only to LSP servers.  Eglot's tests are fairly are
deterministic, all other things like LSP server versions being equal.

I'd say Eglot's tests are even robust to any changes in LSP server's
user init files.  See how XDG_CONFIG_HOME is spooffed in eglot-tests.el
to this effect.  It has been effective AFAICT for some time.  And that's
only for those few servers that do offer these mechanisms: most of them
rely on project-local configurations which is fine because Eglot creates
a temporary project for each test.

All in all, I think the problem of user file clobbering and determinism
are exaggerated -- in this specific case of course.  They are secondary
to the fact that it's tricky, noisy, or even impossible to run Eglot
tests ever since Eglot moved to Emacs core.  

I propose we install my patch and then perfect the solution.  Again,
we're only affecting those devs who _do_ have these local installations,
and that affection is much, much more likely to be beneficial than
harmful.

João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: contovob@tcd.ie,  61637@debbugs.gnu.org
> Date: Sat, 04 Mar 2023 13:23:32 +0000
> 
> > You disregarded the second part of my reasoning, which has to do with
> > the test results being non-deterministic once the user's real home
> > directory is accessible to Emacs.  How do we overcome that?
> 
> Sorry I didn't mean to disregard, I just missed it.  Well, it's not
> accessible to Emacs, only to LSP servers.  Eglot's tests are fairly are
> deterministic, all other things like LSP server versions being equal.
> 
> I'd say Eglot's tests are even robust to any changes in LSP server's
> user init files.  See how XDG_CONFIG_HOME is spooffed in eglot-tests.el
> to this effect.  It has been effective AFAICT for some time.  And that's
> only for those few servers that do offer these mechanisms: most of them
> rely on project-local configurations which is fine because Eglot creates
> a temporary project for each test.
> 
> All in all, I think the problem of user file clobbering and determinism
> are exaggerated -- in this specific case of course.  They are secondary
> to the fact that it's tricky, noisy, or even impossible to run Eglot
> tests ever since Eglot moved to Emacs core.  
> 
> I propose we install my patch and then perfect the solution.  Again,
> we're only affecting those devs who _do_ have these local installations,
> and that affection is much, much more likely to be beneficial than
> harmful.

OK, please go ahead, and thanks.

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Basil Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

João Távora [2023-03-04 13:23 +0000] wrote:

> I'd say Eglot's tests are even robust to any changes in LSP server's
> user init files.  See how XDG_CONFIG_HOME is spooffed in eglot-tests.el
> to this effect.

Based on the test failures I've been seeing for months and tried to
address in the original patches (not all of which are due to an
unexpected HOME setting), it feels like that's probably too optimistic a
conclusion in the general case.

A lot of the relevant tooling places its configuration directly under
HOME for example, not under XDG_CONFIG_HOME.

I can also imagine user-specific settings affecting some of the
auto-formatting functionality tested in eglot-tests.el.

-- 
Basil

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Basil Contovounesios <contovob@tcd.ie> writes:

> Based on the test failures I've been seeing for months and tried to
> address in the original patches (not all of which are due to an
> unexpected HOME setting), it feels like that's probably too optimistic a
> conclusion in the general case.

If you have described those somewhere, I haven't had a chance to look,
but I'm interested.

Anyway, the other changes of your original patch are fine.  You can
just push them if they make tests more robust.

> A lot of the relevant tooling places its configuration directly under
> HOME for example, not under XDG_CONFIG_HOME.

As far as I recall, the only LSP server that relied significantly on
user-wide configuration is pyls, and it was fixed via XDG_CONFIG_HOME.
When that change made it in (in the old Github repo), this was the
commit message:

    Close #441: shield tests from some user customizations
    
    Users' customization of Python indenting style in the standard
    XDG_CONFIG_HOME location of ~/.config/pycodestyle could cause spurious
    test failures. We prevent this and similar problems by overriding that
    environment variable in tests. If this turns out to hurt other
    language servers used in the test suite, we'll have to revisit.

See https://github.com/joaotavora/eglot/issues/441 for more.

I can't recall other servers -- used in eglot-tests.el -- that suffer
from the same effect (at least to the point of that effect being
relevant).

> I can also imagine user-specific settings affecting some of the
> auto-formatting functionality tested in eglot-tests.el.

Yes, but as I said these things are much more commonly set per-project
(via eglot-workspace-configuration) or on server invocation (via
:initializationOptions) than in a user HOME directory.  Makes sense, no?
Every project has its practices, etc.

João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Basil Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii [2023-03-04 14:46 +0200] wrote:

> Is it reasonable to have an LSP installed in the HOME directory when
> running the Emacs test suite, when we clearly document that HOME is
> ignored for these tests?

The fact that I have LSP servers installed under HOME is orthogonal to
the fact that I also happen to frequently run the Emacs test suite,
regardless of what the Emacs test suite chooses to do with the HOME
environment variable.

The purpose of this bug report is to teach eglot-tests.el to gracefully
handle such installations, which IME are increasingly common.

I'm not particularly bothered whether that's achieved by filtering
exec-path, skipping problematic tests, or...

> How about if we ask users who install LSP servers under their home
> directory to somehow specify the exact location of that installation,
> so that the LSP will find its components, but Emacs won't access any
> files in the user's HOME via the "~" shortcut?

...some such mechanism of specifying a HOME-like substitute, like João's
earlier EGLOT_REAL_HOME suggestion.

>> > As I said, I don't like the idea of using the user's real home
>> > directory for test purposes.  We could end up clobbering precious
>> > files there.  We could also have tests fail because some user setting
>> > in the home directory makes the test results unpredictable.
>> 
>> As I understand it, the concern of cloberring user customizations is
>> mostly related to Emacs' own packages like ido or auto-save-mode, some
>> of them do write files in ~/.emacs.d and similar.  That is reasonable.
>> 
>> But this is different IMO.  We're talking about user-installed language
>> servers, which presumably these users are already using (because they
>> installed them).  Only for the specific invocations of these servers is
>> HOME spoofed.  Overall I think the risk is low.  Eglot has had these
>> types of tests since practically the beginning and I've never had
>> complains of clobbered files.
>
> You disregarded the second part of my reasoning, which has to do with
> the test results being non-deterministic once the user's real home
> directory is accessible to Emacs.  How do we overcome that?

Any of the suggestions I've enumerated so far sounds plausible to me.
But I'd appreciate some feedback on which combination if any is
preferred before proceeding to suggest a new patchset (unless João beats
me to it).

Thanks,

-- 
Basil

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Basil Contovounesios <contovob@tcd.ie> writes:

> Any of the suggestions I've enumerated so far sounds plausible to me.
> But I'd appreciate some feedback on which combination if any is
> preferred before proceeding to suggest a new patchset (unless João beats
> me to it).

Eli has already said that he is OK with restoring the HOME unspoofing in
eglot--call-with-fixture unconditionally, so I've just pushed that.

'make check' now runs consistently with 0 failures on emacs-29.

But I'm still interested in your other cleanups, like more idiomatic
code, whitespace fixes, etc.  So feel free to show a patchset for that
or just push directly to emacs-29 (or master).

João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Basil Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

close 61637 30.1
quit

João Távora [2023-03-04 19:22 +0000] wrote:

> But I'm still interested in your other cleanups, like more idiomatic
> code, whitespace fixes, etc.  So feel free to show a patchset for that
> or just push directly to emacs-29 (or master).

Now pushed to master, and closing.

Thanks,

-- 
Basil

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Basil Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

João Távora [2023-03-04 11:48 +0000] wrote:

> In fact, I missed this in Basil's patch, but I think the correct fix is
> actually to fix eglot--call-with-fixture directly

I'm not sure what you mean by 'directly': the patch below
unconditionally sets HOME=~USER for all eglot-tests.el, whereas the
patch in the OP was more conservative in doing this for only those tests
which are found to need it, thus mitigating to an extent the risk that
Eli is (understandably) concerned about.

> diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
> index 5d5de59a19a..5f5adc8e5b3 100644
> --- a/test/lisp/progmodes/eglot-tests.el
> +++ b/test/lisp/progmodes/eglot-tests.el
> @@ -103,22 +103,21 @@ eglot--call-with-fixture
>               (set (car spec) (cadr spec)))
>              ((stringp (car spec)) (push spec file-specs))))
>      (unwind-protect
> -        (let* ((home (getenv "HOME"))
> -               (process-environment
> +        (let* ((process-environment
>                  (append
>                   `(;; Set XDF_CONFIG_HOME to /dev/null to prevent
> -                   ;; user-configuration to have an influence on
> +                   ;; user's configuration to have an influence on
>                     ;; language servers. (See github#441)
>                     "XDG_CONFIG_HOME=/dev/null"

Note that test/Makefile.in already unexports XDG_CONFIG_HOME, so
eglot-tests.el needs not concern itself with it.

>                     ;; ... on the flip-side, a similar technique by
>                     ;; Emacs's test makefiles means that HOME is set to
> -                   ;; /nonexistent.  This breaks some common
> -                   ;; installations for LSP servers like pylsp, making
> -                   ;; these tests mostly useless, so we hack around it
> -                   ;; here with a great big hack.
> -                   ,(format "HOME=%s"
> -                            (if (file-exists-p home) home
> -                              (format "/home/%s" (getenv "USER")))))
> +                   ;; /nonexistent or /tmp/.  This breaks some common
> +                   ;; installations for LSP servers like pylsp, and
> +                   ;; rust-analyzer, making these tests mostly
> +                   ;; useless, so we hack around it here with a great
> +                   ;; big hack.
> +                   ,(format "HOME=%s" (expand-file-name (concat
> +                                                         "~" (user-login-name)))))
>                   process-environment))
>                 ;; Prevent "Can't guess python-indent-offset ..." messages.
>                 (python-indent-guess-indent-offset-verbose . nil)

-- 
Basil

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Basil Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii [2023-03-04 09:46 +0200] wrote:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,  61637@debbugs.gnu.org
>> Date: Sat, 04 Mar 2023 01:04:45 +0000
>> 
>> I installed some language servers like clangd, rust-analyzer and pylsp,
>> and they need to a valid HOME to be able to function.
>
> Please tell more about this: what exactly do these servers need from
> the home directory?

Binaries, configuration files, virtual environments, the whole shebang.
It seems increasingly common to install such fast-moving tooling under
the user's home/local directory without root privileges using
language-specific package managers.

>> diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
>> index 5d5de59a19a..ad994915c52 100644
>> --- a/test/lisp/progmodes/eglot-tests.el
>> +++ b/test/lisp/progmodes/eglot-tests.el
>> @@ -116,9 +116,7 @@ eglot--call-with-fixture
>>                     ;; installations for LSP servers like pylsp, making
>>                     ;; these tests mostly useless, so we hack around it
>>                     ;; here with a great big hack.
>> -                   ,(format "HOME=%s"
>> -                            (if (file-exists-p home) home
>> -                              (format "/home/%s" (getenv "USER")))))
>> +                   ,(format "HOME=%s" (or (getenv "EGLOT_REAL_HOME") (getenv "HOME"))))
>>                   process-environment))
>>                 ;; Prevent "Can't guess python-indent-offset ..." messages.
>>                 (python-indent-guess-indent-offset-verbose . nil)
>> 
>> This is acceptable for Eglot devs like me, but other devs that don't
>> know about this variable will still be confused. WDYT?
>
> As I said, I don't like the idea of using the user's real home
> directory for test purposes.  We could end up clobbering precious
> files there.  We could also have tests fail because some user setting
> in the home directory makes the test results unpredictable.

Then I see three options in this discussion so far:

0. Whenever the tooling in question is detected to fail because it is
   being run under /nonexistent or /tmp, the test should be skipped
   rather than considered a failure.

1. Allow adventurous devs to opt into having their real HOME (or any
   provided substitute thereof) used, presumably using either ERT :tags,
   an environment variable like that which João suggested above, or
   something similar.

2. Filter exec-path to exclude user-specific directories.

None of which sound mutually exclusive to me.  Item 2 is the most
drastic/limiting one, but it complements the existing policy of the
Emacs test suite to modify HOME and unexport XDG_CONFIG_HOME.

I am personally not considering the other mentioned possibilities like
sandboxing, chrooting, containerising, etc. because I am not familiar
with them and have neither the time nor motivation to pursue such
changes to the Eglot test suite.

>> > Why not do this the other way around: trick Eglot to think the home
>> > directory doesn't exist (as I understand it already knows how to
>> > handle this)?
>> 
>> I don't understand what you mean: this is entirely constrained to how
>> certain language servers work, so eglot.el itself can't handle it.
>
> In the original report, Basil sais:
>
>> The main problem is that:
>> 0. test/Makefile.in sets HOME=/nonexistent
>> 1. lisp/emacs-lisp/ert-x.el sets HOME=/tmp :(
>> 2. eglot--call-with-fixture tries to detect when HOME is nonexistent,
>>    but /tmp exists, so that's left unchanged
>> 3. The Rust tools look under HOME=/tmp for which toolchain to use, but
>>    the answer is under ~USER, so they give up
>
> I interpreted item 2 as meaning that eglot--call-with-fixture can cope
> with the situation where the user's home directory doesn't exist, and
> suggested to trick Eglot into thinking it doesn't exist in the cases
> in point, as the means of solving the problem.

That's too optimistic an interpretation of what I said, I'm afraid: it
is precisely when HOME does not exist that eglot--call-with-fixture
currently decides to use the user's real home directory.

Normally HOME would not exist during our test suite, except
eglot-tests.el loads ert-x.el, which conditionally sets HOME=/tmp at
top-level (see the definition of ert-remote-temporary-file-directory).

So on my system eglot-tests.el are currently run under /tmp, but on
systems where ert-remote-temporary-file-directory has not messed with
the definition of HOME, they could be running under /nonexistent or the
user's real HOME.

AFAICT João's latest suggestion is to unconditionally use the real HOME
for all eglot-tests.el.

> If my understanding is incorrect, or if what you describe here is a
> different problem (thus item 2 is no longer relevant), I'd like to
> better understand the details, so we could see if there are better
> alternatives than to use the real home directory of the user running
> the tests.

See my comments above.

Thanks,

-- 
Basil

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Basil Contovounesios <contovob@tcd.ie> writes:

> Normally HOME would not exist during our test suite, except
> eglot-tests.el loads ert-x.el, which conditionally sets HOME=/tmp at
> top-level (see the definition of ert-remote-temporary-file-directory).

Ahhh...  so that's why the unspoofing technique stopped working
recently.  Presumably that is a recent change.  If so, that clears
up the mistery.  Thanks!

João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Basil Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

João Távora [2023-03-04 01:04 +0000] wrote:

> Another option is to have eglot-tests.el read a new environment var:
>
> diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
> index 5d5de59a19a..ad994915c52 100644
> --- a/test/lisp/progmodes/eglot-tests.el
> +++ b/test/lisp/progmodes/eglot-tests.el
> @@ -116,9 +116,7 @@ eglot--call-with-fixture
>                     ;; installations for LSP servers like pylsp, making
>                     ;; these tests mostly useless, so we hack around it
>                     ;; here with a great big hack.
> -                   ,(format "HOME=%s"
> -                            (if (file-exists-p home) home
> -                              (format "/home/%s" (getenv "USER")))))
> +                   ,(format "HOME=%s" (or (getenv "EGLOT_REAL_HOME") (getenv "HOME"))))
>                   process-environment))
>                 ;; Prevent "Can't guess python-indent-offset ..." messages.
>                 (python-indent-guess-indent-offset-verbose . nil)
>
> This is acceptable for Eglot devs like me, but other devs that don't
> know about this variable will still be confused. WDYT?

So the suggestion is for parties interested in running the full
unadulterated Eglot suite to set this environment variable, thus opting
into using their HOME or any other HOME-like environment they may have
set up?

If so, sounds fine to me, thanks,

-- 
Basil

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Eli Zaretskii]

> From: Basil Contovounesios <contovob@tcd.ie>
> Cc: Eli Zaretskii <eliz@gnu.org>,  61637@debbugs.gnu.org
> Date: Sat, 04 Mar 2023 17:19:26 +0000
> 
> João Távora [2023-03-04 01:04 +0000] wrote:
> 
> > Another option is to have eglot-tests.el read a new environment var:
> >
> > diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
> > index 5d5de59a19a..ad994915c52 100644
> > --- a/test/lisp/progmodes/eglot-tests.el
> > +++ b/test/lisp/progmodes/eglot-tests.el
> > @@ -116,9 +116,7 @@ eglot--call-with-fixture
> >                     ;; installations for LSP servers like pylsp, making
> >                     ;; these tests mostly useless, so we hack around it
> >                     ;; here with a great big hack.
> > -                   ,(format "HOME=%s"
> > -                            (if (file-exists-p home) home
> > -                              (format "/home/%s" (getenv "USER")))))
> > +                   ,(format "HOME=%s" (or (getenv "EGLOT_REAL_HOME") (getenv "HOME"))))
> >                   process-environment))
> >                 ;; Prevent "Can't guess python-indent-offset ..." messages.
> >                 (python-indent-guess-indent-offset-verbose . nil)
> >
> > This is acceptable for Eglot devs like me, but other devs that don't
> > know about this variable will still be confused. WDYT?
> 
> So the suggestion is for parties interested in running the full
> unadulterated Eglot suite to set this environment variable, thus opting
> into using their HOME or any other HOME-like environment they may have
> set up?

For process-environment only, i.e. for the LSP server that is being
started.  Presumably, not for Emacs itself.

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> So the suggestion is for parties interested in running the full
>> unadulterated Eglot suite to set this environment variable, thus opting
>> into using their HOME or any other HOME-like environment they may have
>> set up?
>
> For process-environment only, i.e. for the LSP server that is being
> started.  Presumably, not for Emacs itself.

That is correct.  Only for process-environment, and only within
'eglot--with-fixture'.

João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[João Távora]

Basil Contovounesios <contovob@tcd.ie> writes:

> João Távora [2023-03-04 01:04 +0000] wrote:
>
>> Another option is to have eglot-tests.el read a new environment var:
>>
>> diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
>> index 5d5de59a19a..ad994915c52 100644
>> --- a/test/lisp/progmodes/eglot-tests.el
>> +++ b/test/lisp/progmodes/eglot-tests.el
>> @@ -116,9 +116,7 @@ eglot--call-with-fixture
>>                     ;; installations for LSP servers like pylsp, making
>>                     ;; these tests mostly useless, so we hack around it
>>                     ;; here with a great big hack.
>> -                   ,(format "HOME=%s"
>> -                            (if (file-exists-p home) home
>> -                              (format "/home/%s" (getenv "USER")))))
>> +                   ,(format "HOME=%s" (or (getenv "EGLOT_REAL_HOME") (getenv "HOME"))))
>>                   process-environment))
>>                 ;; Prevent "Can't guess python-indent-offset ..." messages.
>>                 (python-indent-guess-indent-offset-verbose . nil)
>>
>> This is acceptable for Eglot devs like me, but other devs that don't
>> know about this variable will still be confused. WDYT?
>
> So the suggestion is for parties interested in running the full
> unadulterated Eglot suite to set this environment variable, thus opting
> into using their HOME or any other HOME-like environment they may have
> set up?
>
> If so, sounds fine to me, thanks,

Yes that would be the suggestion, but it still has considerable drawback
that non-Eglot devs looking just to check that they didn't break
anything and who _do_ happen to have these servers installed will still
see test failures in Eglot that are hard to diagnose.  They likely won't
know about EGLOT_REAL_HOME, and neither should they, really.

This solution would only be acceptable if PATH was _also_ spoofed (and
then unspoofed on request).  Bug spoofing PATH is a taller order, since
it's hard to know what are platform specific system paths and what are
"user paths".

João

bug#61637: 30.0.50; Fix Eglot tests that need HOME=~USER

[Eli Zaretskii]

> Cc: "João Távora" <joaotavora@gmail.com>
> Date: Sun, 19 Feb 2023 19:10:03 +0000
> From:  "Basil L. Contovounesios" via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> The main problem is that:
> 0. test/Makefile.in sets HOME=/nonexistent
> 1. lisp/emacs-lisp/ert-x.el sets HOME=/tmp :(
> 2. eglot--call-with-fixture tries to detect when HOME is nonexistent,
>    but /tmp exists, so that's left unchanged
> 3. The Rust tools look under HOME=/tmp for which toolchain to use, but
>    the answer is under ~USER, so they give up
> 
> The following patch temporarily sets HOME=~USER in only those tests that
> need it.  The patch consolidates some let-bindings into a single form to
> minimise collateral indentation changes.  It also bumps the
> rust-analyzer client/registerCapability timeout from 1 second to 3,
> because sometimes 1 second was too short a wait.  Finally, it updates
> the expected autopep8 reformatting results to what I see locally, and
> skips a test that fails without YASnippet.

I'd rather we didn't touch the real user home directory during tests,
it could do real damage or at the very least cause unintended changes
to the user customizations.

Why not do this the other way around: trick Eglot to think the home
directory doesn't exist (as I understand it already knows how to
handle this)?

> WDYT?  And should any fixes go to emacs-29 or master?

Master, please.  We have enough adventures on emacs-29 already.  And
this one is not really critical.