all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / Atom feed
* [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record.
@ 2020-09-26  6:11 Maxim Cournoyer
  2020-09-26  6:14 ` [bug#43627] [PATCH core-updates 1/3] guix: Add an append-separator? " Maxim Cournoyer
                   ` (3 more replies)
  0 siblings, 4 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2020-09-26  6:11 UTC (permalink / raw)
  To: 43627

Hello Guix!

These three commits extend our search-path-specification record with a
new field, that can be used to produce a trailing separator, which can
be useful at least with Emacs (I can think of at least another place
where it could be used: the INFOPATH variable).

It was motivated to allow defining the Emacs search path in a more
robust way.

Thanks!

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates 1/3] guix: Add an append-separator? field to the <search-path-specification> record.
  2020-09-26  6:11 [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Maxim Cournoyer
@ 2020-09-26  6:14 ` Maxim Cournoyer
  2020-09-26  6:14   ` [bug#43627] [PATCH core-updates 2/3] gnu: emacs: Use the new append-separator? option to define its search path Maxim Cournoyer
  2020-09-26  6:14   ` [bug#43627] [PATCH core-updates 3/3] Revert "emacs-build-system: Ensure the core libraries appear last in the load path." Maxim Cournoyer
  2020-09-27 19:01 ` [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Ludovic Courtès
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2020-09-26  6:14 UTC (permalink / raw)
  To: 43627; +Cc: Maxim Cournoyer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 14127 bytes --]

This new field allows to specify in a search-path-specification that a
trailing separator should be added to the computed value of the environment
variable.  A trailing separator sometimes has the meaning that the usual
builtin locations should be looked up as well as the ones explicitly
specified.

One use case is to specify the Emacs library paths using EMACSLOADPATH.  This
allows to not embed the Emacs version in its search path specification, which
has been shown to cause issues when upgrading a profile or when defining
variant Emacs packages of different versions.

* guix/search-paths.scm (searh-path-specification): Add an APPEND-SEPARATOR?
field.
(search-path-specification->sexp): Adjust accordingly.
(sexp->search-path-specification): Likewise.
(evaluate-search-paths): Append a separator to the search path value when both
`separator' and `append-separator?' are #t.  Document the new behavior.
* guix/scripts/environment.scm (create-environment): Adjust the logic used to
merge search-path values when creating an environment profile.
* guix/build/gnu-build-system.scm (set-paths): Adjust accordingly.
---
 guix/build/gnu-build-system.scm | 11 +++++---
 guix/build/profiles.scm         | 13 ++++++---
 guix/build/utils.scm            | 13 +++++++--
 guix/scripts/environment.scm    | 11 ++++++--
 guix/search-paths.scm           | 49 +++++++++++++++++++++++----------
 5 files changed, 70 insertions(+), 27 deletions(-)

diff --git a/guix/build/gnu-build-system.scm b/guix/build/gnu-build-system.scm
index d3347c9518..7039910c79 100644
--- a/guix/build/gnu-build-system.scm
+++ b/guix/build/gnu-build-system.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2018 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2020 Brendan Tildesley <mail@brendan.scot>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -96,23 +97,25 @@ See https://reproducible-builds.org/specs/source-date-epoch/."
                                              input-directories)))
 
   (for-each (match-lambda
-             ((env-var (files ...) separator type pattern)
+             ((env-var (files ...) separator type pattern append-sep)
               (set-path-environment-variable env-var files
                                              input-directories
                                              #:separator separator
                                              #:type type
-                                             #:pattern pattern)))
+                                             #:pattern pattern
+                                             #:append-separator? append-sep)))
             search-paths)
 
   (when native-search-paths
     ;; Search paths for native inputs, when cross building.
     (for-each (match-lambda
-               ((env-var (files ...) separator type pattern)
+               ((env-var (files ...) separator type pattern append-sep)
                 (set-path-environment-variable env-var files
                                                native-input-directories
                                                #:separator separator
                                                #:type type
-                                               #:pattern pattern)))
+                                               #:pattern pattern
+                                               #:append-separator? append-sep)))
               native-search-paths))
 
   #t)
diff --git a/guix/build/profiles.scm b/guix/build/profiles.scm
index 67ee9b665a..035c6558ba 100644
--- a/guix/build/profiles.scm
+++ b/guix/build/profiles.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -20,6 +21,7 @@
   #:use-module (guix build union)
   #:use-module (guix build utils)
   #:use-module (guix search-paths)
+  #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (ice-9 ftw)
   #:use-module (ice-9 match)
@@ -51,10 +53,13 @@ user-friendly name of the profile is, for instance ~/.guix-profile rather than
          ((? string? separator)
           (let ((items (string-tokenize* value separator)))
             (cons search-path
-                  (string-join (map (lambda (str)
-                                      (string-append replacement (crop str)))
-                                    items)
-                               separator)))))))))
+                  (string-join
+                   (map (lambda (str)
+                          (string-append replacement (crop str)))
+                        ;; When APPEND-SEPARATOR? is #t, the trailing
+                        ;; separator causes an empty string item.  Remove it.
+                        (remove string-null? items))
+                   separator)))))))))
 
 (define (write-environment-variable-definition port)
   "Write the given environment variable definition to PORT."
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index afcb71fae3..0a3f38fc69 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -6,6 +6,7 @@
 ;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -524,7 +525,8 @@ for under the directories designated by FILES.  For example:
                                         #:key
                                         (separator ":")
                                         (type 'directory)
-                                        pattern)
+                                        pattern
+                                        (append-separator? #f))
   "Look for each of FILES of the given TYPE (a symbol as returned by
 'stat:type') in INPUT-DIRS.  Set ENV-VAR to a SEPARATOR-separated path
 accordingly.  Example:
@@ -541,11 +543,16 @@ denoting file names to look for under the directories designated by FILES:
                                  (list docbook-xml docbook-xsl)
                                  #:type 'regular
                                  #:pattern \"^catalog\\\\.xml$\")
-"
+
+When both SEPARATOR and APPEND-SEPARATOR? are true, a separator is appended to
+the value of the environment variable."
   (let* ((path  (search-path-as-list files input-dirs
                                      #:type type
                                      #:pattern pattern))
-         (value (list->search-path-as-string path separator)))
+         (value (list->search-path-as-string path separator))
+         (value (if append-separator?
+                          (string-append value separator)
+                          value)))
     (if (string-null? value)
         (begin
           ;; Never set ENV-VAR to an empty string because often, the empty
diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index ad50281eb2..f3d53c7ae5 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2014, 2015, 2018 David Thompson <davet@gnu.org>
 ;;; Copyright © 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2018 Mike Gerwitz <mtg@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -80,12 +81,18 @@ variables with additional search paths."
   (when pure?
     (purify-environment white-list))
   (for-each (match-lambda
-              ((($ <search-path-specification> variable _ separator) . value)
+              ((($ <search-path-specification> variable _ separator _
+                                               append-separator?) . value)
                (let ((current (getenv variable)))
                  (setenv variable
                          (if (and current (not pure?))
                              (if separator
-                                 (string-append value separator current)
+                                 (if append-separator?
+                                     ;; There is already a trailing separator
+                                     ;; at the end of value.
+                                     ;; (see: `evaluate-search-paths').
+                                     (string-append value current separator)
+                                     (string-append value separator current))
                                  value)
                              value)))))
             (profile-search-paths profile manifest))
diff --git a/guix/search-paths.scm b/guix/search-paths.scm
index 002e6342bb..d783a2815f 100644
--- a/guix/search-paths.scm
+++ b/guix/search-paths.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2017, 2018 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -30,6 +31,7 @@
             search-path-specification-separator
             search-path-specification-file-type
             search-path-specification-file-pattern
+            search-path-specification-append-separator?
 
             $PATH
 
@@ -54,14 +56,16 @@
 (define-record-type* <search-path-specification>
   search-path-specification make-search-path-specification
   search-path-specification?
-  (variable     search-path-specification-variable) ;string
-  (files        search-path-specification-files)    ;list of strings
-  (separator    search-path-specification-separator ;string | #f
-                (default ":"))
-  (file-type    search-path-specification-file-type ;symbol
-                (default 'directory))
-  (file-pattern search-path-specification-file-pattern ;#f | string
-                (default #f)))
+  (variable          search-path-specification-variable) ;string
+  (files             search-path-specification-files)    ;list of strings
+  (separator         search-path-specification-separator ;string | #f
+                     (default ":"))
+  (file-type         search-path-specification-file-type ;symbol
+                     (default 'directory))
+  (file-pattern      search-path-specification-file-pattern ;#f | string
+                     (default #f))
+  (append-separator? search-path-specification-append-separator? ;#f | #t
+                     (default #f)))
 
 (define $PATH
   ;; The 'PATH' variable.  This variable is a bit special: it is not attached
@@ -76,20 +80,32 @@ corresponds to the arguments expected by `set-path-environment-variable'."
   ;; Note that this sexp format is used both by build systems and in
   ;; (guix profiles), so think twice before you change it.
   (match spec
-    (($ <search-path-specification> variable files separator type pattern)
-     `(,variable ,files ,separator ,type ,pattern))))
+    (($ <search-path-specification> variable files separator type pattern
+                                    append-separator?)
+     `(,variable ,files ,separator ,type ,pattern ,append-separator?))))
 
 (define (sexp->search-path-specification sexp)
   "Convert SEXP, which is as returned by 'search-path-specification->sexp', to
 a <search-path-specification> object."
   (match sexp
+    ((variable files separator type pattern append-separator?)
+     (search-path-specification
+      (variable variable)
+      (files files)
+      (separator separator)
+      (file-type type)
+      (file-pattern pattern)
+      (append-separator? append-separator?)))
+    ;; Previous search-path-specification form (without append-separator?
+    ;; might still be found in manifest files.
     ((variable files separator type pattern)
      (search-path-specification
       (variable variable)
       (files files)
       (separator separator)
       (file-type type)
-      (file-pattern pattern)))))
+      (file-pattern pattern)
+      (append-separator? #f)))))
 
 (define-syntax-rule (with-null-error-port exp)
   "Evaluate EXP with the error port pointing to the bit bucket."
@@ -131,7 +147,9 @@ like `string-tokenize', but SEPARATOR is a string."
   "Evaluate SEARCH-PATHS, a list of search-path specifications, for
 DIRECTORIES, a list of directory names, and return a list of
 specification/value pairs.  Use GETENV to determine the current settings and
-report only settings not already effective."
+report only settings not already effective.  When the search path
+specification APPEND-SEPARATOR? and SEPARATOR fields are both set to true, a
+separator is appended to its computed value."
   (define (search-path-definition spec)
     (match spec
       (($ <search-path-specification> variable files #f type pattern)
@@ -148,7 +166,7 @@ report only settings not already effective."
                 #f                         ;VARIABLE already set appropriately
                 (cons spec head))))))
       (($ <search-path-specification> variable files separator
-                                      type pattern)
+                                      type pattern append-separator?)
        (let* ((values (or (and=> (getenv variable)
                                  (cut string-tokenize* <> separator))
                           '()))
@@ -161,7 +179,10 @@ report only settings not already effective."
                                             #:pattern pattern))))
          (if (every (cut member <> values) path)
              #f                         ;VARIABLE is already set appropriately
-             (cons spec (string-join path separator)))))))
+             (let ((value (string-join path separator)))
+               (cons spec (if append-separator?
+                              (string-append value separator)
+                              value))))))))
 
   (filter-map search-path-definition search-paths))
 
-- 
2.28.0





^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates 2/3] gnu: emacs: Use the new append-separator? option to define its search path.
  2020-09-26  6:14 ` [bug#43627] [PATCH core-updates 1/3] guix: Add an append-separator? " Maxim Cournoyer
@ 2020-09-26  6:14   ` Maxim Cournoyer
  2020-09-26  6:14   ` [bug#43627] [PATCH core-updates 3/3] Revert "emacs-build-system: Ensure the core libraries appear last in the load path." Maxim Cournoyer
  1 sibling, 0 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2020-09-26  6:14 UTC (permalink / raw)
  To: 43627; +Cc: Maxim Cournoyer

Fixes <https://issues.guix.info/43277>.

* gnu/packages/emacs.scm (emacs): Remove the versioned lisp file name from the
search path, and set the append-separator? field to #t.
---
 gnu/packages/emacs.scm | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index 03c28ee7a7..d673bc741d 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -257,9 +257,8 @@
     (native-search-paths
      (list (search-path-specification
             (variable "EMACSLOADPATH")
-            ;; The versioned entry is for the Emacs' builtin libraries.
-            (files (list "share/emacs/site-lisp"
-                         (string-append "share/emacs/" version "/lisp"))))
+            (files (list "share/emacs/site-lisp"))
+            (append-separator? #t))
            (search-path-specification
             (variable "INFOPATH")
             (files '("share/info")))))
-- 
2.28.0





^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates 3/3] Revert "emacs-build-system: Ensure the core libraries appear last in the load path."
  2020-09-26  6:14 ` [bug#43627] [PATCH core-updates 1/3] guix: Add an append-separator? " Maxim Cournoyer
  2020-09-26  6:14   ` [bug#43627] [PATCH core-updates 2/3] gnu: emacs: Use the new append-separator? option to define its search path Maxim Cournoyer
@ 2020-09-26  6:14   ` Maxim Cournoyer
  1 sibling, 0 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2020-09-26  6:14 UTC (permalink / raw)
  To: 43627; +Cc: Maxim Cournoyer

This reverts commit e34e02707d6bd38c79ce7bec776fcdc528548a0d.  This hack is no
longer necessary, now that search path specifications allows specifying that a
trailing separator should be appended, which for Emacs stands for the location
of the builtin Elisp libraries.
---
 guix/build/emacs-build-system.scm | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 26ea59bc25..227434d904 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -76,18 +76,8 @@ archive, a directory, or an Emacs Lisp file."
 (define* (add-source-to-load-path #:key dummy #:allow-other-keys)
   "Augment the EMACSLOADPATH environment variable with the source directory."
   (let* ((source-directory (getcwd))
-         (emacs-load-path (string-split (getenv "EMACSLOADPATH") #\:))
-         ;; XXX: Make sure the Emacs core libraries appear at the end of
-         ;; EMACSLOADPATH, to avoid shadowing any other libraries depended
-         ;; upon.
-         (emacs-load-path-non-core (filter (cut string-contains <>
-                                                "/share/emacs/site-lisp")
-                                           emacs-load-path))
-         (emacs-load-path-value (string-append
-                                 (string-join (cons source-directory
-                                                    emacs-load-path-non-core)
-                                              ":")
-                                 ":")))
+         (emacs-load-path-value (string-append source-directory ":"
+                                               (getenv "EMACSLOADPATH"))))
     (setenv "EMACSLOADPATH" emacs-load-path-value)
     (format #t "source directory ~s prepended to the `EMACSLOADPATH' \
 environment variable\n" source-directory)))
-- 
2.28.0





^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record.
  2020-09-26  6:11 [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Maxim Cournoyer
  2020-09-26  6:14 ` [bug#43627] [PATCH core-updates 1/3] guix: Add an append-separator? " Maxim Cournoyer
@ 2020-09-27 19:01 ` Ludovic Courtès
  2020-10-03 21:22   ` Maxim Cournoyer
  2021-03-30 15:51 ` [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? " Maxim Cournoyer
  2021-03-30 18:41 ` [bug#43627] [PATCH] gnu: emacs: Wrap EMACSLOADPATH Leo Prikler
  3 siblings, 1 reply; 35+ messages in thread
From: Ludovic Courtès @ 2020-09-27 19:01 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43627

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> These three commits extend our search-path-specification record with a
> new field, that can be used to produce a trailing separator, which can
> be useful at least with Emacs (I can think of at least another place
> where it could be used: the INFOPATH variable).
>
> It was motivated to allow defining the Emacs search path in a more
> robust way.

I’m skeptical since we have only one (or two?) use cases.  I think we
should weigh the added complexity, both in terms of implementation and
of semantic clarity, compared to reduced complexity elsewhere.  It seems
to me that the costs outweigh the benefits here.

Also, the empty search path entry has a special meaning.  For
EMACSLOADPATH, that entry doesn’t have to be last, one can choose to put
it in the middle of the search path (info "(emacs) General Variables").
The trailing separator is just a special case.

WDYT?

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record.
  2020-09-27 19:01 ` [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Ludovic Courtès
@ 2020-10-03 21:22   ` Maxim Cournoyer
  2020-11-02 13:59     ` Ludovic Courtès
  0 siblings, 1 reply; 35+ messages in thread
From: Maxim Cournoyer @ 2020-10-03 21:22 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43627

Hello Ludovic!

Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> These three commits extend our search-path-specification record with a
>> new field, that can be used to produce a trailing separator, which can
>> be useful at least with Emacs (I can think of at least another place
>> where it could be used: the INFOPATH variable).
>>
>> It was motivated to allow defining the Emacs search path in a more
>> robust way.
>
> I’m skeptical since we have only one (or two?) use cases.  I think we
> should weigh the added complexity, both in terms of implementation and
> of semantic clarity, compared to reduced complexity elsewhere.  It seems
> to me that the costs outweigh the benefits here.

I too was skeptical at first (which explains why this commit was not
submitted for inclusion for 3 years :-)), but recent events surrounding
emacs-next and the bump to Emacs 27 have put its value back into light.

> Also, the empty search path entry has a special meaning.  For
> EMACSLOADPATH, that entry doesn’t have to be last, one can choose to put
> it in the middle of the search path (info "(emacs) General Variables").
> The trailing separator is just a special case.

Yes, I'm aware of this; it doesn't makes sense for our use to put it
anywhere else than at the end though, because we want to allow user
installed libraries to override builtin ones (e.g., a more recent Org
from emacs-org package overriding the builtin Org version), not the
contrary.

Another reason to consider this change is combining profiles.

Currently:

$ guix install -p /tmp/p1 emacs-no-x emacs-sr-speedbar
$ guix install -p /tmp/p2 emacs-no-x emacs-org
$ guix package -p /tmp/p1 -p /tmp/p2 --search-paths | grep EMACS
export
EMACSLOADPATH="/tmp/profile1/share/emacs/site-lisp:/tmp/profile1/share/emacs/27.1/lisp:/tmp/profile2/share/emacs/site-lisp:/tmp/profile2/share/emacs/27.1/lisp"

Which is clearly wrong, as the Emacs' own libraries will appear before
the user-installed libraries of the second profile, causing the Org
version used to be that of Emacs rather than the one the user installed.

There's no way to work around this except by adding ad-hoc klugdes
around the code where different profiles need to be merged.

On the other hand, if the information is recorded in the search-path
object itself, we can combined search-path in the way they were meant to
be.  In the occurrence, the resulting combined search path would have
the append-separator? set, causing the end result to have a single ':'
appended, with the correct behavior:

(with this proposed change)

$ ./pre-inst-env guix install -p /tmp/p1v2 emacs-no-x emacs-sr-speedbar
$ ./pre-inst-env guix install -p /tmp/p2v2 emacs-no-x emacs-org
$ ./pre-inst-env guix package -p /tmp/p1v2 -p /tmp/p2v2 --search-paths | grep EMACS
export
EMACSLOADPATH="/tmp/p1v2/share/emacs/site-lisp:/tmp/p2v2/share/emacs/site-lisp:"

I hope this helps understanding the rationale behind this change.

Thanks,

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record.
  2020-10-03 21:22   ` Maxim Cournoyer
@ 2020-11-02 13:59     ` Ludovic Courtès
  2020-11-08  5:49       ` Maxim Cournoyer
  0 siblings, 1 reply; 35+ messages in thread
From: Ludovic Courtès @ 2020-11-02 13:59 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43627

Hi Maxim,

Apologies for not following up earlier on this one…

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi,
>>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>
>>> These three commits extend our search-path-specification record with a
>>> new field, that can be used to produce a trailing separator, which can
>>> be useful at least with Emacs (I can think of at least another place
>>> where it could be used: the INFOPATH variable).
>>>
>>> It was motivated to allow defining the Emacs search path in a more
>>> robust way.
>>
>> I’m skeptical since we have only one (or two?) use cases.  I think we
>> should weigh the added complexity, both in terms of implementation and
>> of semantic clarity, compared to reduced complexity elsewhere.  It seems
>> to me that the costs outweigh the benefits here.
>
> I too was skeptical at first (which explains why this commit was not
> submitted for inclusion for 3 years :-)), but recent events surrounding
> emacs-next and the bump to Emacs 27 have put its value back into light.

Do you know if there are other use cases than Emacs for this?

I vaguely remember that ‘EMACSLOADPATH’ unusual handling of the empty
entry is why we didn’t have it in ‘native-search-paths’ initially, and
why Alex Kost had come up with a different mechanism instead.

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record.
  2020-11-02 13:59     ` Ludovic Courtès
@ 2020-11-08  5:49       ` Maxim Cournoyer
  0 siblings, 0 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2020-11-08  5:49 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43627

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Maxim,
>
> Apologies for not following up earlier on this one…

No worries!

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Hi,
>>>
>>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>>
>>>> These three commits extend our search-path-specification record with a
>>>> new field, that can be used to produce a trailing separator, which can
>>>> be useful at least with Emacs (I can think of at least another place
>>>> where it could be used: the INFOPATH variable).
>>>>
>>>> It was motivated to allow defining the Emacs search path in a more
>>>> robust way.
>>>
>>> I’m skeptical since we have only one (or two?) use cases.  I think we
>>> should weigh the added complexity, both in terms of implementation and
>>> of semantic clarity, compared to reduced complexity elsewhere.  It seems
>>> to me that the costs outweigh the benefits here.
>>
>> I too was skeptical at first (which explains why this commit was not
>> submitted for inclusion for 3 years :-)), but recent events surrounding
>> emacs-next and the bump to Emacs 27 have put its value back into light.
>
> Do you know if there are other use cases than Emacs for this?

The three environment variables that I know that make use of it in this
way are EMACSLOADPATH, INFOPATH and MANPATH.  There may be others, but
it's hard to search for that.

> I vaguely remember that ‘EMACSLOADPATH’ unusual handling of the empty
> entry is why we didn’t have it in ‘native-search-paths’ initially, and
> why Alex Kost had come up with a different mechanism instead.

I'm not sure what this was about, but if there was an issue with using
EMACSLOADPATH, I think we would have found out by now :-).

Thanks,

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#45359] [PATCH]: Re-introduce Emacs packages specific installation prefix.
@ 2020-12-22  3:28     ` Maxim Cournoyer
  2020-12-22  8:51       ` bug#45316: " Leo Prikler
       [not found]       ` <handler.45359.D45359.161704236132600.notifdone@debbugs.gnu.org>
  0 siblings, 2 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2020-12-22  3:28 UTC (permalink / raw)
  To: 45359

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

Hello,

I've goofed and sent this to guix-bugs, apparently, so I'm forwarding it
to guix-patches to make sure everybody possibly interested had a chance
to see it.

Thank you,

Maxim

-------------------- Start of forwarded message --------------------
Subject: bug#45316: [PATCH]: Re-introduce Emacs packages specific installation prefix.
To: 45316@debbugs.gnu.org
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Fri, 18 Dec 2020 17:00:10 -0500


[-- Attachment #2.1: Type: text/plain, Size: 2631 bytes --]

Hello Guix!

tl;dr: The Emacs build system and site-start.el loader are modified so
that Emacs packages are installed in their own distinct installation
directory.

The Emacs packages built with the Emacs built system used to be
installed in a sub-directory under the share/emacs/guix.d/ directory,
but this was changed in commit 65a7dd2950ca13a8b942b2836260a2192351b271
shortly after having accommodated the site-start.el machinery to enable
loading packages from any profile (via the EMACSLOADPATH search path
specification).

While this change allowed to expose simply and directly the packages
found in EMACSLOADPATH, it does introduce the risk of file name
collisions when multiple Emacs packages are joined in the same profile,
especially with Emacs packages increasing in complexity (e.g., using
more than a single .el file!) and expecting to have both their sources
and resources extracted under their own nested directory rather than as
a flat collection (ELPA, MELPA).

One recent example I stumbled on was attempting to use the
emacs-yasnippet-snippets package along with emacs-elpy; both wanted to
install a 'snippets' directory to share/emacs/site-lisp/snippets,
collided and resulted in problems that prove difficult to understand.

This is what motivated this patch series, where the site-start.el
auxiliary code used for package discovery is extended to support
packages installed in their own directory under a 'share/emacs/guix'
installation prefix, via Emacs' own package library!

The emacs-build-system is updated for this new installation prefix, as
well as existing packages and documentation.  Parting with a directly
usable EMACSLOADPATH means that site-start.el *must* run for packages to
appear in the load-path; that means for running a test suite, the -Q or
--quick Emacs options cannot be used, since it implies --no-site-file.

Benefits of using this approach:

+ Avoid inter-package file name collisions.

+ Better integration with user installed packages via M-x
package-install.  The Guix-installed packages are listed in M-x
package-list as 'external'.

Cons include:

- Slightly more complex loader (although much of it is offloaded to
  package.el), thus slightly slower (see the comparison below).

- Requires to ensure every package's test suite doesn't make use of -Q.

In my opinion the benefits outweighs the cons by a comfortable margin,
especially with the boring work of adapting the package collection
already done.

To test the performance of the new approach, the following manifest file
was used to test the rebuild of the ~900 Emacs packages making use of
the Emacs build system:


[-- Attachment #2.2: emacs-packages-manifest.scm --]
[-- Type: text/plain, Size: 806 bytes --]

(use-modules (gnu packages)
             (guix build-system)
             (guix packages)
             (srfi srfi-1))

(define %broken-emacs-packages
  (map specification->package
       '("emacs-picpocket"              ;tests fail
         "emacs-twittering-mode"        ;build fails

         ;; Broken only on current master, without new changes.
         "emacs-md4rd"
         "emacs-el-patch"
         "emacs-flymake-shellcheck"
         )))

(define %emacs-packages
  (fold-packages (lambda (package lst)
                   (if (eq? (build-system-name (package-build-system package))
                            'emacs)
                       (cons package lst)
                       lst))
                 '()))

(packages->manifest
 (lset-difference eqv? %emacs-packages %broken-emacs-packages))

[-- Attachment #2.3: Type: text/plain, Size: 2772 bytes --]


A simple benchmark testing the performance of the activation of the
hundreds of Emacs packages was then run using:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix environment --pure -m emacs-packages-manifest.scm \
                                  --ad-hoc emacs
[env]$ /run/setuid-programs/sudo /bin/sh -c 'echo 3 > /proc/sys/vm/drop_caches'
[env]$ emacs --batch --no-site-file \
    --eval="(progn (require 'guix-emacs) \
                   (require 'benchmark) \
                   (message \"(total gc-count gc-time) = %s\" \
                            (benchmark-run 1 (guix-emacs-autoload-packages))))"
--8<---------------cut here---------------end--------------->8---

On the master branch:

--8<---------------cut here---------------start------------->8---
[...]
Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-profile/share/emacs/site-lisp/zotxt-autoloads...
Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-profile/share/emacs/site-lisp/zoutline-autoloads...
Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-profile/share/emacs/site-lisp/ztree-autoloads...
(total gc-count gc-time) = (25.242400751 13 0.189669369)
--8<---------------cut here---------------end--------------->8---

Or about 0.65 s on a warm cache.

On a branch with these changes:

--8<---------------cut here---------------start------------->8---
Error loading autoloads: (file-missing Cannot open load file No such file or directory kotl/kotl-autoloads)
Error loading autoloads: (file-missing Cannot open load file No such file or directory helm-easymenu)
Error loading autoloads: (file-missing Cannot open load file No such file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-profile/share/emacs/site-lisp/guix/flycheck-cpplint-0.1-1.1d8a090/flycheck-cpplint-autoloads)
Error loading autoloads: (file-missing Cannot open load file No such file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-profile/share/emacs/site-lisp/guix/evil-anzu-0.03/evil-anzu-autoloads)
Error loading autoloads: (file-missing Cannot open load file No such file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-profile/share/emacs/site-lisp/guix/erc-image-0-3.82fb387/erc-image-autoloads)
ad-handle-definition: `ido-completing-read' got redefined
Error loading autoloads: (file-missing Cannot open load file No such file or directory tex-site)
(total gc-count gc-time) = (26.175704339 47 0.783184412)
--8<---------------cut here---------------end--------------->8---

Or about 3 seconds on a warm cache.

There a 6 errors that would need to be looked into, but I these look
like actual packaging problems rather than new issues.  The previously
used way to load the autoloads, '(load f 'noerror)' would have masked
them.

Thanks,

Maxim

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

-------------------- End of forwarded message --------------------

^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45316: [PATCH]: Re-introduce Emacs packages specific installation prefix.
  2020-12-22  3:28     ` [bug#45359] [PATCH]: Re-introduce Emacs packages specific installation prefix Maxim Cournoyer
@ 2020-12-22  8:51       ` Leo Prikler
  2020-12-22 18:09         ` Maxim Cournoyer
       [not found]       ` <handler.45359.D45359.161704236132600.notifdone@debbugs.gnu.org>
  1 sibling, 1 reply; 35+ messages in thread
From: Leo Prikler @ 2020-12-22  8:51 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 45316

Hello Maxim,

As someone, who lobbied for the current status quo, I have some
thoughts to share.

Am Montag, den 21.12.2020, 22:28 -0500 schrieb Maxim Cournoyer:
> The Emacs packages built with the Emacs built system used to be
> installed in a sub-directory under the share/emacs/guix.d/ directory,
> but this was changed in commit
> 65a7dd2950ca13a8b942b2836260a2192351b271
> shortly after having accommodated the site-start.el machinery to
> enable
> loading packages from any profile (via the EMACSLOADPATH search path
> specification).
Won't this reintroduce <https://bugs.gnu.org/38309> then?

> While this change allowed to expose simply and directly the packages
> found in EMACSLOADPATH, it does introduce the risk of file name
> collisions when multiple Emacs packages are joined in the same
> profile,
> especially with Emacs packages increasing in complexity (e.g., using
> more than a single .el file!) and expecting to have both their
> sources
> and resources extracted under their own nested directory rather than
> as
> a flat collection (ELPA, MELPA).

> One recent example I stumbled on was attempting to use the
> emacs-yasnippet-snippets package along with emacs-elpy; both wanted
> to
> install a 'snippets' directory to share/emacs/site-lisp/snippets,
> collided and resulted in problems that prove difficult to understand.
I believe that to be a problem in those packages.  Data should not be
installed into share/emacs/site-lisp, but share/emacs/etc.  See for
instance also emacs-telega, which – while not quite adhering to the
above – installs its data in share/emacs/{telega-contrib,telega-data}.

Regardless of what you intend to do with site-lisp otherwise, data
files should *not*, I repeat *not* be installed there.  I do believe
standardizing share/emacs/etc is the way to go, however.

> This is what motivated this patch series, where the site-start.el
> auxiliary code used for package discovery is extended to support
> packages installed in their own directory under a 'share/emacs/guix'
> installation prefix, via Emacs' own package library!
> 
> The emacs-build-system is updated for this new installation prefix,
> as
> well as existing packages and documentation.  Parting with a directly
> usable EMACSLOADPATH means that site-start.el *must* run for packages
> to
> appear in the load-path; that means for running a test suite, the -Q
> or
> --quick Emacs options cannot be used, since it implies --no-site-
> file.
Having no usable EMACSLOADPATH sounds like a dealbreaker to me.  Could
one be restored by using direct subdirectories to share/emacs/site-lisp 
or would that merely be a cosmetic change?

> + Avoid inter-package file name collisions.
Note: This regards data, which should not be stored in site-lisp to
begin with.  You don't get any benefits doing this for code, because
Emacs has no strong module system.
> + Better integration with user installed packages via M-x
> package-install.  The Guix-installed packages are listed in M-x
> package-list as 'external'.
That does sounds like a genuine pro to me.
> - Slightly more complex loader (although much of it is offloaded to
>   package.el), thus slightly slower (see the comparison below).
That should be bearable.
> - Requires to ensure every package's test suite doesn't make use of
> -Q.
That not so much.  Note, that test cases are not the only use for -Q!

> In my opinion the benefits outweighs the cons by a comfortable
> margin,
> especially with the boring work of adapting the package collection
> already done.
> 
> To test the performance of the new approach, the following manifest
> file
> was used to test the rebuild of the ~900 Emacs packages making use of
> the Emacs build system:
> 
> A simple benchmark testing the performance of the activation of the
> hundreds of Emacs packages was then run using:
> 
> --8<---------------cut here---------------start------------->8---
> $ ./pre-inst-env guix environment --pure -m emacs-packages-
> manifest.scm \
>                                   --ad-hoc emacs
> [env]$ /run/setuid-programs/sudo /bin/sh -c 'echo 3 >
> /proc/sys/vm/drop_caches'
> [env]$ emacs --batch --no-site-file \
>     --eval="(progn (require 'guix-emacs) \
>                    (require 'benchmark) \
>                    (message \"(total gc-count gc-time) = %s\" \
>                             (benchmark-run 1 (guix-emacs-autoload-
> packages))))"
> --8<---------------cut here---------------end--------------->8---
> 
> On the master branch:
> 
> --8<---------------cut here---------------start------------->8---
> [...]
> Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-
> profile/share/emacs/site-lisp/zotxt-autoloads...
> Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-
> profile/share/emacs/site-lisp/zoutline-autoloads...
> Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-
> profile/share/emacs/site-lisp/ztree-autoloads...
> (total gc-count gc-time) = (25.242400751 13 0.189669369)
> --8<---------------cut here---------------end--------------->8---
> 
> Or about 0.65 s on a warm cache.
> 
> On a branch with these changes:
> 
> --8<---------------cut here---------------start------------->8---
> Error loading autoloads: (file-missing Cannot open load file No such
> file or directory kotl/kotl-autoloads)
> Error loading autoloads: (file-missing Cannot open load file No such
> file or directory helm-easymenu)
> Error loading autoloads: (file-missing Cannot open load file No such
> file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-
> profile/share/emacs/site-lisp/guix/flycheck-cpplint-0.1-
> 1.1d8a090/flycheck-cpplint-autoloads)
> Error loading autoloads: (file-missing Cannot open load file No such
> file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-
> profile/share/emacs/site-lisp/guix/evil-anzu-0.03/evil-anzu-
> autoloads)
> Error loading autoloads: (file-missing Cannot open load file No such
> file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-
> profile/share/emacs/site-lisp/guix/erc-image-0-3.82fb387/erc-image-
> autoloads)
> ad-handle-definition: `ido-completing-read' got redefined
> Error loading autoloads: (file-missing Cannot open load file No such
> file or directory tex-site)
> (total gc-count gc-time) = (26.175704339 47 0.783184412)
> --8<---------------cut here---------------end--------------->8---
> 
> Or about 3 seconds on a warm cache.
Looking at these, total does not seem to have changed much, but gc-
count and gc-time more than tripled.  Any idea on how to mitigate this
or do we have to live with that?

> There a 6 errors that would need to be looked into, but I these look
> like actual packaging problems rather than new issues.  The
> previously
> used way to load the autoloads, '(load f 'noerror)' would have masked
> them.
Regardless of how we handle site-lisp going forward, please do look
into those issues and perhaps file them against the individual packages
if they also cause (other) trouble using the current setup.

Regards,
Leo






^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45316: [PATCH]: Re-introduce Emacs packages specific installation prefix.
  2020-12-22  8:51       ` bug#45316: " Leo Prikler
@ 2020-12-22 18:09         ` Maxim Cournoyer
  2020-12-22 19:10           ` Leo Prikler
  0 siblings, 1 reply; 35+ messages in thread
From: Maxim Cournoyer @ 2020-12-22 18:09 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45316

Hi Leo,

Leo Prikler <leo.prikler@student.tugraz.at> writes:

> Hello Maxim,
>
> As someone, who lobbied for the current status quo, I have some
> thoughts to share.

I'm happy you're commenting :-).

> Am Montag, den 21.12.2020, 22:28 -0500 schrieb Maxim Cournoyer:
>> The Emacs packages built with the Emacs built system used to be
>> installed in a sub-directory under the share/emacs/guix.d/ directory,
>> but this was changed in commit
>> 65a7dd2950ca13a8b942b2836260a2192351b271
>> shortly after having accommodated the site-start.el machinery to
>> enable
>> loading packages from any profile (via the EMACSLOADPATH search path
>> specification).
> Won't this reintroduce <https://bugs.gnu.org/38309> then?

This bug was caused by the EMACSLOADPATH environment variable being too
long.  This was because the original search path specification was
recursively adding any directories under site-lisp/ so that package
installed under a guix.d sub-directory could were directly added to the
load path.

In the current situation, the EMACSLOADPATH contains only the site-lisp/
directory of any profile, plus the versioned lisp/ directory of the
installed Emacs package.  This patch series doesn't change that, so no,
that bug wouldn't be reintroduced.

>> While this change allowed to expose simply and directly the packages
>> found in EMACSLOADPATH, it does introduce the risk of file name
>> collisions when multiple Emacs packages are joined in the same
>> profile,
>> especially with Emacs packages increasing in complexity (e.g., using
>> more than a single .el file!) and expecting to have both their
>> sources
>> and resources extracted under their own nested directory rather than
>> as
>> a flat collection (ELPA, MELPA).
>
>> One recent example I stumbled on was attempting to use the
>> emacs-yasnippet-snippets package along with emacs-elpy; both wanted
>> to
>> install a 'snippets' directory to share/emacs/site-lisp/snippets,
>> collided and resulted in problems that prove difficult to understand.

> I believe that to be a problem in those packages.  Data should not be
> installed into share/emacs/site-lisp, but share/emacs/etc.  See for
> instance also emacs-telega, which – while not quite adhering to the
> above – installs its data in share/emacs/{telega-contrib,telega-data}.
>
> Regardless of what you intend to do with site-lisp otherwise, data
> files should *not*, I repeat *not* be installed there.  I do believe
> standardizing share/emacs/etc is the way to go, however.

While I agree that it would be more elegant the way you propose, that's
not the way Emacs packages have been standardized.  So going against the
single "content directory" (c.f. info "(elisp) Packaging Basics") would
break many Elisp library assumptions about where there files are,
causing more friction (thus work) to adapt them in Guix.  The content
directory of an Elisp package can contain any kind of files (code,
images, etc.), according to info "(elisp) Multi-file Packages".

>> This is what motivated this patch series, where the site-start.el
>> auxiliary code used for package discovery is extended to support
>> packages installed in their own directory under a 'share/emacs/guix'
>> installation prefix, via Emacs' own package library!
>>
>> The emacs-build-system is updated for this new installation prefix,
>> as
>> well as existing packages and documentation.  Parting with a directly
>> usable EMACSLOADPATH means that site-start.el *must* run for packages
>> to
>> appear in the load-path; that means for running a test suite, the -Q
>> or
>> --quick Emacs options cannot be used, since it implies --no-site-
>> file.
> Having no usable EMACSLOADPATH sounds like a dealbreaker to me.  Could
> one be restored by using direct subdirectories to share/emacs/site-lisp
> or would that merely be a cosmetic change?

We have two schemes to accommodate for our Emacs packages:

1. Those installed via their own mean, e.g. make install, using the
gnu-build-system for example.  These would still typically install their
packages directly under site-lisp, possibly multiple files (that could
still collide).

2. Those installed via the emacs-build-system.  With the proposed
changes, those now go to site-lisp/guix/.  The 'guix' sub-directory
makes it unambiguous that anything found under is to be loaded by
package.el; the `package-directory-list' variable can be pointed to it
to have the Emacs' package library discover these self-contained
packages.

>> + Avoid inter-package file name collisions.
> Note: This regards data, which should not be stored in site-lisp to
> begin with.  You don't get any benefits doing this for code, because
> Emacs has no strong module system.

True.

>> + Better integration with user installed packages via M-x
>> package-install.  The Guix-installed packages are listed in M-x
>> package-list as 'external'.
> That does sounds like a genuine pro to me.
>> - Slightly more complex loader (although much of it is offloaded to
>>   package.el), thus slightly slower (see the comparison below).
> That should be bearable.
>> - Requires to ensure every package's test suite doesn't make use of
>> -Q.
> That not so much.  Note, that test cases are not the only use for -Q!

Currently if you use -Q, the Elisp libraries are in the load-path, but
not loaded (you need manually do M-x load-library before you can use
it), or call M-x guix-emacs-autoload-packages to load their autoloads.
For programs, this mean (require 'some-library) works, but M-x
some-library-autoloaded-function doesn't.

After this change, if you use -Q the new style Emacs packages are not
visible at all until you activate them with 'M-x load-library
guix-emacs' then 'M-x guix-package-initialize', or (require 'guix-emacs)
then (guix-package-initialize), programmatically.

I don't see this as a problem.  -Q is simply an alias for
"--no-init-file" "--no-site-lisp" "--no-splash" "--no-x-resources"
"--no-site-file".

>> In my opinion the benefits outweighs the cons by a comfortable
>> margin,
>> especially with the boring work of adapting the package collection
>> already done.
>>
>> To test the performance of the new approach, the following manifest
>> file
>> was used to test the rebuild of the ~900 Emacs packages making use of
>> the Emacs build system:
>>
>> A simple benchmark testing the performance of the activation of the
>> hundreds of Emacs packages was then run using:
>>
>> --8<---------------cut here---------------start------------->8---
>> $ ./pre-inst-env guix environment --pure -m emacs-packages-
>> manifest.scm \
>>                                   --ad-hoc emacs
>> [env]$ /run/setuid-programs/sudo /bin/sh -c 'echo 3 >
>> /proc/sys/vm/drop_caches'
>> [env]$ emacs --batch --no-site-file \
>>     --eval="(progn (require 'guix-emacs) \
>>                    (require 'benchmark) \
>>                    (message \"(total gc-count gc-time) = %s\" \
>>                             (benchmark-run 1 (guix-emacs-autoload-
>> packages))))"
>> --8<---------------cut here---------------end--------------->8---
>>
>> On the master branch:
>>
>> --8<---------------cut here---------------start------------->8---
>> [...]
>> Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-
>> profile/share/emacs/site-lisp/zotxt-autoloads...
>> Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-
>> profile/share/emacs/site-lisp/zoutline-autoloads...
>> Loading /gnu/store/qajc70c7nqycs1301ram8s3x7k9ibg5f-
>> profile/share/emacs/site-lisp/ztree-autoloads...
>> (total gc-count gc-time) = (25.242400751 13 0.189669369)
>> --8<---------------cut here---------------end--------------->8---
>>
>> Or about 0.65 s on a warm cache.
>>
>> On a branch with these changes:
>>
>> --8<---------------cut here---------------start------------->8---
>> Error loading autoloads: (file-missing Cannot open load file No such
>> file or directory kotl/kotl-autoloads)
>> Error loading autoloads: (file-missing Cannot open load file No such
>> file or directory helm-easymenu)
>> Error loading autoloads: (file-missing Cannot open load file No such
>> file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-
>> profile/share/emacs/site-lisp/guix/flycheck-cpplint-0.1-
>> 1.1d8a090/flycheck-cpplint-autoloads)
>> Error loading autoloads: (file-missing Cannot open load file No such
>> file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-
>> profile/share/emacs/site-lisp/guix/evil-anzu-0.03/evil-anzu-
>> autoloads)
>> Error loading autoloads: (file-missing Cannot open load file No such
>> file or directory /gnu/store/ryh0rasi9frm98dkd3kbck6hya6hn2qr-
>> profile/share/emacs/site-lisp/guix/erc-image-0-3.82fb387/erc-image-
>> autoloads)
>> ad-handle-definition: `ido-completing-read' got redefined
>> Error loading autoloads: (file-missing Cannot open load file No such
>> file or directory tex-site)
>> (total gc-count gc-time) = (26.175704339 47 0.783184412)
>> --8<---------------cut here---------------end--------------->8---
>>
>> Or about 3 seconds on a warm cache.
> Looking at these, total does not seem to have changed much, but gc-
> count and gc-time more than tripled.  Any idea on how to mitigate this
> or do we have to live with that?

I don't know enough about Elisp to offer a guess.  We'd have to profile
it.  When put in perspective, that's 3 seconds to refresh about 900
packages.  I'm guessing most users must have less than 100 Emacs
packages in their collections (I have about 35). I think the current
performance is good enough to not worry optimizing it at this stage.  In
any case, performance optimizations would need to be made to Emacs'
package.el.

>> There a 6 errors that would need to be looked into, but I these look
>> like actual packaging problems rather than new issues.  The
>> previously
>> used way to load the autoloads, '(load f 'noerror)' would have masked
>> them.

> Regardless of how we handle site-lisp going forward, please do look
> into those issues and perhaps file them against the individual packages
> if they also cause (other) trouble using the current setup.

I'll try having a look, thanks for the reminder.

Thank you for commenting!

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45316: [PATCH]: Re-introduce Emacs packages specific installation prefix.
  2020-12-22 18:09         ` Maxim Cournoyer
@ 2020-12-22 19:10           ` Leo Prikler
  2020-12-26  5:01             ` Maxim Cournoyer
  0 siblings, 1 reply; 35+ messages in thread
From: Leo Prikler @ 2020-12-22 19:10 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 45316

Hi Maxim,

Am Dienstag, den 22.12.2020, 13:09 -0500 schrieb Maxim Cournoyer:
> Hi Leo,
> 
> Leo Prikler <leo.prikler@student.tugraz.at> writes:
> 
> > Hello Maxim,
> > 
> > As someone, who lobbied for the current status quo, I have some
> > thoughts to share.
> 
> I'm happy you're commenting :-).
> 
> > Am Montag, den 21.12.2020, 22:28 -0500 schrieb Maxim Cournoyer:
> > > The Emacs packages built with the Emacs built system used to be
> > > installed in a sub-directory under the share/emacs/guix.d/
> > > directory,
> > > but this was changed in commit
> > > 65a7dd2950ca13a8b942b2836260a2192351b271
> > > shortly after having accommodated the site-start.el machinery to
> > > enable
> > > loading packages from any profile (via the EMACSLOADPATH search
> > > path
> > > specification).
> > Won't this reintroduce <https://bugs.gnu.org/38309> then?
> 
> This bug was caused by the EMACSLOADPATH environment variable being
> too
> long.  This was because the original search path specification was
> recursively adding any directories under site-lisp/ so that package
> installed under a guix.d sub-directory could were directly added to
> the
> load path.
> 
> In the current situation, the EMACSLOADPATH contains only the site-
> lisp/
> directory of any profile, plus the versioned lisp/ directory of the
> installed Emacs package.  This patch series doesn't change that, so
> no,
> that bug wouldn't be reintroduced.
Which is also the reason why our startup code changes and people have
to accommodate to that, right?

> > > While this change allowed to expose simply and directly the
> > > packages
> > > found in EMACSLOADPATH, it does introduce the risk of file name
> > > collisions when multiple Emacs packages are joined in the same
> > > profile,
> > > especially with Emacs packages increasing in complexity (e.g.,
> > > using
> > > more than a single .el file!) and expecting to have both their
> > > sources
> > > and resources extracted under their own nested directory rather
> > > than
> > > as
> > > a flat collection (ELPA, MELPA).
> > > One recent example I stumbled on was attempting to use the
> > > emacs-yasnippet-snippets package along with emacs-elpy; both
> > > wanted
> > > to
> > > install a 'snippets' directory to share/emacs/site-lisp/snippets,
> > > collided and resulted in problems that prove difficult to
> > > understand.
> > I believe that to be a problem in those packages.  Data should not
> > be
> > installed into share/emacs/site-lisp, but share/emacs/etc.  See for
> > instance also emacs-telega, which – while not quite adhering to the
> > above – installs its data in share/emacs/{telega-contrib,telega-
> > data}.
> > 
> > Regardless of what you intend to do with site-lisp otherwise, data
> > files should *not*, I repeat *not* be installed there.  I do
> > believe
> > standardizing share/emacs/etc is the way to go, however.
> 
> While I agree that it would be more elegant the way you propose,
> that's
> not the way Emacs packages have been standardized.  So going against
> the
> single "content directory" (c.f. info "(elisp) Packaging Basics")
> would
> break many Elisp library assumptions about where there files are,
> causing more friction (thus work) to adapt them in Guix.  The content
> directory of an Elisp package can contain any kind of files (code,
> images, etc.), according to info "(elisp) Multi-file Packages".

I suppose said manual is perhaps slightly outdated.
> A package is either a “simple package” or a “multi-file package”.  A
> simple package is stored in a package archive as a single Emacs Lisp
> file, while a multi-file package is stored as a tar file (containing
> multiple Lisp files, and possibly non-Lisp files such as a manual).
When was the last time, you've installed a tar to site-lisp?  I also
feel as though the expected contents are limited very much to README,
COPYING and the like:

--8<---------------cut here--------------begin-------------->8---
$ find ~/.guix-profile/share/emacs/27.1/lisp/ -type f \
  -not -name '*.el' \
  -and -not -name '*.elc' \
  -and -not -name '*.el.gz'
~/.guix-profile/share/emacs/27.1/lisp/term/README
~/.guix-profile/share/emacs/27.1/lisp/COPYING
~/.guix-profile/share/emacs/27.1/lisp/README
--8<---------------cut here---------------end--------------->8---

Perhaps we should ask Emacs folks how they feel about separating code
and data, but bugs have already been caused here and elsewhere by
stuffing them together.  (I believe yasnippet was once already the
culprit at some point before it got reorganized.)  In the meantime,
yes, doing so might cause extra work, but
1) it only affects a subset of packages, and
2) of this subset, we can prioritize those, that do exhibit this
behaviour.

> > > This is what motivated this patch series, where the site-start.el
> > > auxiliary code used for package discovery is extended to support
> > > packages installed in their own directory under a
> > > 'share/emacs/guix'
> > > installation prefix, via Emacs' own package library!
> > > 
> > > The emacs-build-system is updated for this new installation
> > > prefix,
> > > as
> > > well as existing packages and documentation.  Parting with a
> > > directly
> > > usable EMACSLOADPATH means that site-start.el *must* run for
> > > packages
> > > to
> > > appear in the load-path; that means for running a test suite, the
> > > -Q
> > > or
> > > --quick Emacs options cannot be used, since it implies --no-site-
> > > file.
> > Having no usable EMACSLOADPATH sounds like a dealbreaker to
> > me.  Could
> > one be restored by using direct subdirectories to share/emacs/site-
> > lisp
> > or would that merely be a cosmetic change?
> 
> We have two schemes to accommodate for our Emacs packages:
> 
> 1. Those installed via their own mean, e.g. make install, using the
> gnu-build-system for example.  These would still typically install
> their
> packages directly under site-lisp, possibly multiple files (that
> could
> still collide).
Why?  Seems inconsistent, does it not?

> 2. Those installed via the emacs-build-system.  With the proposed
> changes, those now go to site-lisp/guix/.  The 'guix' sub-directory
> makes it unambiguous that anything found under is to be loaded by
> package.el; the `package-directory-list' variable can be pointed to
> it
> to have the Emacs' package library discover these self-contained
> packages.
I think you've lost me here.  Basically, instead of being able to
`(require 'my-package)` you first need to load my-package through
package.el and then can `(require 'my-package)`?  I am not sure, what
the benefit of that would be, if any. 

> Currently if you use -Q, the Elisp libraries are in the load-path,
> but
> not loaded (you need manually do M-x load-library before you can use
> it), or call M-x guix-emacs-autoload-packages to load their
> autoloads.
> For programs, this mean (require 'some-library) works, but M-x
> some-library-autoloaded-function doesn't.
> 
> After this change, if you use -Q the new style Emacs packages are not
> visible at all until you activate them with 'M-x load-library
> guix-emacs' then 'M-x guix-package-initialize', or (require 'guix-
> emacs)
> then (guix-package-initialize), programmatically.
I do think, that there are legitimate reasons to not require a full
initialization here, particularly for the use in batch scripts, but I'm
not quite sure how much work we currently put into making sure that
everything is available.

> I don't see this as a problem.  -Q is simply an alias for
> "--no-init-file" "--no-site-lisp" "--no-splash" "--no-x-resources"
> "--no-site-file".
But not "--no-load-path" "--no-nothing" ;)
Being able to load the same libraries as without is vital for debugging
and scripts.  (Granted, some distros probably break with --no-site-
lisp, but that's nothing to strive for imo.)

> [...]
> When put in perspective, that's 3 seconds to refresh about 900
> packages.  I'm guessing most users must have less than 100 Emacs
> packages in their collections (I have about 35). I think the current
> performance is good enough to not worry optimizing it at this stage.
Point taken, I don't have that many either.  I was merely interested,
given that I rarely dive that deep into runtime statistics of Elisp
code.

Regards,
Leo





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45316: [PATCH]: Re-introduce Emacs packages specific installation prefix.
  2020-12-22 19:10           ` Leo Prikler
@ 2020-12-26  5:01             ` Maxim Cournoyer
  2020-12-26 10:56               ` Leo Prikler
  0 siblings, 1 reply; 35+ messages in thread
From: Maxim Cournoyer @ 2020-12-26  5:01 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45316

Hi Leo!

Leo Prikler <leo.prikler@student.tugraz.at> writes:

> Hi Maxim,
>
> Am Dienstag, den 22.12.2020, 13:09 -0500 schrieb Maxim Cournoyer:
>> Hi Leo,
>>
>> Leo Prikler <leo.prikler@student.tugraz.at> writes:
>>
>> > Hello Maxim,
>> >
>> > As someone, who lobbied for the current status quo, I have some
>> > thoughts to share.
>>
>> I'm happy you're commenting :-).
>>
>> > Am Montag, den 21.12.2020, 22:28 -0500 schrieb Maxim Cournoyer:
>> > > The Emacs packages built with the Emacs built system used to be
>> > > installed in a sub-directory under the share/emacs/guix.d/
>> > > directory,
>> > > but this was changed in commit
>> > > 65a7dd2950ca13a8b942b2836260a2192351b271
>> > > shortly after having accommodated the site-start.el machinery to
>> > > enable
>> > > loading packages from any profile (via the EMACSLOADPATH search
>> > > path
>> > > specification).
>> > Won't this reintroduce <https://bugs.gnu.org/38309> then?
>>
>> This bug was caused by the EMACSLOADPATH environment variable being
>> too
>> long.  This was because the original search path specification was
>> recursively adding any directories under site-lisp/ so that package
>> installed under a guix.d sub-directory could were directly added to
>> the
>> load path.
>>
>> In the current situation, the EMACSLOADPATH contains only the site-
>> lisp/
>> directory of any profile, plus the versioned lisp/ directory of the
>> installed Emacs package.  This patch series doesn't change that, so
>> no,
>> that bug wouldn't be reintroduced.

> Which is also the reason why our startup code changes and people have
> to accommodate to that, right?

Yes. The startup code is now responsible to activate packages found not
directly in the EMACSLOADPATH (share/emacs/site-lisp), but in
sub-directories under share/emacs/site-lisp/guix.

>> > > While this change allowed to expose simply and directly the
>> > > packages
>> > > found in EMACSLOADPATH, it does introduce the risk of file name
>> > > collisions when multiple Emacs packages are joined in the same
>> > > profile,
>> > > especially with Emacs packages increasing in complexity (e.g.,
>> > > using
>> > > more than a single .el file!) and expecting to have both their
>> > > sources
>> > > and resources extracted under their own nested directory rather
>> > > than
>> > > as
>> > > a flat collection (ELPA, MELPA).
>> > > One recent example I stumbled on was attempting to use the
>> > > emacs-yasnippet-snippets package along with emacs-elpy; both
>> > > wanted
>> > > to
>> > > install a 'snippets' directory to share/emacs/site-lisp/snippets,
>> > > collided and resulted in problems that prove difficult to
>> > > understand.
>> > I believe that to be a problem in those packages.  Data should not
>> > be
>> > installed into share/emacs/site-lisp, but share/emacs/etc.  See for
>> > instance also emacs-telega, which – while not quite adhering to the
>> > above – installs its data in share/emacs/{telega-contrib,telega-
>> > data}.
>> >
>> > Regardless of what you intend to do with site-lisp otherwise, data
>> > files should *not*, I repeat *not* be installed there.  I do
>> > believe
>> > standardizing share/emacs/etc is the way to go, however.
>>
>> While I agree that it would be more elegant the way you propose,
>> that's
>> not the way Emacs packages have been standardized.  So going against
>> the
>> sin--8<---------------cut here---------------start------------->8---
--8<---------------cut here---------------start------------->8---
gle "content directory" (c.f. info "(elisp) Packaging Basics")
>> would
>> break many Elisp library assumptions about where there files are,
>> causing more friction (thus work) to adapt them in Guix.  The content
>> directory of an Elisp package can contain any kind of files (code,
>> images, etc.), according to info "(elisp) Multi-file Packages".
>
> I suppose said manual is perhaps slightly outdated.

The doc/package.texi file of the Emacs git repository is actively
maintained, although cited contents above hasn't changed much for the
last 8 years.  In any case if you look into package.el, you'll see that
package contents are fetched as a single archived, and its content
installed to a single directory.

>> A package is either a “simple package” or a “multi-file package”.  A
>> simple package is stored in a package archive as a single Emacs Lisp
>> file, while a multi-file package is stored as a tar file (containing
>> multiple Lisp files, and possibly non-Lisp files such as a manual).
> When was the last time, you've installed a tar to site-lisp?  I also
> feel as though the expected contents are limited very much to README,
> COPYING and the like:
>
> $ find ~/.guix-profile/share/emacs/27.1/lisp/ -type f \
>   -not -name '*.el' \
>   -and -not -name '*.elc' \
>   -and -not -name '*.el.gz'
> ~/.guix-profile/share/emacs/27.1/lisp/term/README
> ~/.guix-profile/share/emacs/27.1/lisp/COPYING
> ~/.guix-profile/share/emacs/27.1/lisp/README

The lisp/ directory is for "the standard Lisp files that come with
Emacs" (from info "(elisp)Library Search").  I don't think we can draw
much comparison between these and user-installable packages from ELPA or
MELPA.

Also, just to be clear, the tar is not installed as-is; it is extracted
and its contents are installed in the "content directory".  Citing again
'(elisp) Multi-file Packages':

    Prior to installation, a multi-file package is stored in a package
    archive as a tar file.  The tar file must be named ‘NAME-VERSION.tar’,
    where NAME is the package name and VERSION is the version number.  Its
    contents, once extracted, must all appear in a directory named
    ‘NAME-VERSION’, the “content directory” (*note Packaging Basics::).
    Files may also extract into subdirectories of the content directory.

       One of the files in the content directory must be named
    ‘NAME-pkg.el’.  It must contain a single Lisp form, consisting of a call
    to the function ‘define-package’, described below.  This defines the
    package’s attributes: version, brief description, and requirements.

       For example, if we distribute version 1.3 of the superfrobnicator as
    a multi-file package, the tar file would be ‘superfrobnicator-1.3.tar’.
    Its contents would extract into the directory ‘superfrobnicator-1.3’,
    and one of these would be the file ‘superfrobnicator-pkg.el’.

If you want to have a clearer idea of how packages from ELPA and the
likes are installed, you can have a peek into the 'package.el' file
shipped with Emacs (spoiler: it's basically just extracting the contents
of the package archive to a single directory -- see the `package-unpack'
procedure).

Here's an experiment I've done with a profile containing the whole of
our current emacs-build-system based packages, with this current change,
that checks for collision at the top level of a package installation
directory:

--8<---------------cut here---------------end--------------->8---
$ ./pre-inst-env guix package -p /tmp/nnew-emacs -m emacs-packages-manifest.scm

$  find -L /tmp/nnew-emacs/share/emacs/site-lisp/guix -maxdepth 2
-mindepth 2 -not -regex '.*\.elc?$' \
          | awk -F '/' '{ if ($9 in packages) \
            {printf("%s directory of %s package collides with that of package %s\n", $9, $8, packages[$9])} \
            else {packages[$9] = $8} }'

doc directory of modus-operandi-theme-1.0.2 package collides with that of package racket-mode-0.0.2-6.5eb31a2
tools directory of unidecode-0.2-1.5502ada package collides with that of package company-cabal-0.3.0-1.62112a7
snippets directory of minitest-0.8.0-1.1aadb78 package collides with that of package elpy-1.35.0
test directory of realgud-1.5.1 package collides with that of package flycheck-haskell-0.8-2.32ddff8
snippets directory of yasnippet-snippets-0.23 package collides with that of package elpy-1.35.0
snippets directory of haskell-snippets-0.1.0-0.07b0f46 package collides with that of package elpy-1.35.0
snippets directory of rspec-1.11-1.66ea7cc package collides with that of package elpy-1.35.0
doc directory of modus-themes-1.0.2 package collides with that of package racket-mode-0.0.2-6.5eb31a2
data directory of emojify-1.2 package collides with that of package unidecode-0.2-1.5502ada
test directory of systemd-mode-1.6 package collides with that of package flycheck-haskell-0.8-2.32ddff8
lib directory of slime-2.26.1 package collides with that of package robe-0.8.2
contrib directory of sly-1.0.0-7.68561f1 package collides with that of package slime-2.26.1
lib directory of sly-1.0.0-7.68561f1 package collides with that of package robe-0.8.2
doc directory of modus-vivendi-theme-1.0.2 package collides with that of package racket-mode-0.0.2-6.5eb31a2
doc directory of evil-1.14.0 package collides with that of package racket-mode-0.0.2-6.5eb31a2
data directory of all-the-icons-4.0.1 package collides with that of package unidecode-0.2-1.5502ada
snippets directory of feature-mode-20190801-1.11ae167 package collides with that of package elpy-1.35.0
--8<---------------cut here---------------end--------------->8---

So 17 Emacs packages in Guix currently conflict, and Guix seems to be
silent about it when building a profile with them via 'guix package -p'
(a bug?).

> Perhaps we should ask Emacs folks how they feel about separating code
> and data, but bugs have already been caused here and elsewhere by
> stuffing them together.  (I believe yasnippet was once already the
> culprit at some point before it got reorganized.)  In the meantime,
> yes, doing so might cause extra work, but
> 1) it only affects a subset of packages, and
> 2) of this subset, we can prioritize those, that do exhibit this
> behaviour.

The above does output does show that's it's a subset of the packages
that are affected.

[...]

>> We have two schemes to accommodate for our Emacs packages:
>>
>> 1. Those installed via their own mean, e.g. make install, using the
>> gnu-build-system for example.  These would still typically install
>> their
>> packages directly under site-lisp, possibly multiple files (that
>> could
>> still collide).
> Why?  Seems inconsistent, does it not?

They can be adapted in time; until then it's easy to accommodate both.

>> 2. Those installed via the emacs-build-system.  With the proposed
>> changes, those now go to site-lisp/guix/.  The 'guix' sub-directory
>> makes it unambiguous that anything found under is to be loaded by
>> package.el; the `package-directory-list' variable can be pointed to
>> it
>> to have the Emacs' package library discover these self-contained
>> packages.
> I think you've lost me here.  Basically, instead of being able to
> `(require 'my-package)` you first need to load my-package through
> package.el and then can `(require 'my-package)`?  I am not sure, what
> the benefit of that would be, if any.

Only if you've specified --no-site-start, which would disable
user-installed package discovery.  This is the drawback of the
trade-off, not the benefit :-).

>> Currently if you use -Q, the Elisp libraries are in the load-path,
>> but
>> not loaded (you need manually do M-x load-library before you can use
>> it), or call M-x guix-emacs-autoload-packages to load their
>> autoloads.
>> For programs, this mean (require 'some-library) works, but M-x
>> some-library-autoloaded-function doesn't.
>>
>> After this change, if you use -Q the new style Emacs packages are not
>> visible at all until you activate them with 'M-x load-library
>> guix-emacs' then 'M-x guix-package-initialize', or (require 'guix-
>> emacs)
>> then (guix-package-initialize), programmatically.

> I do think, that there are legitimate reasons to not require a full
> initialization here, particularly for the use in batch scripts, but I'm
> not quite sure how much work we currently put into making sure that
> everything is available.

That's a fair point, although I'm not concerned about the overhead of
loading compiled autoloads files.

>> I don't see this as a problem.  -Q is simply an alias for
>> "--no-init-file" "--no-site-lisp" "--no-splash" "--no-x-resources"
>> "--no-site-file".
> But not "--no-load-path" "--no-nothing" ;)

One could argue that the spirit of -Q is to give an Emacs as minimal as
possible (that's what the Emacs manual has to say about it, and the
implicit --no-site-lisp has the meaning of "no user installed
packages").

> Being able to load the same libraries as without is vital for debugging
> and scripts.  (Granted, some distros probably break with --no-site-
> lisp, but that's nothing to strive for imo.)

I think you meant --no-site-file here.  There's not much magic; if you
allow it to run, user installed packages are added to the load path and
autoloaded; if you don't you don't then you only get Emacs standard
libraries.  Nothing else happens in the site-start.el file in Guix
proposed here.  Scripting works well enough that our 900'ish packages
can be rebuilt with the change, many of which come with a test suite.

Thanks for having me think harder about if this is really
desirable/needed.  For me the main plus is that we'd be adhering to the
current standards used for Emacs packaging: everything is unpacked into
a single directory owned by that package, so as long as we include any
needed resources in the #:include argument of the emacs-build-system, it
works as upstream intended it, with no need to patch variables or
anything.  I expect this situation to worsen as Emacs packages tend to
get more complicated, and I don't feel strongly enough about it to go
arguing about Emacs packaging standards on the Emacs mailing list :-).

On the minus side it makes the startup slightly more expensive in terms
of pure scripting, and requires the users to be mindful that
site-start.el evaluation is needed for the user installed packages to be
activated.

Further thoughts?

Thanks again,

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45316: [PATCH]: Re-introduce Emacs packages specific installation prefix.
  2020-12-26  5:01             ` Maxim Cournoyer
@ 2020-12-26 10:56               ` Leo Prikler
  2020-12-27  4:44                 ` Maxim Cournoyer
  0 siblings, 1 reply; 35+ messages in thread
From: Leo Prikler @ 2020-12-26 10:56 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 45316

Hi Maxim,

Am Samstag, den 26.12.2020, 00:01 -0500 schrieb Maxim Cournoyer:
> > > > > While this change allowed to expose simply and directly the
> > > > > packages
> > > > > found in EMACSLOADPATH, it does introduce the risk of file
> > > > > name
> > > > > collisions when multiple Emacs packages are joined in the
> > > > > same
> > > > > profile,
> > > > > especially with Emacs packages increasing in complexity
> > > > > (e.g.,
> > > > > using
> > > > > more than a single .el file!) and expecting to have both
> > > > > their
> > > > > sources
> > > > > and resources extracted under their own nested directory
> > > > > rather
> > > > > than
> > > > > as
> > > > > a flat collection (ELPA, MELPA).
> > > > > One recent example I stumbled on was attempting to use the
> > > > > emacs-yasnippet-snippets package along with emacs-elpy; both
> > > > > wanted
> > > > > to
> > > > > install a 'snippets' directory to share/emacs/site-
> > > > > lisp/snippets,
> > > > > collided and resulted in problems that prove difficult to
> > > > > understand.
> > > > I believe that to be a problem in those packages.  Data should
> > > > not
> > > > be
> > > > installed into share/emacs/site-lisp, but share/emacs/etc.  See
> > > > for
> > > > instance also emacs-telega, which – while not quite adhering to
> > > > the
> > > > above – installs its data in share/emacs/{telega-
> > > > contrib,telega-
> > > > data}.
> > > > 
> > > > Regardless of what you intend to do with site-lisp otherwise,
> > > > data
> > > > files should *not*, I repeat *not* be installed there.  I do
> > > > believe
> > > > standardizing share/emacs/etc is the way to go, however.
> > > 
> > > While I agree that it would be more elegant the way you propose,
> > > that's
> > > not the way Emacs packages have been standardized.  So going
> > > against
> > > the
> > > sin--8<---------------cut here---------------start-------------
> > > >8---
> --8<---------------cut here---------------start------------->8---
> gle "content directory" (c.f. info "(elisp) Packaging Basics")
> > > would
> > > break many Elisp library assumptions about where there files are,
> > > causing more friction (thus work) to adapt them in Guix.  The
> > > content
> > > directory of an Elisp package can contain any kind of files
> > > (code,
> > > images, etc.), according to info "(elisp) Multi-file Packages".
> > 
> > I suppose said manual is perhaps slightly outdated.
> 
> The doc/package.texi file of the Emacs git repository is actively
> maintained, although cited contents above hasn't changed much for the
> last 8 years.  In any case if you look into package.el, you'll see
> that
> package contents are fetched as a single archived, and its content
> installed to a single directory.
> 
> > > A package is either a “simple package” or a “multi-file
> > > package”.  A
> > > simple package is stored in a package archive as a single Emacs
> > > Lisp
> > > file, while a multi-file package is stored as a tar file
> > > (containing
> > > multiple Lisp files, and possibly non-Lisp files such as a
> > > manual).
> > When was the last time, you've installed a tar to site-lisp?  I
> > also
> > feel as though the expected contents are limited very much to
> > README,
> > COPYING and the like:
> > 
> > $ find ~/.guix-profile/share/emacs/27.1/lisp/ -type f \
> >   -not -name '*.el' \
> >   -and -not -name '*.elc' \
> >   -and -not -name '*.el.gz'
> > ~/.guix-profile/share/emacs/27.1/lisp/term/README
> > ~/.guix-profile/share/emacs/27.1/lisp/COPYING
> > ~/.guix-profile/share/emacs/27.1/lisp/README
> 
> The lisp/ directory is for "the standard Lisp files that come with
> Emacs" (from info "(elisp)Library Search").  I don't think we can
> draw
> much comparison between these and user-installable packages from ELPA
> or
> MELPA.
Fair enough, perhaps I read too much into that.

> Also, just to be clear, the tar is not installed as-is; it is
> extracted
> and its contents are installed in the "content directory".  Citing
> again
> '(elisp) Multi-file Packages':
> 
>     Prior to installation, a multi-file package is stored in a
> package
>     archive as a tar file.  The tar file must be named ‘NAME-
> VERSION.tar’,
>     where NAME is the package name and VERSION is the version
> number.  Its
>     contents, once extracted, must all appear in a directory named
>     ‘NAME-VERSION’, the “content directory” (*note Packaging
> Basics::).
>     Files may also extract into subdirectories of the content
> directory.
> 
>        One of the files in the content directory must be named
>     ‘NAME-pkg.el’.  It must contain a single Lisp form, consisting of
> a call
>     to the function ‘define-package’, described below.  This defines
> the
>     package’s attributes: version, brief description, and
> requirements.
> 
>        For example, if we distribute version 1.3 of the
> superfrobnicator as
>     a multi-file package, the tar file would be ‘superfrobnicator-
> 1.3.tar’.
>     Its contents would extract into the directory ‘superfrobnicator-
> 1.3’,
>     and one of these would be the file ‘superfrobnicator-pkg.el’.
> 
> If you want to have a clearer idea of how packages from ELPA and the
> likes are installed, you can have a peek into the 'package.el' file
> shipped with Emacs (spoiler: it's basically just extracting the
> contents
> of the package archive to a single directory -- see the `package-
> unpack'
> procedure).
If we follow that to the letter, should we not install those packages
directly to site-lisp/$PACKAGE-$VERSION?  Why the guix directory?

Also, if we do go that route, I think we should install a subdirs.el
through a profile hook, that adds all these package directories to the
load path.  See also '(elisp) Startup Summary'.  Alternatively, we can
let subdirs.el defer to guix-emacs.el, but that'd be very cheeky. 
Either way, we should definitely make -Q work.

> Here's an experiment I've done with a profile containing the whole of
> our current emacs-build-system based packages, with this current
> change,
> that checks for collision at the top level of a package installation
> directory:
> 
> --8<---------------cut here---------------end--------------->8---
> $ ./pre-inst-env guix package -p /tmp/nnew-emacs -m emacs-packages-
> manifest.scm
> 
> $  find -L /tmp/nnew-emacs/share/emacs/site-lisp/guix -maxdepth 2
> -mindepth 2 -not -regex '.*\.elc?$' \
>           | awk -F '/' '{ if ($9 in packages) \
>             {printf("%s directory of %s package collides with that of
> package %s\n", $9, $8, packages[$9])} \
>             else {packages[$9] = $8} }'
> 
> doc directory of modus-operandi-theme-1.0.2 package collides with
> that of package racket-mode-0.0.2-6.5eb31a2
> tools directory of unidecode-0.2-1.5502ada package collides with that
> of package company-cabal-0.3.0-1.62112a7
> snippets directory of minitest-0.8.0-1.1aadb78 package collides with
> that of package elpy-1.35.0
> test directory of realgud-1.5.1 package collides with that of package
> flycheck-haskell-0.8-2.32ddff8
> snippets directory of yasnippet-snippets-0.23 package collides with
> that of package elpy-1.35.0
> snippets directory of haskell-snippets-0.1.0-0.07b0f46 package
> collides with that of package elpy-1.35.0
> snippets directory of rspec-1.11-1.66ea7cc package collides with that
> of package elpy-1.35.0
> doc directory of modus-themes-1.0.2 package collides with that of
> package racket-mode-0.0.2-6.5eb31a2
> data directory of emojify-1.2 package collides with that of package
> unidecode-0.2-1.5502ada
> test directory of systemd-mode-1.6 package collides with that of
> package flycheck-haskell-0.8-2.32ddff8
> lib directory of slime-2.26.1 package collides with that of package
> robe-0.8.2
> contrib directory of sly-1.0.0-7.68561f1 package collides with that
> of package slime-2.26.1
> lib directory of sly-1.0.0-7.68561f1 package collides with that of
> package robe-0.8.2
> doc directory of modus-vivendi-theme-1.0.2 package collides with that
> of package racket-mode-0.0.2-6.5eb31a2
> doc directory of evil-1.14.0 package collides with that of package
> racket-mode-0.0.2-6.5eb31a2
> data directory of all-the-icons-4.0.1 package collides with that of
> package unidecode-0.2-1.5502ada
> snippets directory of feature-mode-20190801-1.11ae167 package
> collides with that of package elpy-1.35.0
> --8<---------------cut here---------------end--------------->8---
> 
> So 17 Emacs packages in Guix currently conflict, and Guix seems to be
> silent about it when building a profile with them via 'guix package
> -p'
> (a bug?).
It's a bug in those packages; not in Guix.  The whole idea of Guix is,
that such directories are merged.  Now collisions within files in those
directories are a different topic – only one can be chosen there and
IIRC there should also be a way to make them errors.

> > Perhaps we should ask Emacs folks how they feel about separating
> > code
> > and data, but bugs have already been caused here and elsewhere by
> > stuffing them together.  (I believe yasnippet was once already the
> > culprit at some point before it got reorganized.)  In the meantime,
> > yes, doing so might cause extra work, but
> > 1) it only affects a subset of packages, and
> > 2) of this subset, we can prioritize those, that do exhibit this
> > behaviour.
> 
> The above does output does show that's it's a subset of the packages
> that are affected.
> 
> [...]
> 
> > > We have two schemes to accommodate for our Emacs packages:
> > > 
> > > 1. Those installed via their own mean, e.g. make install, using
> > > the
> > > gnu-build-system for example.  These would still typically
> > > install
> > > their
> > > packages directly under site-lisp, possibly multiple files (that
> > > could
> > > still collide).
> > Why?  Seems inconsistent, does it not?
> 
> They can be adapted in time; until then it's easy to accommodate
> both.
I think we should aim for consistency here, but let's adapt them in
follow-up commits.

> > > 2. Those installed via the emacs-build-system.  With the proposed
> > > changes, those now go to site-lisp/guix/.  The 'guix' sub-
> > > directory
> > > makes it unambiguous that anything found under is to be loaded by
> > > package.el; the `package-directory-list' variable can be pointed
> > > to
> > > it
> > > to have the Emacs' package library discover these self-contained
> > > packages.
> > I think you've lost me here.  Basically, instead of being able to
> > `(require 'my-package)` you first need to load my-package through
> > package.el and then can `(require 'my-package)`?  I am not sure,
> > what
> > the benefit of that would be, if any.
> 
> Only if you've specified --no-site-start, which would disable
> user-installed package discovery.  This is the drawback of the
> trade-off, not the benefit :-).
It wasn't worded like one.  The word unambiguous typically has a
positive connotation in my mind.  And package discovery should function
regardless of site-start.

> > > Currently if you use -Q, the Elisp libraries are in the load-
> > > path,
> > > but
> > > not loaded (you need manually do M-x load-library before you can
> > > use
> > > it), or call M-x guix-emacs-autoload-packages to load their
> > > autoloads.
> > > For programs, this mean (require 'some-library) works, but M-x
> > > some-library-autoloaded-function doesn't.
> > > 
> > > After this change, if you use -Q the new style Emacs packages are
> > > not
> > > visible at all until you activate them with 'M-x load-library
> > > guix-emacs' then 'M-x guix-package-initialize', or (require
> > > 'guix-
> > > emacs)
> > > then (guix-package-initialize), programmatically.
> > I do think, that there are legitimate reasons to not require a full
> > initialization here, particularly for the use in batch scripts, but
> > I'm
> > not quite sure how much work we currently put into making sure that
> > everything is available.
> 
> That's a fair point, although I'm not concerned about the overhead of
> loading compiled autoloads files.
> 
> > > I don't see this as a problem.  -Q is simply an alias for
> > > "--no-init-file" "--no-site-lisp" "--no-splash" "--no-x-
> > > resources"
> > > "--no-site-file".
> > But not "--no-load-path" "--no-nothing" ;)
> 
> One could argue that the spirit of -Q is to give an Emacs as minimal
> as
> possible (that's what the Emacs manual has to say about it, and the
> implicit --no-site-lisp has the meaning of "no user installed
> packages").
Well then the spirit of -Q is lost.

> > Being able to load the same libraries as without is vital for
> > debugging
> > and scripts.  (Granted, some distros probably break with --no-site-
> > lisp, but that's nothing to strive for imo.)
> 
> [...]
> 
> Thanks for having me think harder about if this is really
> desirable/needed.  For me the main plus is that we'd be adhering to
> the
> current standards used for Emacs packaging: everything is unpacked
> into
> a single directory owned by that package, so as long as we include
> any
> needed resources in the #:include argument of the emacs-build-system, 
> it
> works as upstream intended it, with no need to patch variables or
> anything.  I expect this situation to worsen as Emacs packages tend
> to
> get more complicated, and I don't feel strongly enough about it to go
> arguing about Emacs packaging standards on the Emacs mailing list :-
> ).
I agree, that adhering to the standards sounds nice, but this patch
makes it seem as if we're still cooking our own soup.  I believe we'd
need to adhere to them harder than that in order for it to be an
upgrade of the status quo.

> On the minus side it makes the startup slightly more expensive in
> terms
> of pure scripting, and requires the users to be mindful that
> site-start.el evaluation is needed for the user installed packages to
> be
> activated.
> 
> Further thoughts?
I think I've already laid that out above, but we really ought to have
site-lisp/$PACKAGE-$VERSION and a working subdirs.el.  If we do it like
that, I don't think the multi-directory layout should cause us too many
troubles.

As far as the build system is concerned, we would probably need to set
up an environment similar to what will be found in the end.
Imagine this:

- environment-variables
- $PACKAGE-$VERSION/
- $PACKAGE-$VERSION-inputs/
  - subdirs.el
  - $INPUT1/
  - $INPUT2/
  - ...

Regards,
Leo





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45316: [PATCH]: Re-introduce Emacs packages specific installation prefix.
  2020-12-26 10:56               ` Leo Prikler
@ 2020-12-27  4:44                 ` Maxim Cournoyer
  2020-12-27  8:46                   ` Leo Prikler
  0 siblings, 1 reply; 35+ messages in thread
From: Maxim Cournoyer @ 2020-12-27  4:44 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 45316

Hi Leo,

Leo Prikler <leo.prikler@student.tugraz.at> writes:

[...]

>> If you want to have a clearer idea of how packages from ELPA and the
>> likes are installed, you can have a peek into the 'package.el' file
>> shipped with Emacs (spoiler: it's basically just extracting the
>> contents
>> of the package archive to a single directory -- see the `package-
>> unpack'
>> procedure).
> If we follow that to the letter, should we not install those packages
> directly to site-lisp/$PACKAGE-$VERSION?  Why the guix directory?

I used to wonder the same, but after getting familiar with the 'package'
Emacs library, it defaults (per the default value of the
`package-directory-list' variable) to such a layout:

directory-on-load-path/prefix/name-version/

where 'prefix' defaults to 'elpa'.  It seems a good idea to have the
ELPA-style packages live by themselves, separated from the 'classic'
packages that are installed directly to the load path, to avoid the
package.el loader getting confused or scanning entries it wouldn't know
how to load anyway.

That said, it's shouldn't be strictly required, as old style packages or
resource directories would be lacking a *-pkg.el file and would be
disregarded, and it should be possible to install the package
directories directly to the load path if we wanted to.

> Also, if we do go that route, I think we should install a subdirs.el
> through a profile hook, that adds all these package directories to the
> load path.  See also '(elisp) Startup Summary'.  Alternatively, we can
> let subdirs.el defer to guix-emacs.el, but that'd be very cheeky.
> Either way, we should definitely make -Q work.

If we use a subdirs.el file, it could add the ELPA-style packages more
simply if they are under the guix sub-directory (guix/*).  Otherwise
we'd have to filter the resource directories that potentially find their
ways to the load path for old style packages.  Our custom subdirs.el
file could be shipped with our Emacs packages, installed in their output
at share/emacs/site-lisp/subdirs.el.  When a profile union would be
created this subdirs.el would automatically be made available.

>> Here's an experiment I've done with a profile containing the whole of
>> our current emacs-build-system based packages, with this current
>> change,
>> that checks for collision at the top level of a package installation
>> directory:
>>
>> --8<---------------cut here---------------end--------------->8---
>> $ ./pre-inst-env guix package -p /tmp/nnew-emacs -m emacs-packages-
>> manifest.scm
>>
>> $  find -L /tmp/nnew-emacs/share/emacs/site-lisp/guix -maxdepth 2
>> -mindepth 2 -not -regex '.*\.elc?$' \
>>           | awk -F '/' '{ if ($9 in packages) \
>>             {printf("%s directory of %s package collides with that of
>> package %s\n", $9, $8, packages[$9])} \
>>             else {packages[$9] = $8} }'
>>
>> doc directory of modus-operandi-theme-1.0.2 package collides with
>> that of package racket-mode-0.0.2-6.5eb31a2
>> tools directory of unidecode-0.2-1.5502ada package collides with that
>> of package company-cabal-0.3.0-1.62112a7
>> snippets directory of minitest-0.8.0-1.1aadb78 package collides with
>> that of package elpy-1.35.0
>> test directory of realgud-1.5.1 package collides with that of package
>> flycheck-haskell-0.8-2.32ddff8
>> snippets directory of yasnippet-snippets-0.23 package collides with
>> that of package elpy-1.35.0
>> snippets directory of haskell-snippets-0.1.0-0.07b0f46 package
>> collides with that of package elpy-1.35.0
>> snippets directory of rspec-1.11-1.66ea7cc package collides with that
>> of package elpy-1.35.0
>> doc directory of modus-themes-1.0.2 package collides with that of
>> package racket-mode-0.0.2-6.5eb31a2
>> data directory of emojify-1.2 package collides with that of package
>> unidecode-0.2-1.5502ada
>> test directory of systemd-mode-1.6 package collides with that of
>> package flycheck-haskell-0.8-2.32ddff8
>> lib directory of slime-2.26.1 package collides with that of package
>> robe-0.8.2
>> contrib directory of sly-1.0.0-7.68561f1 package collides with that
>> of package slime-2.26.1
>> lib directory of sly-1.0.0-7.68561f1 package collides with that of
>> package robe-0.8.2
>> doc directory of modus-vivendi-theme-1.0.2 package collides with that
>> of package racket-mode-0.0.2-6.5eb31a2
>> doc directory of evil-1.14.0 package collides with that of package
>> racket-mode-0.0.2-6.5eb31a2
>> data directory of all-the-icons-4.0.1 package collides with that of
>> package unidecode-0.2-1.5502ada
>> snippets directory of feature-mode-20190801-1.11ae167 package
>> collides with that of package elpy-1.35.0
>> --8<---------------cut here---------------end--------------->8---
>>
>> So 17 Emacs packages in Guix currently conflict, and Guix seems to be
>> silent about it when building a profile with them via 'guix package
>> -p'
>> (a bug?).

> It's a bug in those packages; not in Guix.

I used to think the same, but after reading the Elisp reference manual
mentioning how Emacs packages to be installed with package.el are
expected to live in their own distinct directory with their own
resources; we can't really say that's it's a bug in the packages: they
just weren't designed to be merged in a flat directory with other Elisp
packages.  Users installing these packages manually can simply add their
top level directory to their load path, else use package.el to install
them.

> The whole idea of Guix is, that such directories are merged.  Now
> collisions within files in those directories are a different topic –
> only one can be chosen there and IIRC there should also be a way to
> make them errors.

I'll investigate this separately.

[...]

>> > > We have two schemes to accommodate for our Emacs packages:
>> > >
>> > > 1. Those installed via their own mean, e.g. make install, using
>> > > the
>> > > gnu-build-system for example.  These would still typically
>> > > install
>> > > their
>> > > packages directly under site-lisp, possibly multiple files (that
>> > > could
>> > > still collide).
>> > Why?  Seems inconsistent, does it not?
>>
>> They can be adapted in time; until then it's easy to accommodate
>> both.
> I think we should aim for consistency here, but let's adapt them in
> follow-up commits.

Agreed.

>> > > 2. Those installed via the emacs-build-system.  With the proposed
>> > > changes, those now go to site-lisp/guix/.  The 'guix' sub-
>> > > directory
>> > > makes it unambiguous that anything found under is to be loaded by
>> > > package.el; the `package-directory-list' variable can be pointed
>> > > to
>> > > it
>> > > to have the Emacs' package library discover these self-contained
>> > > packages.
>> > I think you've lost me here.  Basically, instead of being able to
>> > `(require 'my-package)` you first need to load my-package through
>> > package.el and then can `(require 'my-package)`?  I am not sure,
>> > what
>> > the benefit of that would be, if any.
>>
>> Only if you've specified --no-site-start, which would disable
>> user-installed package discovery.  This is the drawback of the
>> trade-off, not the benefit :-).
> It wasn't worded like one.  The word unambiguous typically has a
> positive connotation in my mind.  And package discovery should function
> regardless of site-start.

The word unambiguous was used to describe that using the sub-directory
guix/ would mean anything under it would be strictly packages to be
loaded with package.el and nothing else.

[...]

>> > Being able to load the same libraries as without is vital for
>> > debugging
>> > and scripts.  (Granted, some distros probably break with --no-site-
>> > lisp, but that's nothing to strive for imo.)
>>
>> [...]
>>
>> Thanks for having me think harder about if this is really
>> desirable/needed.  For me the main plus is that we'd be adhering to
>> the
>> current standards used for Emacs packaging: everything is unpacked
>> into
>> a single directory owned by that package, so as long as we include
>> any
>> needed resources in the #:include argument of the emacs-build-system,
>> it
>> works as upstream intended it, with no need to patch variables or
>> anything.  I expect this situation to worsen as Emacs packages tend
>> to
>> get more complicated, and I don't feel strongly enough about it to go
>> arguing about Emacs packaging standards on the Emacs mailing list :-
>> ).
> I agree, that adhering to the standards sounds nice, but this patch
> makes it seem as if we're still cooking our own soup.  I believe we'd
> need to adhere to them harder than that in order for it to be an
> upgrade of the status quo.

I understand this may feel a hack; because for all I know, it is ;-).
I'll try improving it with the subdirs.el discussed above.

[...]

> I think I've already laid that out above, but we really ought to have
> site-lisp/$PACKAGE-$VERSION and a working subdirs.el.  If we do it like
> that, I don't think the multi-directory layout should cause us too many
> troubles.
>
> As far as the build system is concerned, we would probably need to set
> up an environment similar to what will be found in the end.
> Imagine this:
>
> - environment-variables
> - $PACKAGE-$VERSION/
> - $PACKAGE-$VERSION-inputs/
>   - subdirs.el
>   - $INPUT1/
>   - $INPUT2/

In the build environment, there is no profile so each package add their
individual site-lisp/ to the load path (EMACSLOADPATH).  With my
proposed idea to add subdirs.el to Emacs itself, there'd be nothing to
do here.

Thanks again for this discussion; I'll work on a revised patch.  Until
then, happy new year!

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45316: [PATCH]: Re-introduce Emacs packages specific installation prefix.
  2020-12-27  4:44                 ` Maxim Cournoyer
@ 2020-12-27  8:46                   ` Leo Prikler
  0 siblings, 0 replies; 35+ messages in thread
From: Leo Prikler @ 2020-12-27  8:46 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 45316

Hi Maxim,
Am Samstag, den 26.12.2020, 23:44 -0500 schrieb Maxim Cournoyer:
> Hi Leo,
> 
> Leo Prikler <leo.prikler@student.tugraz.at> writes:
> 
> [...]
> 
> > > If you want to have a clearer idea of how packages from ELPA and
> > > the
> > > likes are installed, you can have a peek into the 'package.el'
> > > file
> > > shipped with Emacs (spoiler: it's basically just extracting the
> > > contents
> > > of the package archive to a single directory -- see the `package-
> > > unpack'
> > > procedure).
> > If we follow that to the letter, should we not install those
> > packages
> > directly to site-lisp/$PACKAGE-$VERSION?  Why the guix directory?
> 
> I used to wonder the same, but after getting familiar with the
> 'package'
> Emacs library, it defaults (per the default value of the
> `package-directory-list' variable) to such a layout:
> 
> directory-on-load-path/prefix/name-version/
> 
> where 'prefix' defaults to 'elpa'.  It seems a good idea to have the
> ELPA-style packages live by themselves, separated from the 'classic'
> packages that are installed directly to the load path, to avoid the
> package.el loader getting confused or scanning entries it wouldn't
> know
> how to load anyway.
> 
> That said, it's shouldn't be strictly required, as old style packages
> or
> resource directories would be lacking a *-pkg.el file and would be
> disregarded, and it should be possible to install the package
> directories directly to the load path if we wanted to.
I don't really know how to feel about faking elpa.  Also, I don't think
that 'classic' means 'not supported by package.el'.  There are other
reasons to use e.g. gnu-build-system over emacs-build-system and
packages might ship a *-pkg.el even if they do build using a Makefile. 
It's also trivial to provide one if they don't and Guix has the
facilities to do so.

> > Also, if we do go that route, I think we should install a
> > subdirs.el
> > through a profile hook, that adds all these package directories to
> > the
> > load path.  See also '(elisp) Startup Summary'.  Alternatively, we
> > can
> > let subdirs.el defer to guix-emacs.el, but that'd be very cheeky.
> > Either way, we should definitely make -Q work.
> 
> If we use a subdirs.el file, it could add the ELPA-style packages
> more
> simply if they are under the guix sub-directory (guix/*).  Otherwise
> we'd have to filter the resource directories that potentially find
> their
> ways to the load path for old style packages.  Our custom subdirs.el
> file could be shipped with our Emacs packages, installed in their
> output
> at share/emacs/site-lisp/subdirs.el.  When a profile union would be
> created this subdirs.el would automatically be made available.
IOW if we don't use guix/*, we would have to at once fix all our
packages, that don't adhere to this standard.  Sounds like a bit of
trouble, but also like it'd improve consistency.

> > > Here's an experiment I've done with a profile containing the
> > > whole of
> > > our current emacs-build-system based packages, with this current
> > > change,
> > > that checks for collision at the top level of a package
> > > installation
> > > directory:
> > > 
> > > --8<---------------cut here---------------end--------------->8---
> > > $ ./pre-inst-env guix package -p /tmp/nnew-emacs -m emacs-
> > > packages-
> > > manifest.scm
> > > 
> > > $  find -L /tmp/nnew-emacs/share/emacs/site-lisp/guix -maxdepth 2
> > > -mindepth 2 -not -regex '.*\.elc?$' \
> > >           | awk -F '/' '{ if ($9 in packages) \
> > >             {printf("%s directory of %s package collides with
> > > that of
> > > package %s\n", $9, $8, packages[$9])} \
> > >             else {packages[$9] = $8} }'
> > > 
> > > doc directory of modus-operandi-theme-1.0.2 package collides with
> > > that of package racket-mode-0.0.2-6.5eb31a2
> > > tools directory of unidecode-0.2-1.5502ada package collides with
> > > that
> > > of package company-cabal-0.3.0-1.62112a7
> > > snippets directory of minitest-0.8.0-1.1aadb78 package collides
> > > with
> > > that of package elpy-1.35.0
> > > test directory of realgud-1.5.1 package collides with that of
> > > package
> > > flycheck-haskell-0.8-2.32ddff8
> > > snippets directory of yasnippet-snippets-0.23 package collides
> > > with
> > > that of package elpy-1.35.0
> > > snippets directory of haskell-snippets-0.1.0-0.07b0f46 package
> > > collides with that of package elpy-1.35.0
> > > snippets directory of rspec-1.11-1.66ea7cc package collides with
> > > that
> > > of package elpy-1.35.0
> > > doc directory of modus-themes-1.0.2 package collides with that of
> > > package racket-mode-0.0.2-6.5eb31a2
> > > data directory of emojify-1.2 package collides with that of
> > > package
> > > unidecode-0.2-1.5502ada
> > > test directory of systemd-mode-1.6 package collides with that of
> > > package flycheck-haskell-0.8-2.32ddff8
> > > lib directory of slime-2.26.1 package collides with that of
> > > package
> > > robe-0.8.2
> > > contrib directory of sly-1.0.0-7.68561f1 package collides with
> > > that
> > > of package slime-2.26.1
> > > lib directory of sly-1.0.0-7.68561f1 package collides with that
> > > of
> > > package robe-0.8.2
> > > doc directory of modus-vivendi-theme-1.0.2 package collides with
> > > that
> > > of package racket-mode-0.0.2-6.5eb31a2
> > > doc directory of evil-1.14.0 package collides with that of
> > > package
> > > racket-mode-0.0.2-6.5eb31a2
> > > data directory of all-the-icons-4.0.1 package collides with that
> > > of
> > > package unidecode-0.2-1.5502ada
> > > snippets directory of feature-mode-20190801-1.11ae167 package
> > > collides with that of package elpy-1.35.0
> > > --8<---------------cut here---------------end--------------->8---
> > > 
> > > So 17 Emacs packages in Guix currently conflict, and Guix seems
> > > to be
> > > silent about it when building a profile with them via 'guix
> > > package
> > > -p'
> > > (a bug?).
> > It's a bug in those packages; not in Guix.
> 
> I used to think the same, but after reading the Elisp reference
> manual
> mentioning how Emacs packages to be installed with package.el are
> expected to live in their own distinct directory with their own
> resources; we can't really say that's it's a bug in the packages:
> they
> just weren't designed to be merged in a flat directory with other
> Elisp
> packages.  Users installing these packages manually can simply add
> their
> top level directory to their load path, else use package.el to
> install
> them.
Idk, feels like a mixed bag to me.  Especially yasnippet will actually
be broken by going to this new layout.  The user will be expected to
have $GUIX_PROFILE/share/emacs/site-lisp/yasnippet-snippets-
$VERSION/snippets in their yas-snippets-dir.  My really old setup
assumes, they'd be stored in $GUIX_PROFILE/share/emacs/yasnippet-
snippets/ instead, because I forgot it moved to site-lisp/snippets. 
Now what do you think happens on version upgrades?

I think, this is another argument to separate code and data even if we
do go the route of using subdirectories to store code.

> > I think I've already laid that out above, but we really ought to
> > have
> > site-lisp/$PACKAGE-$VERSION and a working subdirs.el.  If we do it
> > like
> > that, I don't think the multi-directory layout should cause us too
> > many
> > troubles.
> > 
> > As far as the build system is concerned, we would probably need to
> > set
> > up an environment similar to what will be found in the end.
> > Imagine this:
> > 
> > - environment-variables
> > - $PACKAGE-$VERSION/
> > - $PACKAGE-$VERSION-inputs/
> >   - subdirs.el
> >   - $INPUT1/
> >   - $INPUT2/
> 
> In the build environment, there is no profile so each package add
> their
> individual site-lisp/ to the load path (EMACSLOADPATH).  With my
> proposed idea to add subdirs.el to Emacs itself, there'd be nothing
> to
> do here.
Yes there would.  Exactly because there is no union-build, those
packages would not not have subdirs.el, so it's dubious as to whether
they'd be properly expanded.

> Thanks again for this discussion; I'll work on a revised
> patch.  Until
> then, happy new year!
Happy new year to you too and happy hacking 🙂

Leo





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
@ 2021-03-29  2:02 Mark H Weaver
  2021-03-29  8:07 ` Leo Prikler
                   ` (2 more replies)
  0 siblings, 3 replies; 35+ messages in thread
From: Mark H Weaver @ 2021-03-29  2:02 UTC (permalink / raw)
  To: 47458

I just updated my Guix system, which included the Emacs update from 27.1
to 27.2.  After "guix package -m mhw-manifest.scm" finished running
(which takes a long time for me, since I don't use substitutes), and
before I even noticed that it had finished, my existing Emacs session
started misbehaving badly.

It failed to even open a plain text file in fundamental mode (a .drv
file) with an inscrutible error about 'arrayp'.  I tried to enable the
debugger with M-x toggle-debug-on-error, but then I started getting
errors about 'debug' not found.  (I neglected to record the exact
messages).

I tried running "emacs -Q", and the same errors happened there too.  I
tried running "emacs -Q" from my root shell on a Linux text terminal,
and the same errors happened there.

I resorted to using 'vi' (which thankfully I had, and still remember how
to use for basic editing) to revert the emacs update on my private
branch and to rebuild my user profiles.

Eventually, I realized what the problem was:

(1) My existing emacs session started failing because
    ~/.guix-profile/share/emacs/27.1 had disappeared out from under it.

(2) My newly launched emacs sessions were failing because my
    EMACSLOADPATH variable was still set to its old value, pointing at
    /home/mhw/.guix-profile/share/emacs/27.1/lisp, which no longer
    existed.

I'm not sure why I've never run into this problem before.  I'm also not
sure what can be done to make this better, but if anyone has ideas, that
would be good.  If a 7+ year Guix veteran developer gets bitten badly by
this, I doubt that less experienced users will be impressed.

Any ideas?

       Mark




^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
  2021-03-29  2:02 Mark H Weaver
@ 2021-03-29  8:07 ` Leo Prikler
  2021-03-29  8:24   ` Maxime Devos
  2021-03-29 15:55 ` [bug#45359] " Ludovic Courtès
  2021-03-29 15:55 ` Ludovic Courtès
  2 siblings, 1 reply; 35+ messages in thread
From: Leo Prikler @ 2021-03-29  8:07 UTC (permalink / raw)
  To: Mark H Weaver, 47458

Am Sonntag, den 28.03.2021, 22:02 -0400 schrieb Mark H Weaver:
> I just updated my Guix system, which included the Emacs update from
> 27.1
> to 27.2.  After "guix package -m mhw-manifest.scm" finished running
> (which takes a long time for me, since I don't use substitutes), and
> before I even noticed that it had finished, my existing Emacs session
> started misbehaving badly.
> 
> It failed to even open a plain text file in fundamental mode (a .drv
> file) with an inscrutible error about 'arrayp'.  I tried to enable
> the
> debugger with M-x toggle-debug-on-error, but then I started getting
> errors about 'debug' not found.  (I neglected to record the exact
> messages).
As you are probably aware by now, this is a result of parts of your
EMACSLOADPATH being deleted.  I don't think there's a good solution to
this, but you could (as part of your early init file) resolve the
symlinks in it, so that it behaves deterministically until it is
garbage collected?

> [...]
> 
> Eventually, I realized what the problem was:
> 
> (1) My existing emacs session started failing because
>     ~/.guix-profile/share/emacs/27.1 had disappeared out from under
> it.
> 
> (2) My newly launched emacs sessions were failing because my
>     EMACSLOADPATH variable was still set to its old value, pointing
> at
>     /home/mhw/.guix-profile/share/emacs/27.1/lisp, which no longer
>     existed.
> 
> I'm not sure why I've never run into this problem before.  I'm also
> not
> sure what can be done to make this better, but if anyone has ideas,
> that
> would be good.  If a 7+ year Guix veteran developer gets bitten badly
> by
> this, I doubt that less experienced users will be impressed.
Remember the snippet, that tells you you might want to recompute your
environment variables at the end of `guix package'?  Well, this is just
that.  I've already made it a habit to close Emacs at that point (and
you probably have as well), but as you said, you didn't even notice the
update succeed, so that's what lead to the confusion.

In a similar manner, if I see an Emacs version upgrade at the start of
the transaction, I already know to prepare for a little environment
variable dance to get it to start correctly.  I think there has been an
idea to update environment variables in GNOME Shell directly, for
instance, but
a. we're lacking the technology to do so at the moment (e.g. guile-
dbus)
b. it's not clear, that Guix itself should do such a thing rather than
relying on the user to e.g. code up a oneshot shepherd service 

Regards,
Leo





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
  2021-03-29  8:07 ` Leo Prikler
@ 2021-03-29  8:24   ` Maxime Devos
  2021-03-29  8:43     ` Leo Prikler
  0 siblings, 1 reply; 35+ messages in thread
From: Maxime Devos @ 2021-03-29  8:24 UTC (permalink / raw)
  To: Leo Prikler, Mark H Weaver, 47458

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

On Mon, 2021-03-29 at 10:07 +0200, Leo Prikler wrote:
> [...]
> 
> In a similar manner, if I see an Emacs version upgrade at the start of
> the transaction, I already know to prepare for a little environment
> variable dance to get it to start correctly.  I think there has been an
> idea to update environment variables in GNOME Shell directly, for
> instance, but
> a. we're lacking the technology to do so at the moment (e.g. guile-
> dbus)

Actually, we do have dbus bindings for guile (actually, a reimplementation
of dbus in (Guile) Scheme): guile-ac-d-bus.  It doesn't support file descriptor
passing (at least on Guile, other Schemes may differ) though, but that's
probably not required for this.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
  2021-03-29  8:24   ` Maxime Devos
@ 2021-03-29  8:43     ` Leo Prikler
  0 siblings, 0 replies; 35+ messages in thread
From: Leo Prikler @ 2021-03-29  8:43 UTC (permalink / raw)
  To: Maxime Devos, Mark H Weaver, 47458

Am Montag, den 29.03.2021, 10:24 +0200 schrieb Maxime Devos:
> On Mon, 2021-03-29 at 10:07 +0200, Leo Prikler wrote:
> > [...]
> > 
> > In a similar manner, if I see an Emacs version upgrade at the start
> > of
> > the transaction, I already know to prepare for a little environment
> > variable dance to get it to start correctly.  I think there has
> > been an
> > idea to update environment variables in GNOME Shell directly, for
> > instance, but
> > a. we're lacking the technology to do so at the moment (e.g. guile-
> > dbus)
> 
> Actually, we do have dbus bindings for guile (actually, a
> reimplementation
> of dbus in (Guile) Scheme): guile-ac-d-bus.  It doesn't support file
> descriptor
> passing (at least on Guile, other Schemes may differ) though, but
> that's
> probably not required for this.
Thanks for pointing this out!  Now I can finally write Guile code to
update all those environment variables.  (Hopefully this lets us do
polkit in Guix as well.)





^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#45359] bug#47458: Terrible UX upgrading Emacs in Guix
  2021-03-29  2:02 Mark H Weaver
  2021-03-29  8:07 ` Leo Prikler
@ 2021-03-29 15:55 ` Ludovic Courtès
  2021-03-29 15:55 ` Ludovic Courtès
  2 siblings, 0 replies; 35+ messages in thread
From: Ludovic Courtès @ 2021-03-29 15:55 UTC (permalink / raw)
  To: Mark H Weaver; +Cc: 47458, Maxim Cournoyer, 45359

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

> Eventually, I realized what the problem was:
>
> (1) My existing emacs session started failing because
>     ~/.guix-profile/share/emacs/27.1 had disappeared out from under it.
>
> (2) My newly launched emacs sessions were failing because my
>     EMACSLOADPATH variable was still set to its old value, pointing at
>     /home/mhw/.guix-profile/share/emacs/27.1/lisp, which no longer
>     existed.
>
> I'm not sure why I've never run into this problem before.  I'm also not
> sure what can be done to make this better, but if anyone has ideas, that
> would be good.  If a 7+ year Guix veteran developer gets bitten badly by
> this, I doubt that less experienced users will be impressed.

Ouch.  “It used to be” (speaking like a veteran :-)) that Emacs in Guix
would not use EMACSLOADPATH.  Then we switched to EMACSLOADPATH, which
had some advantages, but necessarily has this drawback.

IIUC, <https://issues.guix.gnu.org/45359> is about possibly
backtracking.  Maxim, what’s the status of this one?

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#45359] bug#47458: Terrible UX upgrading Emacs in Guix
  2021-03-29  2:02 Mark H Weaver
  2021-03-29  8:07 ` Leo Prikler
  2021-03-29 15:55 ` [bug#45359] " Ludovic Courtès
@ 2021-03-29 15:55 ` Ludovic Courtès
  2021-03-29 18:25   ` bug#45359: " Maxim Cournoyer
  2 siblings, 1 reply; 35+ messages in thread
From: Ludovic Courtès @ 2021-03-29 15:55 UTC (permalink / raw)
  To: Mark H Weaver; +Cc: 47458, Maxim Cournoyer, 45359

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

> Eventually, I realized what the problem was:
>
> (1) My existing emacs session started failing because
>     ~/.guix-profile/share/emacs/27.1 had disappeared out from under it.
>
> (2) My newly launched emacs sessions were failing because my
>     EMACSLOADPATH variable was still set to its old value, pointing at
>     /home/mhw/.guix-profile/share/emacs/27.1/lisp, which no longer
>     existed.
>
> I'm not sure why I've never run into this problem before.  I'm also not
> sure what can be done to make this better, but if anyone has ideas, that
> would be good.  If a 7+ year Guix veteran developer gets bitten badly by
> this, I doubt that less experienced users will be impressed.

Ouch.  “It used to be” (speaking like a veteran :-)) that Emacs in Guix
would not use EMACSLOADPATH.  Then we switched to EMACSLOADPATH, which
had some advantages, but necessarily has this drawback.

IIUC, <https://issues.guix.gnu.org/45359> is about possibly
backtracking.  Maxim, what’s the status of this one?

Thanks,
Ludo’.




^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45359: bug#47458: Terrible UX upgrading Emacs in Guix
  2021-03-29 15:55 ` Ludovic Courtès
@ 2021-03-29 18:25   ` Maxim Cournoyer
  2020-12-22  3:28     ` [bug#45359] [PATCH]: Re-introduce Emacs packages specific installation prefix Maxim Cournoyer
  2021-03-30  8:04     ` [bug#45359] bug#47458: Terrible UX upgrading Emacs in Guix Ludovic Courtès
  0 siblings, 2 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2021-03-29 18:25 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: Mark H Weaver, 45359-done, 47458

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Mark,
>
> Mark H Weaver <mhw@netris.org> skribis:
>
>> Eventually, I realized what the problem was:
>>
>> (1) My existing emacs session started failing because
>>     ~/.guix-profile/share/emacs/27.1 had disappeared out from under it.
>>
>> (2) My newly launched emacs sessions were failing because my
>>     EMACSLOADPATH variable was still set to its old value, pointing at
>>     /home/mhw/.guix-profile/share/emacs/27.1/lisp, which no longer
>>     existed.
>>
>> I'm not sure why I've never run into this problem before.  I'm also not
>> sure what can be done to make this better, but if anyone has ideas, that
>> would be good.  If a 7+ year Guix veteran developer gets bitten badly by
>> this, I doubt that less experienced users will be impressed.
>
> Ouch.  “It used to be” (speaking like a veteran :-)) that Emacs in Guix
> would not use EMACSLOADPATH.  Then we switched to EMACSLOADPATH, which
> had some advantages, but necessarily has this drawback.
>
> IIUC, <https://issues.guix.gnu.org/45359> is about possibly
> backtracking.  Maxim, what’s the status of this one?

It's abandoned, The MUMI tracker lacks the responses from Leo Prickler,
but they had good arguments maintaining the status quo rather than going
with the extra complexity.  It also wouldn't change the issue at hand;
it'd merely prevent conflicts of *resources* files of Emacs packages
(and somewhat integrate with the Emacs native package manager, while
making the autoloading a bit slower).  It seems the price to pay is too
high for such a small gain.  I'm closing it now.

On the other hand, this very problem was the motivation for this patch
series here: https://issues.guix.gnu.org/43627, which would solve the
issue ta hand.  You were skeptical of the benefits the last time you
took a look at it; perhaps it's time to take a new look at it :-).

Thanks,

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#45359] closed (Re: bug#47458: Terrible UX upgrading Emacs in Guix)
       [not found]       ` <handler.45359.D45359.161704236132600.notifdone@debbugs.gnu.org>
@ 2021-03-29 18:45         ` Maxim Cournoyer
  2021-03-29 18:48         ` bug#45359: [PATCH]: Re-introduce Emacs packages specific installation prefix Maxim Cournoyer
  1 sibling, 0 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2021-03-29 18:45 UTC (permalink / raw)
  To: GNU Debbugs; +Cc: 45359

reopen 45359
thanks

Hi,

It seems I've closed that report by mistake.  Sorry!

Reopening.

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#45359: [PATCH]: Re-introduce Emacs packages specific installation prefix.
       [not found]       ` <handler.45359.D45359.161704236132600.notifdone@debbugs.gnu.org>
  2021-03-29 18:45         ` [bug#45359] closed (Re: bug#47458: Terrible UX upgrading Emacs in Guix) Maxim Cournoyer
@ 2021-03-29 18:48         ` Maxim Cournoyer
  1 sibling, 0 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2021-03-29 18:48 UTC (permalink / raw)
  Cc: 45359-done

Hi,

help-debbugs@gnu.org (GNU bug Tracking System) writes:

> Your bug report
>
> #45359: [PATCH]: Re-introduce Emacs packages specific installation prefix.
>
> which was filed against the guix-patches package, has been closed.
>
> The explanation is attached below, along with your original report.
> If you require more details, please reply to 45359@debbugs.gnu.org.

Seems the reply title got me confused; this *is* the issue I want to
close.  Doing so again, with the title fixed this time.

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#45359] bug#47458: Terrible UX upgrading Emacs in Guix
  2021-03-29 18:25   ` bug#45359: " Maxim Cournoyer
  2020-12-22  3:28     ` [bug#45359] [PATCH]: Re-introduce Emacs packages specific installation prefix Maxim Cournoyer
@ 2021-03-30  8:04     ` Ludovic Courtès
  1 sibling, 0 replies; 35+ messages in thread
From: Ludovic Courtès @ 2021-03-30  8:04 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: Mark H Weaver, 45359-done, 47458

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> Ouch.  “It used to be” (speaking like a veteran :-)) that Emacs in Guix
>> would not use EMACSLOADPATH.  Then we switched to EMACSLOADPATH, which
>> had some advantages, but necessarily has this drawback.
>>
>> IIUC, <https://issues.guix.gnu.org/45359> is about possibly
>> backtracking.  Maxim, what’s the status of this one?
>
> It's abandoned, The MUMI tracker lacks the responses from Leo Prickler,
> but they had good arguments maintaining the status quo rather than going
> with the extra complexity.  It also wouldn't change the issue at hand;
> it'd merely prevent conflicts of *resources* files of Emacs packages
> (and somewhat integrate with the Emacs native package manager, while
> making the autoloading a bit slower).  It seems the price to pay is too
> high for such a small gain.  I'm closing it now.

I see, makes sense!

> On the other hand, this very problem was the motivation for this patch
> series here: https://issues.guix.gnu.org/43627, which would solve the
> issue ta hand.  You were skeptical of the benefits the last time you
> took a look at it; perhaps it's time to take a new look at it :-).

Ah!  Now I may have to revisit it, indeed.

Thanks for explaining!

Ludo’.




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? field to the <search-path-specification> record.
  2020-09-26  6:11 [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Maxim Cournoyer
  2020-09-26  6:14 ` [bug#43627] [PATCH core-updates 1/3] guix: Add an append-separator? " Maxim Cournoyer
  2020-09-27 19:01 ` [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Ludovic Courtès
@ 2021-03-30 15:51 ` Maxim Cournoyer
  2021-03-30 15:51   ` [bug#43627] [PATCH core-updates v2 2/2] gnu: emacs: Use the new append-separator? option to define its search path Maxim Cournoyer
  2021-04-10 20:42   ` [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? field to the <search-path-specification> record Ludovic Courtès
  2021-03-30 18:41 ` [bug#43627] [PATCH] gnu: emacs: Wrap EMACSLOADPATH Leo Prikler
  3 siblings, 2 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2021-03-30 15:51 UTC (permalink / raw)
  To: 43627; +Cc: ludo, Maxim Cournoyer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 13879 bytes --]

This new field allows to specify in a search-path-specification that a
trailing separator should be added to the computed value of the environment
variable.  A trailing separator sometimes has the meaning that the usual
builtin locations should be looked up as well as the ones explicitly
specified.

One use case is to specify the Emacs library paths using EMACSLOADPATH.  This
allows to not embed the Emacs version in its search path specification, which
has been shown to cause issues when upgrading a profile or when defining
variant Emacs packages of different versions.

* guix/search-paths.scm (searh-path-specification): Add an APPEND-SEPARATOR?
field.
(search-path-specification->sexp): Adjust accordingly.
(sexp->search-path-specification): Likewise.
(evaluate-search-paths): Append a separator to the search path value when both
`separator' and `append-separator?' are #t.  Document the new behavior.
* guix/scripts/environment.scm (create-environment): Adjust the logic used to
merge search-path values when creating an environment profile.
* guix/build/gnu-build-system.scm (set-paths): Adjust accordingly.
---
 guix/build/gnu-build-system.scm | 12 ++++----
 guix/build/profiles.scm         | 13 ++++++---
 guix/build/utils.scm            | 12 ++++++--
 guix/scripts/environment.scm    | 11 ++++++--
 guix/search-paths.scm           | 49 +++++++++++++++++++++++----------
 5 files changed, 69 insertions(+), 28 deletions(-)

diff --git a/guix/build/gnu-build-system.scm b/guix/build/gnu-build-system.scm
index af64b3b61f..1e4d8fecb4 100644
--- a/guix/build/gnu-build-system.scm
+++ b/guix/build/gnu-build-system.scm
@@ -2,7 +2,7 @@
 ;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2018 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2020 Brendan Tildesley <mail@brendan.scot>
-;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2020, 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -100,23 +100,25 @@ there are none."
                                              input-directories)))
 
   (for-each (match-lambda
-             ((env-var (files ...) separator type pattern)
+             ((env-var (files ...) separator type pattern append-sep)
               (set-path-environment-variable env-var files
                                              input-directories
                                              #:separator separator
                                              #:type type
-                                             #:pattern pattern)))
+                                             #:pattern pattern
+                                             #:append-separator? append-sep)))
             search-paths)
 
   (when native-search-paths
     ;; Search paths for native inputs, when cross building.
     (for-each (match-lambda
-               ((env-var (files ...) separator type pattern)
+               ((env-var (files ...) separator type pattern append-sep)
                 (set-path-environment-variable env-var files
                                                native-input-directories
                                                #:separator separator
                                                #:type type
-                                               #:pattern pattern)))
+                                               #:pattern pattern
+                                               #:append-separator? append-sep)))
               native-search-paths)))
 
 (define* (install-locale #:key
diff --git a/guix/build/profiles.scm b/guix/build/profiles.scm
index a40c3f96de..83a4c4dd94 100644
--- a/guix/build/profiles.scm
+++ b/guix/build/profiles.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -20,6 +21,7 @@
   #:use-module (guix build union)
   #:use-module (guix build utils)
   #:use-module (guix search-paths)
+  #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (ice-9 ftw)
   #:use-module (ice-9 match)
@@ -51,10 +53,13 @@ user-friendly name of the profile is, for instance ~/.guix-profile rather than
          ((? string? separator)
           (let ((items (string-tokenize* value separator)))
             (cons search-path
-                  (string-join (map (lambda (str)
-                                      (string-append replacement (crop str)))
-                                    items)
-                               separator)))))))))
+                  (string-join
+                   (map (lambda (str)
+                          (string-append replacement (crop str)))
+                        ;; When APPEND-SEPARATOR? is #t, the trailing
+                        ;; separator causes an empty string item.  Remove it.
+                        (remove string-null? items))
+                   separator)))))))))
 
 (define (write-environment-variable-definition port)
   "Write the given environment variable definition to PORT."
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 6c37021673..354be2e6e3 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -573,7 +573,8 @@ for under the directories designated by FILES.  For example:
                                         #:key
                                         (separator ":")
                                         (type 'directory)
-                                        pattern)
+                                        pattern
+                                        (append-separator? #f))
   "Look for each of FILES of the given TYPE (a symbol as returned by
 'stat:type') in INPUT-DIRS.  Set ENV-VAR to a SEPARATOR-separated path
 accordingly.  Example:
@@ -590,11 +591,16 @@ denoting file names to look for under the directories designated by FILES:
                                  (list docbook-xml docbook-xsl)
                                  #:type 'regular
                                  #:pattern \"^catalog\\\\.xml$\")
-"
+
+When both SEPARATOR and APPEND-SEPARATOR? are true, a separator is appended to
+the value of the environment variable."
   (let* ((path  (search-path-as-list files input-dirs
                                      #:type type
                                      #:pattern pattern))
-         (value (list->search-path-as-string path separator)))
+         (value (list->search-path-as-string path separator))
+         (value (if append-separator?
+                          (string-append value separator)
+                          value)))
     (if (string-null? value)
         (begin
           ;; Never set ENV-VAR to an empty string because often, the empty
diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index 0360761683..c0e081a4bc 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2014, 2015, 2018 David Thompson <davet@gnu.org>
 ;;; Copyright © 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2018 Mike Gerwitz <mtg@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -81,12 +82,18 @@ variables with additional search paths."
   (when pure?
     (purify-environment white-list))
   (for-each (match-lambda
-              ((($ <search-path-specification> variable _ separator) . value)
+              ((($ <search-path-specification> variable _ separator _
+                                               append-separator?) . value)
                (let ((current (getenv variable)))
                  (setenv variable
                          (if (and current (not pure?))
                              (if separator
-                                 (string-append value separator current)
+                                 (if append-separator?
+                                     ;; There is already a trailing separator
+                                     ;; at the end of value.
+                                     ;; (see: `evaluate-search-paths').
+                                     (string-append value current separator)
+                                     (string-append value separator current))
                                  value)
                              value)))))
             (profile-search-paths profile manifest))
diff --git a/guix/search-paths.scm b/guix/search-paths.scm
index 002e6342bb..d783a2815f 100644
--- a/guix/search-paths.scm
+++ b/guix/search-paths.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2017, 2018 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -30,6 +31,7 @@
             search-path-specification-separator
             search-path-specification-file-type
             search-path-specification-file-pattern
+            search-path-specification-append-separator?
 
             $PATH
 
@@ -54,14 +56,16 @@
 (define-record-type* <search-path-specification>
   search-path-specification make-search-path-specification
   search-path-specification?
-  (variable     search-path-specification-variable) ;string
-  (files        search-path-specification-files)    ;list of strings
-  (separator    search-path-specification-separator ;string | #f
-                (default ":"))
-  (file-type    search-path-specification-file-type ;symbol
-                (default 'directory))
-  (file-pattern search-path-specification-file-pattern ;#f | string
-                (default #f)))
+  (variable          search-path-specification-variable) ;string
+  (files             search-path-specification-files)    ;list of strings
+  (separator         search-path-specification-separator ;string | #f
+                     (default ":"))
+  (file-type         search-path-specification-file-type ;symbol
+                     (default 'directory))
+  (file-pattern      search-path-specification-file-pattern ;#f | string
+                     (default #f))
+  (append-separator? search-path-specification-append-separator? ;#f | #t
+                     (default #f)))
 
 (define $PATH
   ;; The 'PATH' variable.  This variable is a bit special: it is not attached
@@ -76,20 +80,32 @@ corresponds to the arguments expected by `set-path-environment-variable'."
   ;; Note that this sexp format is used both by build systems and in
   ;; (guix profiles), so think twice before you change it.
   (match spec
-    (($ <search-path-specification> variable files separator type pattern)
-     `(,variable ,files ,separator ,type ,pattern))))
+    (($ <search-path-specification> variable files separator type pattern
+                                    append-separator?)
+     `(,variable ,files ,separator ,type ,pattern ,append-separator?))))
 
 (define (sexp->search-path-specification sexp)
   "Convert SEXP, which is as returned by 'search-path-specification->sexp', to
 a <search-path-specification> object."
   (match sexp
+    ((variable files separator type pattern append-separator?)
+     (search-path-specification
+      (variable variable)
+      (files files)
+      (separator separator)
+      (file-type type)
+      (file-pattern pattern)
+      (append-separator? append-separator?)))
+    ;; Previous search-path-specification form (without append-separator?
+    ;; might still be found in manifest files.
     ((variable files separator type pattern)
      (search-path-specification
       (variable variable)
       (files files)
       (separator separator)
       (file-type type)
-      (file-pattern pattern)))))
+      (file-pattern pattern)
+      (append-separator? #f)))))
 
 (define-syntax-rule (with-null-error-port exp)
   "Evaluate EXP with the error port pointing to the bit bucket."
@@ -131,7 +147,9 @@ like `string-tokenize', but SEPARATOR is a string."
   "Evaluate SEARCH-PATHS, a list of search-path specifications, for
 DIRECTORIES, a list of directory names, and return a list of
 specification/value pairs.  Use GETENV to determine the current settings and
-report only settings not already effective."
+report only settings not already effective.  When the search path
+specification APPEND-SEPARATOR? and SEPARATOR fields are both set to true, a
+separator is appended to its computed value."
   (define (search-path-definition spec)
     (match spec
       (($ <search-path-specification> variable files #f type pattern)
@@ -148,7 +166,7 @@ report only settings not already effective."
                 #f                         ;VARIABLE already set appropriately
                 (cons spec head))))))
       (($ <search-path-specification> variable files separator
-                                      type pattern)
+                                      type pattern append-separator?)
        (let* ((values (or (and=> (getenv variable)
                                  (cut string-tokenize* <> separator))
                           '()))
@@ -161,7 +179,10 @@ report only settings not already effective."
                                             #:pattern pattern))))
          (if (every (cut member <> values) path)
              #f                         ;VARIABLE is already set appropriately
-             (cons spec (string-join path separator)))))))
+             (let ((value (string-join path separator)))
+               (cons spec (if append-separator?
+                              (string-append value separator)
+                              value))))))))
 
   (filter-map search-path-definition search-paths))
 
-- 
2.31.1





^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates v2 2/2] gnu: emacs: Use the new append-separator? option to define its search path.
  2021-03-30 15:51 ` [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? " Maxim Cournoyer
@ 2021-03-30 15:51   ` Maxim Cournoyer
  2021-04-10 20:42   ` [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? field to the <search-path-specification> record Ludovic Courtès
  1 sibling, 0 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2021-03-30 15:51 UTC (permalink / raw)
  To: 43627; +Cc: ludo, Maxim Cournoyer

Fixes <https://issues.guix.info/43277>.

* gnu/packages/emacs.scm (emacs): Remove the versioned lisp file name from the
search path, and set the append-separator? field to #t.
---
 gnu/packages/emacs.scm | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index fe5b3b25b3..148593e355 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -261,9 +261,8 @@
     (native-search-paths
      (list (search-path-specification
             (variable "EMACSLOADPATH")
-            ;; The versioned entry is for the Emacs' builtin libraries.
-            (files (list "share/emacs/site-lisp"
-                         (string-append "share/emacs/" version "/lisp"))))
+            (files (list "share/emacs/site-lisp"))
+            (append-separator? #t))
            (search-path-specification
             (variable "INFOPATH")
             (files '("share/info")))))
-- 
2.31.1





^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH] gnu: emacs: Wrap EMACSLOADPATH.
  2020-09-26  6:11 [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Maxim Cournoyer
                   ` (2 preceding siblings ...)
  2021-03-30 15:51 ` [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? " Maxim Cournoyer
@ 2021-03-30 18:41 ` Leo Prikler
  2021-04-04  4:35   ` bug#47458: Terrible UX upgrading Emacs in Guix Maxim Cournoyer
  3 siblings, 1 reply; 35+ messages in thread
From: Leo Prikler @ 2021-03-30 18:41 UTC (permalink / raw)
  To: 47458; +Cc: 43627, maxim.cournoyer

With this, the search path specification of EMACSLOADPATH does no longer
depend on the version of Emacs, which should make upgrading major versions
less painful.  See also:
- <https://bugs.gnu.org/43627>
- <https://bugs.gnu.org/47458>

* gnu/packages/emacs.scm (emacs)[#:phases]: Add ‘wrap-load-path’.
[native-search-path]<EMACSLOADPATH>: Do not search for builtin libraries.
(emacs-next)[native-search-path]: Inherit from emacs.
---
 gnu/packages/emacs.scm | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index 7447cfe33a..e12c489f8d 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -201,6 +201,20 @@
                 (car (find-files "bin" "^emacs-([0-9]+\\.)+[0-9]+$"))
                 "bin/emacs")
                #t)))
+         (add-after 'strip-double-wrap 'wrap-load-path
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let* ((out (assoc-ref outputs "out"))
+                    (lisp-dirs (find-files (string-append out "/share/emacs")
+                                           "^lisp$"
+                                           #:directories? #t)))
+               (for-each
+                (lambda (prog)
+                  (wrap-program prog
+                    `("EMACSLOADPATH" suffix ,lisp-dirs)))
+                (find-files (string-append out "/bin")
+                            ;; versioned and unversioned emacs binaries
+                            "^emacs(-[0-9]+(\\.[0-9]+)*)?$"))
+               #t)))
          (add-before 'reset-gzip-timestamps 'make-compressed-files-writable
            ;; The 'reset-gzip-timestamps phase will throw a permission error
            ;; if gzip files aren't writable then.  This phase is needed when
@@ -255,9 +269,7 @@
     (native-search-paths
      (list (search-path-specification
             (variable "EMACSLOADPATH")
-            ;; The versioned entry is for the Emacs' builtin libraries.
-            (files (list "share/emacs/site-lisp"
-                         (string-append "share/emacs/" version "/lisp"))))
+            (files '("share/emacs/site-lisp")))
            (search-path-specification
             (variable "INFOPATH")
             (files '("share/info")))))
@@ -294,18 +306,7 @@ languages.")
            "0igjm9kwiswn2dpiy2k9xikbdfc7njs07ry48fqz70anljj8y7y3"))))
       (native-inputs
        `(("autoconf" ,autoconf)
-         ,@(package-native-inputs emacs)))
-      (native-search-paths
-       (list (search-path-specification
-              (variable "EMACSLOADPATH")
-              ;; The versioned entry is for the Emacs' builtin libraries.
-              (files (list "share/emacs/site-lisp"
-                           (string-append "share/emacs/"
-                                          (version-major+minor+point version)
-                                          "/lisp"))))
-             (search-path-specification
-              (variable "INFOPATH")
-              (files '("share/info"))))))))
+         ,@(package-native-inputs emacs))))))
 
 (define-public emacs-next-pgtk
   (let ((commit "ae18c8ec4f0ef37c8c9cda473770ff47e41291e2")
-- 
2.31.0





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
  2021-03-30 18:41 ` [bug#43627] [PATCH] gnu: emacs: Wrap EMACSLOADPATH Leo Prikler
@ 2021-04-04  4:35   ` Maxim Cournoyer
  2021-04-04  7:49     ` Leo Prikler
  0 siblings, 1 reply; 35+ messages in thread
From: Maxim Cournoyer @ 2021-04-04  4:35 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 47458

Hi Leo!

Leo Prikler <leo.prikler@student.tugraz.at> writes:

> With this, the search path specification of EMACSLOADPATH does no longer
> depend on the version of Emacs, which should make upgrading major versions
> less painful.  See also:
> - <https://bugs.gnu.org/43627>
> - <https://bugs.gnu.org/47458>
>
> * gnu/packages/emacs.scm (emacs)[#:phases]: Add ‘wrap-load-path’.
> [native-search-path]<EMACSLOADPATH>: Do not search for builtin libraries.
> (emacs-next)[native-search-path]: Inherit from emacs.
> ---
>  gnu/packages/emacs.scm | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
> index 7447cfe33a..e12c489f8d 100644
> --- a/gnu/packages/emacs.scm
> +++ b/gnu/packages/emacs.scm
> @@ -201,6 +201,20 @@
>                  (car (find-files "bin" "^emacs-([0-9]+\\.)+[0-9]+$"))
>                  "bin/emacs")
>                 #t)))
> +         (add-after 'strip-double-wrap 'wrap-load-path
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out"))
> +                    (lisp-dirs (find-files (string-append out "/share/emacs")
> +                                           "^lisp$"
> +                                           #:directories? #t)))
> +               (for-each
> +                (lambda (prog)
> +                  (wrap-program prog
> +                    `("EMACSLOADPATH" suffix ,lisp-dirs)))
> +                (find-files (string-append out "/bin")
> +                            ;; versioned and unversioned emacs binaries
> +                            "^emacs(-[0-9]+(\\.[0-9]+)*)?$"))
> +               #t)))

Shouldn't we wrap all the binaries to be on the safe side?  Things such
as emacsclient probably ought to have EMACSLOADPATH set correctly, no?

>           (add-before 'reset-gzip-timestamps 'make-compressed-files-writable
>             ;; The 'reset-gzip-timestamps phase will throw a permission error
>             ;; if gzip files aren't writable then.  This phase is needed when
> @@ -255,9 +269,7 @@
>      (native-search-paths
>       (list (search-path-specification
>              (variable "EMACSLOADPATH")
> -            ;; The versioned entry is for the Emacs' builtin libraries.
> -            (files (list "share/emacs/site-lisp"
> -                         (string-append "share/emacs/" version "/lisp"))))
> +            (files '("share/emacs/site-lisp")))
>             (search-path-specification
>              (variable "INFOPATH")
>              (files '("share/info")))))
> @@ -294,18 +306,7 @@ languages.")
>             "0igjm9kwiswn2dpiy2k9xikbdfc7njs07ry48fqz70anljj8y7y3"))))
>        (native-inputs
>         `(("autoconf" ,autoconf)
> -         ,@(package-native-inputs emacs)))
> -      (native-search-paths
> -       (list (search-path-specification
> -              (variable "EMACSLOADPATH")
> -              ;; The versioned entry is for the Emacs' builtin libraries.
> -              (files (list "share/emacs/site-lisp"
> -                           (string-append "share/emacs/"
> -                                          (version-major+minor+point version)
> -                                          "/lisp"))))
> -             (search-path-specification
> -              (variable "INFOPATH")
> -              (files '("share/info"))))))))
> +         ,@(package-native-inputs emacs))))))
>  
>  (define-public emacs-next-pgtk
>    (let ((commit "ae18c8ec4f0ef37c8c9cda473770ff47e41291e2")

This makes sense, and can make it to master rather than core-updates,
which is neat.

Thank you :-)

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
  2021-04-04  4:35   ` bug#47458: Terrible UX upgrading Emacs in Guix Maxim Cournoyer
@ 2021-04-04  7:49     ` Leo Prikler
  2021-04-06 12:09       ` Maxim Cournoyer
  0 siblings, 1 reply; 35+ messages in thread
From: Leo Prikler @ 2021-04-04  7:49 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 47458

Hi Maxim!

Am Sonntag, den 04.04.2021, 00:35 -0400 schrieb Maxim Cournoyer:
> Hi Leo!
> 
> Leo Prikler <leo.prikler@student.tugraz.at> writes:
> 
> > With this, the search path specification of EMACSLOADPATH does no
> > longer
> > depend on the version of Emacs, which should make upgrading major
> > versions
> > less painful.  See also:
> > - <https://bugs.gnu.org/43627>
> > - <https://bugs.gnu.org/47458>
> > 
> > * gnu/packages/emacs.scm (emacs)[#:phases]: Add ‘wrap-load-path’.
> > [native-search-path]<EMACSLOADPATH>: Do not search for builtin
> > libraries.
> > (emacs-next)[native-search-path]: Inherit from emacs.
> > ---
> >  gnu/packages/emacs.scm | 31 ++++++++++++++++---------------
> >  1 file changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
> > index 7447cfe33a..e12c489f8d 100644
> > --- a/gnu/packages/emacs.scm
> > +++ b/gnu/packages/emacs.scm
> > @@ -201,6 +201,20 @@
> >                  (car (find-files "bin" "^emacs-([0-9]+\\.)+[0-
> > 9]+$"))
> >                  "bin/emacs")
> >                 #t)))
> > +         (add-after 'strip-double-wrap 'wrap-load-path
> > +           (lambda* (#:key outputs #:allow-other-keys)
> > +             (let* ((out (assoc-ref outputs "out"))
> > +                    (lisp-dirs (find-files (string-append out
> > "/share/emacs")
> > +                                           "^lisp$"
> > +                                           #:directories? #t)))
> > +               (for-each
> > +                (lambda (prog)
> > +                  (wrap-program prog
> > +                    `("EMACSLOADPATH" suffix ,lisp-dirs)))
> > +                (find-files (string-append out "/bin")
> > +                            ;; versioned and unversioned emacs
> > binaries
> > +                            "^emacs(-[0-9]+(\\.[0-9]+)*)?$"))
> > +               #t)))
> 
> Shouldn't we wrap all the binaries to be on the safe side?  Things
> such
> as emacsclient probably ought to have EMACSLOADPATH set correctly,
> no?
The remaining binaries are
- emacsclient, which inherits its EMACSLOADPATH from the server it
connects to
- ctags, ebrowse and etags, which are helper binaries, that don't seem
to rely on EMACSLOADPATH at all.  (Or is there an indicator, that they
do?)
- .-real binaries, that should only be wrapped once.
We could relax the regex to include the upper two, but I don't think
it's necessary to do so.

> >           (add-before 'reset-gzip-timestamps 'make-compressed-
> > files-writable
> >             ;; The 'reset-gzip-timestamps phase will throw a
> > permission error
> >             ;; if gzip files aren't writable then.  This phase is
> > needed when
> > @@ -255,9 +269,7 @@
> >      (native-search-paths
> >       (list (search-path-specification
> >              (variable "EMACSLOADPATH")
> > -            ;; The versioned entry is for the Emacs' builtin
> > libraries.
> > -            (files (list "share/emacs/site-lisp"
> > -                         (string-append "share/emacs/" version
> > "/lisp"))))
> > +            (files '("share/emacs/site-lisp")))
> >             (search-path-specification
> >              (variable "INFOPATH")
> >              (files '("share/info")))))
> > @@ -294,18 +306,7 @@ languages.")
> >             "0igjm9kwiswn2dpiy2k9xikbdfc7njs07ry48fqz70anljj8y7y3")
> > )))
> >        (native-inputs
> >         `(("autoconf" ,autoconf)
> > -         ,@(package-native-inputs emacs)))
> > -      (native-search-paths
> > -       (list (search-path-specification
> > -              (variable "EMACSLOADPATH")
> > -              ;; The versioned entry is for the Emacs' builtin
> > libraries.
> > -              (files (list "share/emacs/site-lisp"
> > -                           (string-append "share/emacs/"
> > -                                          (version-
> > major+minor+point version)
> > -                                          "/lisp"))))
> > -             (search-path-specification
> > -              (variable "INFOPATH")
> > -              (files '("share/info"))))))))
> > +         ,@(package-native-inputs emacs))))))
> >  
> >  (define-public emacs-next-pgtk
> >    (let ((commit "ae18c8ec4f0ef37c8c9cda473770ff47e41291e2")
> 
> This makes sense, and can make it to master rather than core-updates,
> which is neat.
I'd like to avoid pushing this to master just yet, because we also have
changes in the Emacs build system to discuss and I don't want to cause
an "Emacs world" rebuild twice in a row.  That said, I'm including this
patch in wip-emacs with the plan to push to master or staging once
everything there is resolved.

Regards,
Leo





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
  2021-04-04  7:49     ` Leo Prikler
@ 2021-04-06 12:09       ` Maxim Cournoyer
  2021-04-06 15:49         ` Leo Prikler
  0 siblings, 1 reply; 35+ messages in thread
From: Maxim Cournoyer @ 2021-04-06 12:09 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 47458

Hi Leo!

Leo Prikler <leo.prikler@student.tugraz.at> writes:

[...]

>> Shouldn't we wrap all the binaries to be on the safe side?  Things
>> such
>> as emacsclient probably ought to have EMACSLOADPATH set correctly,
>> no?
> The remaining binaries are
> - emacsclient, which inherits its EMACSLOADPATH from the server it
> connects to
> - ctags, ebrowse and etags, which are helper binaries, that don't seem
> to rely on EMACSLOADPATH at all.  (Or is there an indicator, that they
> do?)
> - .-real binaries, that should only be wrapped once.
> We could relax the regex to include the upper two, but I don't think
> it's necessary to do so.

OK, thanks for the explanation, it makes sense.

[...]

> I'd like to avoid pushing this to master just yet, because we also have
> changes in the Emacs build system to discuss and I don't want to cause
> an "Emacs world" rebuild twice in a row.  That said, I'm including this
> patch in wip-emacs with the plan to push to master or staging once
> everything there is resolved.

Sure!  Which changes do you have in mind?  Are they already on the
tracker for review?

Thank you,

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
  2021-04-06 12:09       ` Maxim Cournoyer
@ 2021-04-06 15:49         ` Leo Prikler
  2021-04-07 19:46           ` Maxim Cournoyer
  0 siblings, 1 reply; 35+ messages in thread
From: Leo Prikler @ 2021-04-06 15:49 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 47458

Hi Maxim!

Am Dienstag, den 06.04.2021, 08:09 -0400 schrieb Maxim Cournoyer:
> Hi Leo!
> 
> Leo Prikler <leo.prikler@student.tugraz.at> writes:
> 
> [...]
> 
> > > Shouldn't we wrap all the binaries to be on the safe
> > > side?  Things
> > > such
> > > as emacsclient probably ought to have EMACSLOADPATH set
> > > correctly,
> > > no?
> > The remaining binaries are
> > - emacsclient, which inherits its EMACSLOADPATH from the server it
> > connects to
> > - ctags, ebrowse and etags, which are helper binaries, that don't
> > seem
> > to rely on EMACSLOADPATH at all.  (Or is there an indicator, that
> > they
> > do?)
> > - .-real binaries, that should only be wrapped once.
> > We could relax the regex to include the upper two, but I don't
> > think
> > it's necessary to do so.
> 
> OK, thanks for the explanation, it makes sense.
> 
Should I also document that (as a comment in the code) or is it
somewhat intuitive, that only Emacs is interested in these variables
(just EMACSLOADPATH currently, maybe also PATH later)?
> 
> > I'd like to avoid pushing this to master just yet, because we also
> > have
> > changes in the Emacs build system to discuss and I don't want to
> > cause
> > an "Emacs world" rebuild twice in a row.  That said, I'm including
> > this
> > patch in wip-emacs with the plan to push to master or staging once
> > everything there is resolved.
> 
> Sure!  Which changes do you have in mind?  Are they already on the
> tracker for review?
> 

I've sent some of the changes for emacs-build-system to 45316.  The
rest only lives on wip-emacs as of yet.  The first 5 patches on that
branch are the ones that include the big changes (well, four of them
anyway), one of which is not yet up to review as it results from the
combined fixes to 45316 and 47458, the rest are mostly small "fixup"
commits.

Regards,
Leo





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#47458: Terrible UX upgrading Emacs in Guix
  2021-04-06 15:49         ` Leo Prikler
@ 2021-04-07 19:46           ` Maxim Cournoyer
  0 siblings, 0 replies; 35+ messages in thread
From: Maxim Cournoyer @ 2021-04-07 19:46 UTC (permalink / raw)
  To: Leo Prikler; +Cc: 47458

Hi Leo!

Leo Prikler <leo.prikler@student.tugraz.at> writes:

> Hi Maxim!
>
> Am Dienstag, den 06.04.2021, 08:09 -0400 schrieb Maxim Cournoyer:
>> Hi Leo!
>>
>> Leo Prikler <leo.prikler@student.tugraz.at> writes:
>>
>> [...]
>>
>> > > Shouldn't we wrap all the binaries to be on the safe
>> > > side?  Things
>> > > such
>> > > as emacsclient probably ought to have EMACSLOADPATH set
>> > > correctly,
>> > > no?
>> > The remaining binaries are
>> > - emacsclient, which inherits its EMACSLOADPATH from the server it
>> > connects to
>> > - ctags, ebrowse and etags, which are helper binaries, that don't
>> > seem
>> > to rely on EMACSLOADPATH at all.  (Or is there an indicator, that
>> > they
>> > do?)
>> > - .-real binaries, that should only be wrapped once.
>> > We could relax the regex to include the upper two, but I don't
>> > think
>> > it's necessary to do so.
>>
>> OK, thanks for the explanation, it makes sense.
>>
> Should I also document that (as a comment in the code) or is it
> somewhat intuitive, that only Emacs is interested in these variables
> (just EMACSLOADPATH currently, maybe also PATH later)?

It wouldn't hurt!  It was not obvious to me that emacsclient didn't need
it (I have no knowledge of emacsclient's internals).

>> > I'd like to avoid pushing this to master just yet, because we also
>> > have
>> > changes in the Emacs build system to discuss and I don't want to
>> > cause
>> > an "Emacs world" rebuild twice in a row.  That said, I'm including
>> > this
>> > patch in wip-emacs with the plan to push to master or staging once
>> > everything there is resolved.
>>
>> Sure!  Which changes do you have in mind?  Are they already on the
>> tracker for review?
>>
>
> I've sent some of the changes for emacs-build-system to 45316.  The
> rest only lives on wip-emacs as of yet.  The first 5 patches on that
> branch are the ones that include the big changes (well, four of them
> anyway), one of which is not yet up to review as it results from the
> combined fixes to 45316 and 47458, the rest are mostly small "fixup"
> commits.

OK.  I'll have a look.

Maxim




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? field to the <search-path-specification> record.
  2021-03-30 15:51 ` [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? " Maxim Cournoyer
  2021-03-30 15:51   ` [bug#43627] [PATCH core-updates v2 2/2] gnu: emacs: Use the new append-separator? option to define its search path Maxim Cournoyer
@ 2021-04-10 20:42   ` Ludovic Courtès
  1 sibling, 0 replies; 35+ messages in thread
From: Ludovic Courtès @ 2021-04-10 20:42 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 43627, Leo Prikler

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> This new field allows to specify in a search-path-specification that a
> trailing separator should be added to the computed value of the environment
> variable.  A trailing separator sometimes has the meaning that the usual
> builtin locations should be looked up as well as the ones explicitly
> specified.
>
> One use case is to specify the Emacs library paths using EMACSLOADPATH.  This
> allows to not embed the Emacs version in its search path specification, which
> has been shown to cause issues when upgrading a profile or when defining
> variant Emacs packages of different versions.

Got it now.  :-)

Leo\x19s patch series seems to be addressing the same issue:

  https://issues.guix.gnu.org/47661

Should we hold on until we\x19ve reviewed and acted upon #47661?  Or does
it have known uses apart from Emacs?

> * guix/search-paths.scm (searh-path-specification): Add an APPEND-SEPARATOR?
> field.
> (search-path-specification->sexp): Adjust accordingly.
> (sexp->search-path-specification): Likewise.
> (evaluate-search-paths): Append a separator to the search path value when both
> `separator' and `append-separator?' are #t.  Document the new behavior.
> * guix/scripts/environment.scm (create-environment): Adjust the logic used to
> merge search-path values when creating an environment profile.
> * guix/build/gnu-build-system.scm (set-paths): Adjust accordingly.

Overall LGTM.  I\x19d suggest maybe replacing \x18append-separator?\x19 by
\x18trailing-separator?\x19, which I find a bit clearer.

Could you add a test or two in tests/search-paths.scm, for the corner
cases?

> -             ((env-var (files ...) separator type pattern)
> +             ((env-var (files ...) separator type pattern append-sep)
>                (set-path-environment-variable env-var files
>                                               input-directories
>                                               #:separator separator
>                                               #:type type
> -                                             #:pattern pattern)))
> +                                             #:pattern pattern
> +                                             #:append-separator? append-sep)))
>              search-paths)
>  
>    (when native-search-paths
>      ;; Search paths for native inputs, when cross building.
>      (for-each (match-lambda
> -               ((env-var (files ...) separator type pattern)
> +               ((env-var (files ...) separator type pattern append-sep)
>                  (set-path-environment-variable env-var files

I\x19d fully spell out the variable name, like \x18append-separator?\x19,
\x18append?\x19, \x18trailing?\x19, as per our coding style.

> +++ b/guix/build/profiles.scm
> @@ -1,5 +1,6 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright © 2015, 2017, 2018, 2019, 2020, 2021 Ludovic Court¨s <ludo@gnu.org>
> +;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -20,6 +21,7 @@
>    #:use-module (guix build union)
>    #:use-module (guix build utils)
>    #:use-module (guix search-paths)
> +  #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-26)
>    #:use-module (ice-9 ftw)
>    #:use-module (ice-9 match)
> @@ -51,10 +53,13 @@ user-friendly name of the profile is, for instance ~/.guix-profile rather than
>           ((? string? separator)
>            (let ((items (string-tokenize* value separator)))
>              (cons search-path
> -                  (string-join (map (lambda (str)
> -                                      (string-append replacement (crop str)))
> -                                    items)
> -                               separator)))))))))
> +                  (string-join
> +                   (map (lambda (str)
> +                          (string-append replacement (crop str)))
> +                        ;; When APPEND-SEPARATOR? is #t, the trailing
> +                        ;; separator causes an empty string item.  Remove it.
> +                        (remove string-null? items))
> +                   separator)))))))))

If we remove the empty string, don\x19t we lose the trailing separator?

> +++ b/guix/build/utils.scm
> @@ -573,7 +573,8 @@ for under the directories designated by FILES.  For example:
>                                          #:key
>                                          (separator ":")
>                                          (type 'directory)
> -                                        pattern)
> +                                        pattern
> +                                        (append-separator? #f))
>    "Look for each of FILES of the given TYPE (a symbol as returned by
>  'stat:type') in INPUT-DIRS.  Set ENV-VAR to a SEPARATOR-separated path
>  accordingly.  Example:
> @@ -590,11 +591,16 @@ denoting file names to look for under the directories designated by FILES:
>                                   (list docbook-xml docbook-xsl)
>                                   #:type 'regular
>                                   #:pattern \"^catalog\\\\.xml$\")
> -"
> +
> +When both SEPARATOR and APPEND-SEPARATOR? are true, a separator is appended to
> +the value of the environment variable."
>    (let* ((path  (search-path-as-list files input-dirs
>                                       #:type type
>                                       #:pattern pattern))
> -         (value (list->search-path-as-string path separator)))
> +         (value (list->search-path-as-string path separator))
> +         (value (if append-separator?
> +                          (string-append value separator)
> +                          value)))

Indentation is off.

That\x19s it.  Thanks and apologies for the delay!

Ludo\x19.




^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2021-04-10 20:43 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-26  6:11 [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Maxim Cournoyer
2020-09-26  6:14 ` [bug#43627] [PATCH core-updates 1/3] guix: Add an append-separator? " Maxim Cournoyer
2020-09-26  6:14   ` [bug#43627] [PATCH core-updates 2/3] gnu: emacs: Use the new append-separator? option to define its search path Maxim Cournoyer
2020-09-26  6:14   ` [bug#43627] [PATCH core-updates 3/3] Revert "emacs-build-system: Ensure the core libraries appear last in the load path." Maxim Cournoyer
2020-09-27 19:01 ` [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record Ludovic Courtès
2020-10-03 21:22   ` Maxim Cournoyer
2020-11-02 13:59     ` Ludovic Courtès
2020-11-08  5:49       ` Maxim Cournoyer
2021-03-30 15:51 ` [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? " Maxim Cournoyer
2021-03-30 15:51   ` [bug#43627] [PATCH core-updates v2 2/2] gnu: emacs: Use the new append-separator? option to define its search path Maxim Cournoyer
2021-04-10 20:42   ` [bug#43627] [PATCH core-updates v2 1/2] guix: Add an append-separator? field to the <search-path-specification> record Ludovic Courtès
2021-03-30 18:41 ` [bug#43627] [PATCH] gnu: emacs: Wrap EMACSLOADPATH Leo Prikler
2021-04-04  4:35   ` bug#47458: Terrible UX upgrading Emacs in Guix Maxim Cournoyer
2021-04-04  7:49     ` Leo Prikler
2021-04-06 12:09       ` Maxim Cournoyer
2021-04-06 15:49         ` Leo Prikler
2021-04-07 19:46           ` Maxim Cournoyer
2021-03-29  2:02 Mark H Weaver
2021-03-29  8:07 ` Leo Prikler
2021-03-29  8:24   ` Maxime Devos
2021-03-29  8:43     ` Leo Prikler
2021-03-29 15:55 ` [bug#45359] " Ludovic Courtès
2021-03-29 15:55 ` Ludovic Courtès
2021-03-29 18:25   ` bug#45359: " Maxim Cournoyer
2020-12-22  3:28     ` [bug#45359] [PATCH]: Re-introduce Emacs packages specific installation prefix Maxim Cournoyer
2020-12-22  8:51       ` bug#45316: " Leo Prikler
2020-12-22 18:09         ` Maxim Cournoyer
2020-12-22 19:10           ` Leo Prikler
2020-12-26  5:01             ` Maxim Cournoyer
2020-12-26 10:56               ` Leo Prikler
2020-12-27  4:44                 ` Maxim Cournoyer
2020-12-27  8:46                   ` Leo Prikler
     [not found]       ` <handler.45359.D45359.161704236132600.notifdone@debbugs.gnu.org>
2021-03-29 18:45         ` [bug#45359] closed (Re: bug#47458: Terrible UX upgrading Emacs in Guix) Maxim Cournoyer
2021-03-29 18:48         ` bug#45359: [PATCH]: Re-introduce Emacs packages specific installation prefix Maxim Cournoyer
2021-03-30  8:04     ` [bug#45359] bug#47458: Terrible UX upgrading Emacs in Guix Ludovic Courtès

all messages for Guix-related lists mirrored at yhetil.org

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/guix

Example config snippet for mirrors.


AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git