[PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record.

DoneSubmitted by Maxim Cournoyer.
Details
3 participants
  • Leo Prikler
  • Ludovic Courtès
  • Maxim Cournoyer
Owner
unassigned
Severity
normal
M
M
Maxim Cournoyer wrote on 26 Sep 2020 08:11
(name . guix-patches)(address . guix-patches@gnu.org)
87blhtt6d6.fsf@gmail.com
Hello Guix!
These three commits extend our search-path-specification record with anew field, that can be used to produce a trailing separator, which canbe useful at least with Emacs (I can think of at least another placewhere it could be used: the INFOPATH variable).
It was motivated to allow defining the Emacs search path in a morerobust way.
Thanks!
Maxim
M
M
Maxim Cournoyer wrote on 26 Sep 2020 08:14
[PATCH core-updates 2/3] gnu: emacs: Use the new append-separator? option to define its search path.
(address . 43627@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
20200926061414.7898-2-maxim.cournoyer@gmail.com
Fixes https://issues.guix.info/43277.
* gnu/packages/emacs.scm (emacs): Remove the versioned lisp file name from thesearch path, and set the append-separator? field to #t.--- gnu/packages/emacs.scm | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Toggle diff (18 lines)diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scmindex 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
M
M
Maxim Cournoyer wrote on 26 Sep 2020 08:14
[PATCH core-updates 3/3] Revert "emacs-build-system: Ensure the core libraries appear last in the load path."
(address . 43627@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
20200926061414.7898-3-maxim.cournoyer@gmail.com
This reverts commit e34e02707d6bd38c79ce7bec776fcdc528548a0d. This hack is nolonger necessary, now that search path specifications allows specifying that atrailing separator should be appended, which for Emacs stands for the locationof the builtin Elisp libraries.--- guix/build/emacs-build-system.scm | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
Toggle diff (27 lines)diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scmindex 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
L
L
Ludovic Courtès wrote on 27 Sep 2020 21:01
Re: [bug#43627] [PATCH core-updates]: Add a 'append-separator?' field to the <search-path-specification> record.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43627@debbugs.gnu.org)
875z7zf3hb.fsf@gnu.org
Hi,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (8 lines)> 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 weshould weigh the added complexity, both in terms of implementation andof semantic clarity, compared to reduced complexity elsewhere. It seemsto me that the costs outweigh the benefits here.
Also, the empty search path entry has a special meaning. ForEMACSLOADPATH, that entry doesn’t have to be last, one can choose to putit in the middle of the search path (info "(emacs) General Variables").The trailing separator is just a special case.
WDYT?
Thanks,Ludo’.
M
M
Maxim Cournoyer wrote on 3 Oct 2020 23:22
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43627@debbugs.gnu.org)
87zh53knrh.fsf@gmail.com
Hello Ludovic!
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (17 lines)> 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 notsubmitted for inclusion for 3 years :-)), but recent events surroundingemacs-next and the bump to Emacs 27 have put its value back into light.
Toggle quote (5 lines)> 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 itanywhere else than at the end though, because we want to allow userinstalled libraries to override builtin ones (e.g., a more recent Orgfrom emacs-org package overriding the builtin Org version), not thecontrary.
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 EMACSexportEMACSLOADPATH="/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 beforethe user-installed libraries of the second profile, causing the Orgversion 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 klugdesaround the code where different profiles need to be merged.
On the other hand, if the information is recorded in the search-pathobject itself, we can combined search-path in the way they were meant tobe. In the occurrence, the resulting combined search path would havethe 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 EMACSexportEMACSLOADPATH="/tmp/p1v2/share/emacs/site-lisp:/tmp/p2v2/share/emacs/site-lisp:"
I hope this helps understanding the rationale behind this change.
Thanks,
Maxim
L
L
Ludovic Courtès wrote on 2 Nov 2020 14:59
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 43627@debbugs.gnu.org)
87lffjlv0t.fsf@gnu.org
Hi Maxim,
Apologies for not following up earlier on this one…
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (23 lines)> 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 emptyentry is why we didn’t have it in ‘native-search-paths’ initially, andwhy Alex Kost had come up with a different mechanism instead.
Thanks,Ludo’.
M
M
Maxim Cournoyer wrote on 8 Nov 2020 06:49
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 43627@debbugs.gnu.org)
87h7q0iej7.fsf@gmail.com
Hi Ludovic,
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (4 lines)> Hi Maxim,>> Apologies for not following up earlier on this one…
No worries!
Toggle quote (27 lines)> 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 thisway are EMACSLOADPATH, INFOPATH and MANPATH. There may be others, butit's hard to search for that.
Toggle quote (4 lines)> 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 usingEMACSLOADPATH, I think we would have found out by now :-).
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 30 Mar 17:51 +0200
[PATCH core-updates v2 2/2] gnu: emacs: Use the new append-separator? option to define its search path.
(address . 43627@debbugs.gnu.org)
20210330155102.16610-2-maxim.cournoyer@gmail.com
Fixes https://issues.guix.info/43277.
* gnu/packages/emacs.scm (emacs): Remove the versioned lisp file name from thesearch path, and set the append-separator? field to #t.--- gnu/packages/emacs.scm | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Toggle diff (18 lines)diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scmindex 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
L
L
Leo Prikler wrote on 30 Mar 20:41 +0200
[PATCH] gnu: emacs: Wrap EMACSLOADPATH.
(address . 47458@debbugs.gnu.org)
20210330184101.7643-1-leo.prikler@student.tugraz.at
With this, the search path specification of EMACSLOADPATH does no longerdepend on the version of Emacs, which should make upgrading major versionsless 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(-)
Toggle diff (58 lines)diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scmindex 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
L
L
Ludovic Courtès wrote on 10 Apr 22:42 +0200
Re: [PATCH core-updates v2 1/2] guix: Add an append-separator? field to the <search-path-specification> record.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87o8el27o5.fsf@gnu.org
Hi,
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
Toggle quote (11 lines)> 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. :-)
Leos patch series seems to be addressing the same issue:
https://issues.guix.gnu.org/47661
Should we hold on until weve reviewed and acted upon #47661? Or doesit have known uses apart from Emacs?
Toggle quote (10 lines)> * 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. Id suggest maybe replacing append-separator? bytrailing-separator?, which I find a bit clearer.
Could you add a test or two in tests/search-paths.scm, for the cornercases?
Toggle quote (18 lines)> - ((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
Id fully spell out the variable name, like append-separator?,append?, trailing?, as per our coding style.
Toggle quote (32 lines)> +++ 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, dont we lose the trailing separator?
Toggle quote (28 lines)> +++ 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.
Thats it. Thanks and apologies for the delay!
Ludo.
M
M
Maxim Cournoyer wrote on 20 May 16:24 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)
87eee1zdni.fsf@gmail.com
Hi!
Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (24 lines)> 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
Closed
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send email to 43627@debbugs.gnu.org