Subject: bug#38309: Recent $EMACSLOADPATH changes crash gnome-session

From: Maxim Cournoyer <[email-masked]>
Date: Wed, 27 Nov 2019 22:58:31 +0900

Hello Leo!

Leo Prikler <[email-masked]> writes:

> Hi everyone,
>
> Am Dienstag, den 26.11.2019, 09:56 +0100 schrieb Ludovic Courtès: 
>> are we not going overboard with that big a environment variable? :-)
> I think I vaguely remember a related discussion about the Emacs build
> system adding the guix.d directory, which further worsens this problem
> [1].  Putting that aside however, $EMACSLOADPATH should not contain
> more than
> - $GUIX_PROFILE/share/emacs/$EMACS_VERSION/lisp
> - $GUIX_PROFILE/share/emacs/$EMACS_VERSION/site-lisp
> - $GUIX_PROFILE/share/emacs/site-lisp
> If I read (elisp)Library Search correctly, these directories each
> contain a file to add their subdirectories to the load-path variable.  
> This can be confirmed by searching in the store or through message-
> debugging.  It appears, however, that these files are not quite
> sufficient.  While the load-path is indeed modified, no autoloading
> occurs for files inside guix.d -- indeed, I doubt it would occur for
> any package, regardless of how we name it.

That's a precious find!  I could validate your findings.  The only place
we don't have a union of the Elisp directories (with a subdirs.el file)
is at build time, but in the event we'd stop producing guix.d the search
path would work natively there (as well as causing any newly installed
libraries to be found without any rescanning of directories).

> After further digging around, this appears to be a bug in guix-emacs. 
> Rather than using the load-path variable, it uses $EMACSLOADPATH
> directly via getenv.  I suggest either recursing into subdirectories as
> Emacs itself would or using load-path instead of reverse engineering
> it, preferring the latter if applicable.
>
> Now that this has been cleared up, a fix should be in reach.  First we
> would fix guix-emacs, then we can restrict $EMACSLOADPATH to the above
> three -- perhaps two, as the versioned site-lisp appears unused.

Neat!  I find that this works best when guix.d is removed, as otherwise

1) relying on the load-path would mean we'd have to restart Emacs when
installed new libraries under guix.d directories (to have subdirs.el to
its magic and add them to the load-path)

2) the emacs-build-system simplifications that were made would need to
be reverted because at build time we don't have a profile with
subdirs.el readily available, and must manually hunt for the guix.d
subdirectories.

3) Even if we scanned directories recursively for autoloads from
EMACSLOADPATH ourselves in emacs-guix.el, a user would still need to
call the guix-emacs-autoload-packages manually after installing new
Elisp packages to have Emacs find them.

I've tested these changes with a Gnome VM and the EMACSLOADPATH is now
reduced to just the Emacs' lisp directory as well as the
share/emacs/site-lisp directory of any profile.  Thanks for the great
ideas :-).

Some packages would need to be adapted to finalize the move to a
guix.d-less installation directory (some recipes refer to it), but this
is trivial to do.  The documentation would need to be adapted as well.
I can take care of this if someones deems the attached patches fit to
fix the problems mentioned in this ticket.

From 141e7e8c45c39fbe2e6cfa879f1dc7b7f721bbfc Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <[email-masked]>
Date: Sat, 23 Nov 2019 12:04:50 +0900
Subject: [PATCH 1/6] build: emacs-build-system: Unify the installation
 directory.

This change aims to reduce the length of the EMACSLOADPATH environment
variable, which was found to cause issues such as bug
\#38309 (https://bugs.gnu.org/38309).

It should also enable discovery of newly installed packages without refreshing
the session's EMACSLOADPATH of the user profile (e.g., when launching Emacs
from the desktop manager application launcher), as discussed in bug
\#38309 (https://bugs.gnu.org/38309).

* guix/build/emacs-build-system.scm (%legacy-install-suffix): Rename to...
(%install-dir): ...this.
(%install-suffix): Remove variable.
(build): Adjust installation target directory.
(patch-el-files): Likewise.
(install): Likewise.
(move-doc): Likewise.
(make-autoloads): Likewise.
---
 guix/build/emacs-build-system.scm | 39 +++++++++++++------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index f0c41812f1..e2b792d3dc 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -40,11 +40,10 @@
 ;;
 ;; Code:
 
-;; Directory suffix where we install ELPA packages.  We avoid ".../elpa" as
-;; Emacs expects to find the ELPA repository 'archive-contents' file and the
-;; archive signature.
-(define %legacy-install-suffix "/share/emacs/site-lisp")
-(define %install-suffix (string-append %legacy-install-suffix "/guix.d"))
+;;; All the packages are installed directly under site-lisp, which means that
+;;; having that directory in the EMACSLOADPATH is enough to have them found by
+;;; Emacs.
+(define %install-dir "/share/emacs/site-lisp")
 
 ;; These are the default inclusion/exclusion regexps for the install phase.
 (define %default-include '("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
@@ -87,11 +86,10 @@ environment variable\n" source-directory)))
   "Compile .el files."
   (let* ((emacs (string-append (assoc-ref inputs "emacs") "/bin/emacs"))
          (out (assoc-ref outputs "out"))
-         (elpa-name-ver (store-directory->elpa-name-version out))
-         (el-dir (string-append out %install-suffix "/" elpa-name-ver)))
+         (site-lisp (string-append out %install-dir)))
     (setenv "SHELL" "sh")
     (parameterize ((%emacs emacs))
-      (emacs-byte-compile-directory el-dir))))
+      (emacs-byte-compile-directory site-lisp))))
 
 (define* (patch-el-files #:key outputs #:allow-other-keys)
   "Substitute the absolute \"/bin/\" directory with the right location in the
@@ -108,9 +106,7 @@ store in '.el' files."
       #:binary #t))
 
   (let* ((out (assoc-ref outputs "out"))
-         (elpa-name-ver (store-directory->elpa-name-version out))
-         (el-dir (string-append out %install-suffix "/" elpa-name-ver))
-
+         (site-lisp (string-append out %install-dir))
          ;; (ice-9 regex) uses libc's regexp routines, which cannot deal with
          ;; strings containing NULs.  Filter out such files.  TODO: Remove
          ;; this workaround when <https://bugs.gnu.org/30116> is fixed.
@@ -124,7 +120,7 @@ store in '.el' files."
              (error "patch-el-files: unable to locate " cmd-name))
            (string-append "\"" cmd "\"")))))
 
-    (with-directory-excursion el-dir
+    (with-directory-excursion site-lisp
       ;; Some old '.el' files (e.g., tex-buf.el in AUCTeX) are still
       ;; ISO-8859-1-encoded.
       (unless (false-if-exception (substitute-program-names))
@@ -175,15 +171,14 @@ parallel. PARALLEL-TESTS? is ignored when using a non-make TEST-COMMAND."
            (not (any (cut match-stripped-file "excluded" <>) exclude)))))
 
   (let* ((out (assoc-ref outputs "out"))
-         (elpa-name-ver (store-directory->elpa-name-version out))
-         (target-directory (string-append out %install-suffix "/" elpa-name-ver))
+         (site-lisp (string-append out %install-dir))
          (files-to-install (find-files source install-file?)))
     (cond
      ((not (null? files-to-install))
       (for-each
        (lambda (file)
          (let* ((stripped-file (string-drop file (string-length source)))
-                (target-file (string-append target-directory stripped-file)))
+                (target-file (string-append site-lisp stripped-file)))
            (format #t "`~a' -> `~a'~%" file target-file)
            (install-file file (dirname target-file))))
        files-to-install)
@@ -197,14 +192,12 @@ parallel. PARALLEL-TESTS? is ignored when using a non-make TEST-COMMAND."
 (define* (move-doc #:key outputs #:allow-other-keys)
   "Move info files from the ELPA package directory to the info directory."
   (let* ((out (assoc-ref outputs "out"))
-         (elpa-name-ver (store-directory->elpa-name-version out))
-         (el-dir (string-append out %install-suffix "/" elpa-name-ver))
-         (name-ver (strip-store-file-name out))
+         (site-lisp (string-append out %install-dir))
          (info-dir (string-append out "/share/info/"))
-         (info-files (find-files el-dir "\\.info$")))
+         (info-files (find-files site-lisp "\\.info$")))
     (unless (null? info-files)
       (mkdir-p info-dir)
-      (with-directory-excursion el-dir
+      (with-directory-excursion site-lisp
         (when (file-exists? "dir") (delete-file "dir"))
         (for-each (lambda (f)
                     (copy-file f (string-append info-dir "/" (basename f)))
@@ -216,11 +209,11 @@ parallel. PARALLEL-TESTS? is ignored when using a non-make TEST-COMMAND."
   "Generate the autoloads file."
   (let* ((emacs (string-append (assoc-ref inputs "emacs") "/bin/emacs"))
          (out (assoc-ref outputs "out"))
+         (site-lisp (string-append out %install-dir))
          (elpa-name-ver (store-directory->elpa-name-version out))
-         (elpa-name (package-name->name+version elpa-name-ver))
-         (el-dir (string-append out %install-suffix "/" elpa-name-ver)))
+         (elpa-name (package-name->name+version elpa-name-ver)))
     (parameterize ((%emacs emacs))
-      (emacs-generate-autoloads elpa-name el-dir))))
+      (emacs-generate-autoloads elpa-name site-lisp))))
 
 (define (emacs-package? name)
   "Check if NAME correspond to the name of an Emacs package."
-- 
2.24.0

From 6c2fcc4b6f4e8cf8e0b05858b2daa459cb390635 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <[email-masked]>
Date: Wed, 27 Nov 2019 13:40:20 +0900
Subject: [PATCH 2/6] gnu: emacs: Simplify the EMACSLOADPATH search path
 specification.

The EMACSLOADPATH can be greatly simplified by relying on a subdirs.el file
that causes Emacs to search recursively a directory found in EMACSLOADPATH.

* gnu/packages/emacs.scm (emacs)[native-search-paths]: Remove the match-all
file pattern regexp.  Remove the versioned site-lisp directory from searched
files, as it appears unused by Emacs.

Reported-by: Leo Prikler <[email-masked]>
---
 gnu/packages/emacs.scm | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index 16f9af0a0a..95859b8a88 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -186,11 +186,9 @@
     (native-search-paths
      (list (search-path-specification
             (variable "EMACSLOADPATH")
-            ;; The versioned entries are for the Emacs' builtin libraries.
-            (files (list (string-append "share/emacs/" version "/site-lisp")
-                         (string-append "share/emacs/" version "/lisp")
-                         "share/emacs/site-lisp"))
-            (file-pattern ".*"))        ;recursively add any sub directory
+            ;; The versioned entry is for the Emacs' builtin libraries.
+            (files (list (string-append "share/emacs/" version "/lisp")
+                         "share/emacs/site-lisp")))
            (search-path-specification
             (variable "INFOPATH")
             (files '("share/info")))))
-- 
2.24.0

From 319b81ef8cbfd68c1c98fe644795ef28ad490bd9 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <[email-masked]>
Date: Wed, 27 Nov 2019 13:51:53 +0900
Subject: [PATCH 3/6] gnu: emacs: Fix guix-emacs.el indentation.

* gnu/packages/aux-files/emacs/guix-emacs.el: Fix indentation.
---
 gnu/packages/aux-files/emacs/guix-emacs.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/aux-files/emacs/guix-emacs.el b/gnu/packages/aux-files/emacs/guix-emacs.el
index 46ee557f20..b4315c1a2e 100644
--- a/gnu/packages/aux-files/emacs/guix-emacs.el
+++ b/gnu/packages/aux-files/emacs/guix-emacs.el
@@ -54,8 +54,8 @@ The files in the list do not have extensions (.el, .elc)."
           (seq-filter (lambda (dir)
                         (string-match-p "/share/emacs/site-lisp" dir))
                       (split-string emacs-load-path ":")))
-          (autoloads (mapcan #'guix-emacs-find-autoloads
-                             emacs-non-core-load-path-directories)))
+         (autoloads (mapcan #'guix-emacs-find-autoloads
+                            emacs-non-core-load-path-directories)))
     (mapc (lambda (f)
             (load f 'noerror))
           autoloads)))
-- 
2.24.0

From 3c8b5f63b2e34556463c22fa1565b46c1b31033c Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <[email-masked]>
Date: Wed, 27 Nov 2019 14:02:42 +0900
Subject: [PATCH 4/6] gnu: emacs: Use load-path instead of EMACSLOADPATH.

This enables the use of the subdirs.el feature of Emacs, where specifying a
directory in EMACSLOADPATH translates into a `load-path' variable containing
the directory and all its sub-directories.

* gnu/packages/aux-files/emacs/guix-emacs.el (guix-emacs-autoload-packages):
Use `load-path' directly instead of parsing EMACSLOADPATH.

Reported-by: Leo Prikler <[email-masked]>
---
 gnu/packages/aux-files/emacs/guix-emacs.el | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/aux-files/emacs/guix-emacs.el b/gnu/packages/aux-files/emacs/guix-emacs.el
index b4315c1a2e..05fc9709b6 100644
--- a/gnu/packages/aux-files/emacs/guix-emacs.el
+++ b/gnu/packages/aux-files/emacs/guix-emacs.el
@@ -47,13 +47,12 @@ The files in the list do not have extensions (.el, .elc)."
   ;; FIXME: The autoloads generated by the emacs-build-system are not byte
   ;; compiled.
   (interactive)
-  (let* ((emacs-load-path (getenv "EMACSLOADPATH"))
-         (emacs-non-core-load-path-directories
+  (let* ((emacs-non-core-load-path-directories
           ;; Filter out core Elisp directories, which are already autoloaded
           ;; by Emacs.
           (seq-filter (lambda (dir)
                         (string-match-p "/share/emacs/site-lisp" dir))
-                      (split-string emacs-load-path ":")))
+                      load-path))
          (autoloads (mapcan #'guix-emacs-find-autoloads
                             emacs-non-core-load-path-directories)))
     (mapc (lambda (f)
-- 
2.24.0

From baccbc37f60843f10e0bd384b5729e7670784b7a Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <[email-masked]>
Date: Wed, 27 Nov 2019 22:32:40 +0900
Subject: [PATCH 5/6] gnu: emacs-ert-runner: Fix build.

* gnu/packages/emacs-xyz.scm (emacs-ert-runner): Refer to the updated
installation directory.
---
 gnu/packages/emacs-xyz.scm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
index 0caf12a423..7f140ad5de 100644
--- a/gnu/packages/emacs-xyz.scm
+++ b/gnu/packages/emacs-xyz.scm
@@ -9859,8 +9859,7 @@ Emacs.")
                  (substitute* "bin/ert-runner"
                    (("ERT_RUNNER=\"\\$\\(dirname \\$\\(dirname \\$0\\)\\)")
                     (string-append "ERT_RUNNER=\"" out
-                                   "/share/emacs/site-lisp/guix.d/ert-runner-"
-                                   ,version)))
+                                   "/share/emacs/site-lisp")))
                  (install-file "bin/ert-runner" (string-append out "/bin"))
                  (wrap-program (string-append out "/bin/ert-runner")
                    (list "EMACSLOADPATH" ":" 'prefix
-- 
2.24.0

From 0c7f859eff56d631ffe73227f012c6117040ade4 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <[email-masked]>
Date: Wed, 27 Nov 2019 22:34:44 +0900
Subject: [PATCH 6/6] gnu: emacs-emacsql: Fix build.

* gnu/packages/emacs-xyz.scm (emacs-emacsql): Refer to the updated
installation directory.
---
 gnu/packages/emacs-xyz.scm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
index 7f140ad5de..a30685189b 100644
--- a/gnu/packages/emacs-xyz.scm
+++ b/gnu/packages/emacs-xyz.scm
@@ -11948,8 +11948,7 @@ object has been freed.")
                (install-file "sqlite/emacsql-sqlite"
                              (string-append out "/bin"))
                (for-each (cut install-file <>
-                              (string-append out "/share/emacs/site-lisp/guix.d/"
-                                             "emacsql" "-" ,version))
+                              (string-append out "/share/emacs/site-lisp"))
                          (find-files "." "\\.elc*$")))
              #t)))))
     (inputs
-- 
2.24.0

Maxim