unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / 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; 13+ 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] 13+ 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; 13+ 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 related	[flat|nested] 13+ 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; 13+ 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 related	[flat|nested] 13+ 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; 13+ 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 related	[flat|nested] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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 related	[flat|nested] 13+ 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; 13+ 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 related	[flat|nested] 13+ 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
  3 siblings, 0 replies; 13+ 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 related	[flat|nested] 13+ 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
  2021-05-20 14:24     ` bug#43627: " Maxim Cournoyer
  1 sibling, 1 reply; 13+ 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] 13+ messages in thread

* bug#43627: [PATCH core-updates v2 1/2] guix: Add an append-separator? field to the <search-path-specification> record.
  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-05-20 14:24     ` Maxim Cournoyer
  0 siblings, 0 replies; 13+ messages in thread
From: Maxim Cournoyer @ 2021-05-20 14:24 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 43627-done, Leo Prikler

Hi!

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

> 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.s patch series seems to be addressing the same issue:
>
>   https://issues.guix.gnu.org/47661
>
> Should we hold on until we.ve reviewed and acted upon #47661?  Or does
> it have known uses apart from Emacs?

The above as now been merged, obsoleting this series.  Closing!

Thanks,

Maxim




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

end of thread, other threads:[~2021-05-20 14:48 UTC | newest]

Thread overview: 13+ 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-05-20 14:24     ` bug#43627: " Maxim Cournoyer
2021-03-30 18:41 ` [bug#43627] [PATCH] gnu: emacs: Wrap EMACSLOADPATH Leo Prikler

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).