Package: guix-patches;
Reported by: Nigko Yerden <nigko.yerden <at> gmail.com>
Date: Thu, 29 Aug 2024 06:10:01 UTC
Severity: normal
Tags: patch
To reply to this bug, email your comments to 72867 AT debbugs.gnu.org.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Thu, 29 Aug 2024 06:10:01 GMT) Full text and rfc822 format available.Nigko Yerden <nigko.yerden <at> gmail.com>:guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org.
(Thu, 29 Aug 2024 06:10:01 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: guix-patches <at> gnu.org Cc: Nigko Yerden <nigko.yerden <at> gmail.com> Subject: [PATCH] gexp: Make 'local-file' follow symlinks. Date: Thu, 29 Aug 2024 11:06:14 +0500
Fixes <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html>
While the issue can be easily fixed (a one line change in 'absolute-dirname')
by changing 'current-source-directory' so that it always follows symlinks,
such a change may break someone else's code. Instead, this patch keeps the
original behavior of 'current-source-directory' macro and adds optional
'follow-symlinks?' argument to it.
This patch is the result of collective work of
Florian Pelz <pelzflorian <at> pelzflorian.de> and
Nigko Yerden <nigko.yerden <at> gmail.com>
* guix/utils.scm (absolute-dirname): Add 'follow-symlinks?' mandatory
argument.
(%guix-source-root-directory): Pass #f to 'absolute-dirname'
'follow-symlinks?' argument.
(current-source-directory): Add 'follow-symlinks?' optional argument.
* guix/gexp.scm (local-file): Pass #t to 'current-source-directory'
'follow-symlinks?' argument.
Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59
---
guix/gexp.scm | 2 +-
guix/utils.scm | 52 ++++++++++++++++++++++++++++----------------------
2 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/guix/gexp.scm b/guix/gexp.scm
index 74b4c49f90..5911ca4815 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -508,7 +508,7 @@ (define-syntax local-file
(string? (syntax->datum #'file))
;; FILE is a literal, so resolve it relative to the source directory.
#'(%local-file file
- (delay (absolute-file-name file (current-source-directory)))
+ (delay (absolute-file-name file (current-source-directory #t)))
rest ...))
((_ (assume-valid-file-name file) rest ...)
;; FILE is not a literal, so resolve it relative to the current
diff --git a/guix/utils.scm b/guix/utils.scm
index d8ce6ed886..b5fcf8cb28 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -1110,41 +1110,47 @@ (define (canonical-newline-port port)
(define (%guix-source-root-directory)
"Return the source root directory of the Guix found in %load-path."
- (dirname (absolute-dirname "guix/packages.scm")))
+ (dirname (absolute-dirname "guix/packages.scm" #f)))
(define absolute-dirname
;; Memoize to avoid repeated 'stat' storms from 'search-path'.
- (mlambda (file)
+ (mlambda (file follow-symlinks?)
"Return the absolute name of the directory containing FILE, or #f upon
-failure."
+failure. Follow symlinks if FOLLOW-SYMLINKS? is true."
(match (search-path %load-path file)
(#f #f)
((? string? file)
- ;; If there are relative names in %LOAD-PATH, FILE can be relative and
- ;; needs to be canonicalized.
- (if (string-prefix? "/" file)
- (dirname file)
- (canonicalize-path (dirname file)))))))
+ (if follow-symlinks?
+ (dirname (canonicalize-path file))
+ ;; If there are relative names in %LOAD-PATH, FILE can be relative
+ ;; and needs to be canonicalized.
+ (if (string-prefix? "/" file)
+ (dirname file)
+ (canonicalize-path (dirname file))))))))
(define-syntax current-source-directory
(lambda (s)
"Return the absolute name of the current directory, or #f if it could not
-be determined."
+be determined. Do not follow symlinks if FOLLOW-SYMLINKS? is false (the default)."
+ (define (source-directory follow-symlinks?)
+ (match (assq 'filename (or (syntax-source s) '()))
+ (('filename . (? string? file-name))
+ ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
+ ;; can be relative. In that case, we try to find out at run time
+ ;; the absolute file name by looking at %LOAD-PATH; doing this at
+ ;; run time rather than expansion time is necessary to allow files
+ ;; to be moved on the file system.
+ (if (string-prefix? "/" file-name)
+ (dirname (if follow-symlinks?
+ (canonicalize-path file-name)
+ file-name))
+ #`(absolute-dirname #,file-name #,follow-symlinks?)))
+ ((or ('filename . #f) #f)
+ ;; raising an error would upset Geiser users
+ #f)))
(syntax-case s ()
- ((_)
- (match (assq 'filename (or (syntax-source s) '()))
- (('filename . (? string? file-name))
- ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
- ;; can be relative. In that case, we try to find out at run time
- ;; the absolute file name by looking at %LOAD-PATH; doing this at
- ;; run time rather than expansion time is necessary to allow files
- ;; to be moved on the file system.
- (if (string-prefix? "/" file-name)
- (dirname file-name)
- #`(absolute-dirname #,file-name)))
- ((or ('filename . #f) #f)
- ;; raising an error would upset Geiser users
- #f))))))
+ ((_) (source-directory #f))
+ ((_ follow-symlinks?) (source-directory #'follow-symlinks?)))))
;;;
base-commit: 4c49cd171e2aa06af05cf52403050b18f100867a
--
2.45.2
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Thu, 29 Aug 2024 07:03:01 GMT) Full text and rfc822 format available.Message #8 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Attila Lendvai <attila <at> lendvai.name> To: "72867 <at> debbugs.gnu.org" <72867 <at> debbugs.gnu.org> Subject: when should local-file and current-source-directory not follow symlinks? Date: Thu, 29 Aug 2024 07:01:37 +0000
pardon my ignorance, but can you give me a (plausible) example when someone wants to load some files relative to a source file, and also wants to be conscious of symlinks, and chose not to follow them? let alone making that the default anywhere around such operations? -- • attila lendvai • PGP: 963F 5D5F 45C7 DFCD 0A39 -- “An armed society is a polite society. Manners are good when one may have to back up his acts with his life.” — Robert Heinlein (1907–1988), 'Beyond This Horizon'
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Thu, 29 Aug 2024 09:02:01 GMT) Full text and rfc822 format available.Message #11 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Nigko Yerden <nigko.yerden <at> gmail.com> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Christopher Baines <guix <at> cbaines.net>, 72867 <at> debbugs.gnu.org Subject: Re: [bug#72867] [PATCH] gexp: Make 'local-file' follow symlinks. Date: Thu, 29 Aug 2024 11:00:16 +0200
Hi Nigko, Nigko Yerden <nigko.yerden <at> gmail.com> skribis: > Fixes <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html> > > While the issue can be easily fixed (a one line change in 'absolute-dirname') > by changing 'current-source-directory' so that it always follows symlinks, > such a change may break someone else's code. Instead, this patch keeps the > original behavior of 'current-source-directory' macro and adds optional > 'follow-symlinks?' argument to it. > > This patch is the result of collective work of > Florian Pelz <pelzflorian <at> pelzflorian.de> and > Nigko Yerden <nigko.yerden <at> gmail.com> I haven’t read the thread above. Could you come up with a test case that shows the problem being fixed? (That is, the test should fail when run on current ‘master’.) That will allow us to “formalize” the issue and to make sure it doesn’t come back later. Thanks for your work, Ludo’.
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Thu, 29 Aug 2024 10:12:01 GMT) Full text and rfc822 format available.Message #14 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Nigko Yerden <nigko.yerden <at> gmail.com>, Christopher Baines <guix <at> cbaines.net>, 72867 <at> debbugs.gnu.org Subject: Re: [bug#72867] [PATCH] gexp: Make 'local-file' follow symlinks. Date: Thu, 29 Aug 2024 12:10:09 +0200
Hello all. Thank you to Nigko for sending the patch.
Nigko Yerden <nigko.yerden <at> gmail.com> writes:
> This patch is the result of collective work of
> Florian Pelz <pelzflorian <at> pelzflorian.de> and
> Nigko Yerden <nigko.yerden <at> gmail.com>
All real contribution to this patch is Nigko’s work.
I contributed only the error location in a failed fix.
Ludovic Courtès <ludo <at> gnu.org> writes:
> I haven’t read the thread above. Could you come up with a test case
> that shows the problem being fixed? (That is, the test should fail when
> run on current ‘master’.)
Nigko sums up the fixed issue in
<https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00071.html>:
> pelzflorian (Florian Pelz) wrote:
>> Nonsense; it must have worked; 7.7 Wrapping Up lists
>> https://git.savannah.gnu.org/cgit/guile.git/tree/.guix/modules/guile-package.scm?id=cd57379b3df636198d8cd8e76c1bfbc523762e79
>> as proof.
> […]
> For me pulling from this channel with subsequent
>
> $ guix build guile <at> 3.0.99-git
>
> throws an error ("No such file or directory" "GUILE-VERSION"). However,
>
> $ GUILE_LOAD_PATH= guix build guile <at> 3.0.99-git
>
> , which emulates system without [1] in Guile load path, works like a charm.
> Thus, this repository behaves exactly as does the main branch of [2].
>
> Perhaps many systems (e.g. Guix on foreign distributions) indeed does not
> have [1] in Guile load path, and thus recipe from the Cookbook works for them.
> Regards,
> Nigko
>
> [1] ~/.config/guix/current/share/guile/site/3.0/
> [2] https://gitlab.com/anigko/test-channel.git
There are currently no tests for `current-source-directory'.
To make a test case like in test/channels.scm, we would have to make
a new guile process or build process, I presume?
Regards,
Florian
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Fri, 30 Aug 2024 12:10:01 GMT) Full text and rfc822 format available.Message #17 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> Cc: 72867 <at> debbugs.gnu.org Subject: Re: [bug#72867] [PATCH] gexp: Make 'local-file' follow symlinks. Date: Fri, 30 Aug 2024 17:07:03 +0500
Hello Florian,
> I contributed only the error location in a failed fix.
Discussions and testings also should be counted. Without your
suggestions I would hardly have made this patch.
Thank you for all this.
> There are currently no tests for `current-source-directory'.
> To make a test case like in test/channels.scm, we would have to make
> a new guile process or build process, I presume?
I was thinking about making a test to 'local-file'. It is natural
taking into account the problem this patch solves sits in
'local-file' bad behavior. But 'current-source-directory'
is fine already.
Regards,
Nigko
pelzflorian (Florian Pelz) wrote:
> Hello all. Thank you to Nigko for sending the patch.
>
> Nigko Yerden <nigko.yerden <at> gmail.com> writes:
>> This patch is the result of collective work of
>> Florian Pelz <pelzflorian <at> pelzflorian.de> and
>> Nigko Yerden <nigko.yerden <at> gmail.com>
>
> All real contribution to this patch is Nigko’s work.
> I contributed only the error location in a failed fix.
>
>
> Ludovic Courtès <ludo <at> gnu.org> writes:
>> I haven’t read the thread above. Could you come up with a test case
>> that shows the problem being fixed? (That is, the test should fail when
>> run on current ‘master’.)
>
> Nigko sums up the fixed issue in
> <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00071.html>:
>> pelzflorian (Florian Pelz) wrote:
>>> Nonsense; it must have worked; 7.7 Wrapping Up lists
>>> https://git.savannah.gnu.org/cgit/guile.git/tree/.guix/modules/guile-package.scm?id=cd57379b3df636198d8cd8e76c1bfbc523762e79
>>> as proof.
>> […]
>> For me pulling from this channel with subsequent
>>
>> $ guix build guile <at> 3.0.99-git
>>
>> throws an error ("No such file or directory" "GUILE-VERSION"). However,
>>
>> $ GUILE_LOAD_PATH= guix build guile <at> 3.0.99-git
>>
>> , which emulates system without [1] in Guile load path, works like a charm.
>> Thus, this repository behaves exactly as does the main branch of [2].
>>
>> Perhaps many systems (e.g. Guix on foreign distributions) indeed does not
>> have [1] in Guile load path, and thus recipe from the Cookbook works for them.
>> Regards,
>> Nigko
>>
>> [1] ~/.config/guix/current/share/guile/site/3.0/
>> [2] https://gitlab.com/anigko/test-channel.git
>
> There are currently no tests for `current-source-directory'.
> To make a test case like in test/channels.scm, we would have to make
> a new guile process or build process, I presume?
>
> Regards,
> Florian
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Fri, 30 Aug 2024 14:03:02 GMT) Full text and rfc822 format available.Message #20 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: Attila Lendvai <attila <at> lendvai.name> Cc: 72867 <at> debbugs.gnu.org Subject: [bug#72867] when should local-file and current-source-directory not follow symlinks? Date: Fri, 30 Aug 2024 19:00:38 +0500
No, I can't give you an example. The original 'current-source-directory' was designed not to follow symlinks. This wasn't my idea. By setting the default I just keep the original behavior. Regards, Nigko Attila Lendvai wrote: > pardon my ignorance, but can you give me a (plausible) example when someone > wants to load some files relative to a source file, and also wants to be > conscious of symlinks, and chose not to follow them? > > let alone making that the default anywhere around such operations? > > -- > • attila lendvai > • PGP: 963F 5D5F 45C7 DFCD 0A39 > -- > “An armed society is a polite society. Manners are good when one may have to > back up his acts with his life.” > — Robert Heinlein (1907–1988), 'Beyond This Horizon'
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Sat, 31 Aug 2024 17:12:02 GMT) Full text and rfc822 format available.Message #23 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> To: Nigko Yerden <nigko.yerden <at> gmail.com> Cc: Attila Lendvai <attila <at> lendvai.name>, 72867 <at> debbugs.gnu.org Subject: Re: [bug#72867] when should local-file and current-source-directory not follow symlinks? Date: Sat, 31 Aug 2024 19:10:05 +0200
Nigko Yerden <nigko.yerden <at> gmail.com> writes: > Attila Lendvai wrote: >> pardon my ignorance, but can you give me a (plausible) example when >> someone wants to load some files relative to a source file, and also >> wants to be conscious of symlinks, and chose not to follow them? let >> alone making that the default anywhere around such operations? > No, I can't give you an example. The original 'current-source-directory' was > designed not to follow symlinks. This wasn't my idea. By setting the default > I just keep the original behavior. I guess not following symlinks was not design but an oversight. Profiles like .config/guix/current have lots of symlinks. Perhaps behavior might change when custom code is processing profiles. If we ignored possible custom code breakage, this patch could be simplified, but not to a one-liner, as it canonicalizes paths in both `current-source-directory' (when not in the load-path) and `absolute-dirname' (when in the load-path). Regards, Florian
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Sun, 01 Sep 2024 14:16:02 GMT) Full text and rfc822 format available.Message #26 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Tobias Geerinckx-Rice <me <at> tobias.gr> To: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de>, Nigko Yerden <nigko.yerden <at> gmail.com>, Ludovic Courtès <ludo <at> gnu.org> Cc: 72867 <at> debbugs.gnu.org Subject: Re: [bug#72867] when should local-file and current-source-directory not follow symlinks? Date: Sun, 01 Sep 2024 16:13:53 +0200
[Message part 1 (text/plain, inline)]
Hi, pelzflorian (Florian Pelz) 写道： > If we ignored possible custom code breakage, this patch could be > simplified Please consider doing so, responsibly[0], if everyone agrees that the current default is suboptimal. Keeping ossified (and unintentional?) quirks around forever has a cost each time someone gets bitten by unintuitive behaviour. It gets less recognition than, but eventually outweighs, any immediate switching costs to out-of-tree users. (…/me quietly eyes substitute*…) Kind regards, T G-R [0]: With a news entry, for example.
[signature.asc (application/pgp-signature, inline)]
guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Mon, 02 Sep 2024 05:26:02 GMT) Full text and rfc822 format available.Message #29 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: 72867 <at> debbugs.gnu.org Cc: pelzflorian <at> pelzflorian.de, Nigko Yerden <nigko.yerden <at> gmail.com> Subject: [PATCH v2] gexp: Make 'local-file' follow symlinks. Date: Mon, 2 Sep 2024 09:41:59 +0500
Fixes <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html>
While the issue can be easily fixed (a one line change in 'absolute-dirname')
by changing 'current-source-directory' so that it always follows symlinks,
such a change may break someone else's code. Instead, this patch keeps the
original behavior of 'current-source-directory' macro and adds optional
'follow-symlinks?' argument to it.
This patch is the result of collective work of
Florian Pelz <pelzflorian <at> pelzflorian.de> and
Nigko Yerden <nigko.yerden <at> gmail.com>
* guix/utils.scm (absolute-dirname): Add 'follow-symlinks?' mandatory
argument.
(%guix-source-root-directory): Pass #f to 'absolute-dirname'
'follow-symlinks?' argument.
(current-source-directory): Add 'follow-symlinks?' optional argument.
* guix/gexp.scm (local-file): Pass #t to 'current-source-directory'
'follow-symlinks?' argument.
* tests/gexp.scm ("local-file, load through symlink"): New test.
Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59
---
Hello Ludo, Florian,
Add test to 'local-file'.
Regards,
Nigko
guix/gexp.scm | 2 +-
guix/utils.scm | 52 ++++++++++++++++++++++++++++----------------------
tests/gexp.scm | 23 ++++++++++++++++++++++
3 files changed, 53 insertions(+), 24 deletions(-)
diff --git a/guix/gexp.scm b/guix/gexp.scm
index 74b4c49f90..5911ca4815 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -508,7 +508,7 @@ (define-syntax local-file
(string? (syntax->datum #'file))
;; FILE is a literal, so resolve it relative to the source directory.
#'(%local-file file
- (delay (absolute-file-name file (current-source-directory)))
+ (delay (absolute-file-name file (current-source-directory #t)))
rest ...))
((_ (assume-valid-file-name file) rest ...)
;; FILE is not a literal, so resolve it relative to the current
diff --git a/guix/utils.scm b/guix/utils.scm
index d8ce6ed886..b5fcf8cb28 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -1110,41 +1110,47 @@ (define (canonical-newline-port port)
(define (%guix-source-root-directory)
"Return the source root directory of the Guix found in %load-path."
- (dirname (absolute-dirname "guix/packages.scm")))
+ (dirname (absolute-dirname "guix/packages.scm" #f)))
(define absolute-dirname
;; Memoize to avoid repeated 'stat' storms from 'search-path'.
- (mlambda (file)
+ (mlambda (file follow-symlinks?)
"Return the absolute name of the directory containing FILE, or #f upon
-failure."
+failure. Follow symlinks if FOLLOW-SYMLINKS? is true."
(match (search-path %load-path file)
(#f #f)
((? string? file)
- ;; If there are relative names in %LOAD-PATH, FILE can be relative and
- ;; needs to be canonicalized.
- (if (string-prefix? "/" file)
- (dirname file)
- (canonicalize-path (dirname file)))))))
+ (if follow-symlinks?
+ (dirname (canonicalize-path file))
+ ;; If there are relative names in %LOAD-PATH, FILE can be relative
+ ;; and needs to be canonicalized.
+ (if (string-prefix? "/" file)
+ (dirname file)
+ (canonicalize-path (dirname file))))))))
(define-syntax current-source-directory
(lambda (s)
"Return the absolute name of the current directory, or #f if it could not
-be determined."
+be determined. Do not follow symlinks if FOLLOW-SYMLINKS? is false (the default)."
+ (define (source-directory follow-symlinks?)
+ (match (assq 'filename (or (syntax-source s) '()))
+ (('filename . (? string? file-name))
+ ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
+ ;; can be relative. In that case, we try to find out at run time
+ ;; the absolute file name by looking at %LOAD-PATH; doing this at
+ ;; run time rather than expansion time is necessary to allow files
+ ;; to be moved on the file system.
+ (if (string-prefix? "/" file-name)
+ (dirname (if follow-symlinks?
+ (canonicalize-path file-name)
+ file-name))
+ #`(absolute-dirname #,file-name #,follow-symlinks?)))
+ ((or ('filename . #f) #f)
+ ;; raising an error would upset Geiser users
+ #f)))
(syntax-case s ()
- ((_)
- (match (assq 'filename (or (syntax-source s) '()))
- (('filename . (? string? file-name))
- ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
- ;; can be relative. In that case, we try to find out at run time
- ;; the absolute file name by looking at %LOAD-PATH; doing this at
- ;; run time rather than expansion time is necessary to allow files
- ;; to be moved on the file system.
- (if (string-prefix? "/" file-name)
- (dirname file-name)
- #`(absolute-dirname #,file-name)))
- ((or ('filename . #f) #f)
- ;; raising an error would upset Geiser users
- #f))))))
+ ((_) (source-directory #f))
+ ((_ follow-symlinks?) (source-directory #'follow-symlinks?)))))
;;;
diff --git a/tests/gexp.scm b/tests/gexp.scm
index b35bfc920f..843037fa84 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -292,6 +292,29 @@ (define %extension-package
(equal? (scandir (string-append dir "/tests"))
'("." ".." "gexp.scm"))))))
+(test-assert "local-file, load through symlink"
+ ;; See <https://issues.guix.gnu.org/72867>.
+ (call-with-temporary-directory
+ (lambda (tmp-dir)
+ (chdir tmp-dir)
+ ;; create content file
+ (call-with-output-file "content"
+ (lambda (port) (display "Hi!" port)))
+ ;; create code that call 'local-file'
+ ;; with the content file and returns its
+ ;; absolute file-name. An error is raised
+ ;; if the content file can't be found.
+ (call-with-output-file "code.scm"
+ (lambda (port) (display "\
+(use-modules (guix gexp))
+(define file (local-file \"content\" \"test-file\"))
+(local-file-absolute-file-name file)" port)))
+ (mkdir "dir")
+ (chdir "dir")
+ (symlink "../code.scm" "link-to-code.scm")
+ ;; call 'local-file' through symlink
+ (primitive-load (string-append tmp-dir "/dir/link-to-code.scm")))))
+
(test-assert "one plain file"
(let* ((file (plain-file "hi" "Hello, world!"))
(exp (gexp (display (ungexp file))))
base-commit: 4c49cd171e2aa06af05cf52403050b18f100867a
--
2.45.2
guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Mon, 02 Sep 2024 08:06:01 GMT) Full text and rfc822 format available.Message #32 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: 72867 <at> debbugs.gnu.org Cc: pelzflorian <at> pelzflorian.de, Nigko Yerden <nigko.yerden <at> gmail.com> Subject: [PATCH v3] gexp: Make 'local-file' follow symlinks. Date: Mon, 2 Sep 2024 12:53:32 +0500
Fixes <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html>
While the issue can be easily fixed (a one line change in 'absolute-dirname')
by changing 'current-source-directory' so that it always follows symlinks,
such a change may break someone else's code. Instead, this patch keeps the
original behavior of 'current-source-directory' macro and adds optional
'follow-symlinks?' argument to it.
This patch is the result of collective work of
Florian Pelz <pelzflorian <at> pelzflorian.de> and
Nigko Yerden <nigko.yerden <at> gmail.com>
* guix/utils.scm (absolute-dirname): Add 'follow-symlinks?' mandatory
argument.
(%guix-source-root-directory): Pass #f to 'absolute-dirname'
'follow-symlinks?' argument.
(current-source-directory): Add 'follow-symlinks?' optional argument.
* guix/gexp.scm (local-file): Pass #t to 'current-source-directory'
'follow-symlinks?' argument.
* tests/gexp.scm ("local-file, load through symlink"): New test.
Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59
---
Forgot to unwrap #'follow-symlinks? syntax object with 'syntax->datum'
when calling 'source-directory' inside 'current-source-directory'.
guix/gexp.scm | 2 +-
guix/utils.scm | 53 ++++++++++++++++++++++++++++----------------------
tests/gexp.scm | 23 ++++++++++++++++++++++
3 files changed, 54 insertions(+), 24 deletions(-)
diff --git a/guix/gexp.scm b/guix/gexp.scm
index 74b4c49f90..5911ca4815 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -508,7 +508,7 @@ (define-syntax local-file
(string? (syntax->datum #'file))
;; FILE is a literal, so resolve it relative to the source directory.
#'(%local-file file
- (delay (absolute-file-name file (current-source-directory)))
+ (delay (absolute-file-name file (current-source-directory #t)))
rest ...))
((_ (assume-valid-file-name file) rest ...)
;; FILE is not a literal, so resolve it relative to the current
diff --git a/guix/utils.scm b/guix/utils.scm
index d8ce6ed886..ea3d80707e 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -1110,41 +1110,48 @@ (define (canonical-newline-port port)
(define (%guix-source-root-directory)
"Return the source root directory of the Guix found in %load-path."
- (dirname (absolute-dirname "guix/packages.scm")))
+ (dirname (absolute-dirname "guix/packages.scm" #f)))
(define absolute-dirname
;; Memoize to avoid repeated 'stat' storms from 'search-path'.
- (mlambda (file)
+ (mlambda (file follow-symlinks?)
"Return the absolute name of the directory containing FILE, or #f upon
-failure."
+failure. Follow symlinks if FOLLOW-SYMLINKS? is true."
(match (search-path %load-path file)
(#f #f)
((? string? file)
- ;; If there are relative names in %LOAD-PATH, FILE can be relative and
- ;; needs to be canonicalized.
- (if (string-prefix? "/" file)
- (dirname file)
- (canonicalize-path (dirname file)))))))
+ (if follow-symlinks?
+ (dirname (canonicalize-path file))
+ ;; If there are relative names in %LOAD-PATH, FILE can be relative
+ ;; and needs to be canonicalized.
+ (if (string-prefix? "/" file)
+ (dirname file)
+ (canonicalize-path (dirname file))))))))
(define-syntax current-source-directory
(lambda (s)
"Return the absolute name of the current directory, or #f if it could not
-be determined."
+be determined. Do not follow symlinks if FOLLOW-SYMLINKS? is false (the default)."
+ (define (source-directory follow-symlinks?)
+ (match (assq 'filename (or (syntax-source s) '()))
+ (('filename . (? string? file-name))
+ ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
+ ;; can be relative. In that case, we try to find out at run time
+ ;; the absolute file name by looking at %LOAD-PATH; doing this at
+ ;; run time rather than expansion time is necessary to allow files
+ ;; to be moved on the file system.
+ (if (string-prefix? "/" file-name)
+ (dirname (if follow-symlinks?
+ (canonicalize-path file-name)
+ file-name))
+ #`(absolute-dirname #,file-name #,follow-symlinks?)))
+ ((or ('filename . #f) #f)
+ ;; raising an error would upset Geiser users
+ #f)))
(syntax-case s ()
- ((_)
- (match (assq 'filename (or (syntax-source s) '()))
- (('filename . (? string? file-name))
- ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
- ;; can be relative. In that case, we try to find out at run time
- ;; the absolute file name by looking at %LOAD-PATH; doing this at
- ;; run time rather than expansion time is necessary to allow files
- ;; to be moved on the file system.
- (if (string-prefix? "/" file-name)
- (dirname file-name)
- #`(absolute-dirname #,file-name)))
- ((or ('filename . #f) #f)
- ;; raising an error would upset Geiser users
- #f))))))
+ ((_) (source-directory #f))
+ ((_ follow-symlinks?)
+ (source-directory (syntax->datum #'follow-symlinks?))))))
;;;
diff --git a/tests/gexp.scm b/tests/gexp.scm
index b35bfc920f..843037fa84 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -292,6 +292,29 @@ (define %extension-package
(equal? (scandir (string-append dir "/tests"))
'("." ".." "gexp.scm"))))))
+(test-assert "local-file, load through symlink"
+ ;; See <https://issues.guix.gnu.org/72867>.
+ (call-with-temporary-directory
+ (lambda (tmp-dir)
+ (chdir tmp-dir)
+ ;; create content file
+ (call-with-output-file "content"
+ (lambda (port) (display "Hi!" port)))
+ ;; create code that call 'local-file'
+ ;; with the content file and returns its
+ ;; absolute file-name. An error is raised
+ ;; if the content file can't be found.
+ (call-with-output-file "code.scm"
+ (lambda (port) (display "\
+(use-modules (guix gexp))
+(define file (local-file \"content\" \"test-file\"))
+(local-file-absolute-file-name file)" port)))
+ (mkdir "dir")
+ (chdir "dir")
+ (symlink "../code.scm" "link-to-code.scm")
+ ;; call 'local-file' through symlink
+ (primitive-load (string-append tmp-dir "/dir/link-to-code.scm")))))
+
(test-assert "one plain file"
(let* ((file (plain-file "hi" "Hello, world!"))
(exp (gexp (display (ungexp file))))
base-commit: 4c49cd171e2aa06af05cf52403050b18f100867a
--
2.45.2
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Tue, 03 Sep 2024 15:07:01 GMT) Full text and rfc822 format available.Message #35 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> To: Nigko Yerden <nigko.yerden <at> gmail.com> Cc: 72867 <at> debbugs.gnu.org Subject: Re: [PATCH v3] gexp: Make 'local-file' follow symlinks. Date: Tue, 03 Sep 2024 17:05:44 +0200
Hello Nigko.
Nigko Yerden <nigko.yerden <at> gmail.com> writes:
> This patch is the result of collective work of
> Florian Pelz <pelzflorian <at> pelzflorian.de> and
> Nigko Yerden <nigko.yerden <at> gmail.com>
Thanks for the credit, but it would be unusual to mention me in the
commit message, where discussion does not count.
Please do not put me in the commit message; I made no code contribution.
I also would favor to simplify `current-source-directory' and not add an
optional follow-symlinks? argument. I believe processing profiles is
the only reasonable case that unconditionally following symlinks would
break, and people do not do profile processing in outside code.
> * tests/gexp.scm ("local-file, load through symlink"): New test.
This one is a good test; but it tests only half, namely the
rare-in-practice case of `local-file' when loading a Scheme file. Here,
`current-source-directory' evaluate file-name to
"/tmp/guix-directory.VxrxZT/dir/link-to-code.scm", which has a slash as
prefix, so absolute-dirname is not called.
The original issue is that the package in a channel according to
cookbook’s “The Repository as a Channel” cannot be built when the
load-path is set up in the usual way. There, absolute-dirname gets
called. I think we would need a (very similar) test that covers this.
Instead of primitive-load, we would need to invoke Guile on a file in a
channel or in the GUILE_LOAD_PATH, or set %load-path. I may be wrong
here and do not know how, but we definitely should cover when
`file-name' in is not prefixed with a slash.
Regards,
Florian
guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Thu, 05 Sep 2024 04:26:02 GMT) Full text and rfc822 format available.Message #38 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: 72867 <at> debbugs.gnu.org Cc: Florian Pelz <pelzflorian <at> pelzflorian.de>, Nigko Yerden <nigko.yerden <at> gmail.com> Subject: [PATCH v4] gexp: Make 'local-file' follow symlinks. Date: Thu, 5 Sep 2024 09:16:51 +0500
Fixes <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html>
While the issue can be easily fixed (a one line change in 'absolute-dirname')
by changing 'current-source-directory' so that it always follows symlinks,
such a change may break someone else's code. Instead, this patch keeps the
original behavior of 'current-source-directory' macro and adds optional
'follow-symlinks?' argument to it.
;;; Copyright © 2024 Nigko Yerden <nigko.yerden <at> gmail.com>
* guix/utils.scm (absolute-dirname): Add 'follow-symlinks?' mandatory
argument.
(%guix-source-root-directory): Pass #f to 'absolute-dirname'
'follow-symlinks?' argument.
(current-source-directory): Add 'follow-symlinks?' optional argument.
* guix/gexp.scm (local-file): Pass #t to 'current-source-directory'
'follow-symlinks?' argument.
* tests/gexp.scm ("local-file, load through symlink"): New test.
Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59
---
Hello Florian,
pelzflorian (Florian Pelz) wrote:
>> * tests/gexp.scm ("local-file, load through symlink"): New test.
>
>This one is a good test; but it tests only half, namely the
>rare-in-practice case of `local-file' when loading a Scheme file. Here,
>`current-source-directory' evaluate file-name to
>"/tmp/guix-directory.VxrxZT/dir/link-to-code.scm", which has a slash as
>prefix, so absolute-dirname is not called.
Thanks for noticing this. Indeed 'absolute-dirname' was not called.
I have fixed this by turning 'code.scm' into a module 'test-local-file.scm'
and loading it twice: first using 'use-module' and then via 'load'
(for some unclear reason 'primitive-load' causes an error here, so
I replaced it with 'load').
>Thanks for the credit, but it would be unusual to mention me in the
>commit message, where discussion does not count.
>Please do not put me in the commit message; I made no code contribution.
OK, I removed your name from the commit message.
Regards,
Nigko
guix/gexp.scm | 2 +-
guix/utils.scm | 53 ++++++++++++++++++++++++++++----------------------
tests/gexp.scm | 33 +++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/guix/gexp.scm b/guix/gexp.scm
index 74b4c49f90..5911ca4815 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -508,7 +508,7 @@ (define-syntax local-file
(string? (syntax->datum #'file))
;; FILE is a literal, so resolve it relative to the source directory.
#'(%local-file file
- (delay (absolute-file-name file (current-source-directory)))
+ (delay (absolute-file-name file (current-source-directory #t)))
rest ...))
((_ (assume-valid-file-name file) rest ...)
;; FILE is not a literal, so resolve it relative to the current
diff --git a/guix/utils.scm b/guix/utils.scm
index d8ce6ed886..ea3d80707e 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -1110,41 +1110,48 @@ (define (canonical-newline-port port)
(define (%guix-source-root-directory)
"Return the source root directory of the Guix found in %load-path."
- (dirname (absolute-dirname "guix/packages.scm")))
+ (dirname (absolute-dirname "guix/packages.scm" #f)))
(define absolute-dirname
;; Memoize to avoid repeated 'stat' storms from 'search-path'.
- (mlambda (file)
+ (mlambda (file follow-symlinks?)
"Return the absolute name of the directory containing FILE, or #f upon
-failure."
+failure. Follow symlinks if FOLLOW-SYMLINKS? is true."
(match (search-path %load-path file)
(#f #f)
((? string? file)
- ;; If there are relative names in %LOAD-PATH, FILE can be relative and
- ;; needs to be canonicalized.
- (if (string-prefix? "/" file)
- (dirname file)
- (canonicalize-path (dirname file)))))))
+ (if follow-symlinks?
+ (dirname (canonicalize-path file))
+ ;; If there are relative names in %LOAD-PATH, FILE can be relative
+ ;; and needs to be canonicalized.
+ (if (string-prefix? "/" file)
+ (dirname file)
+ (canonicalize-path (dirname file))))))))
(define-syntax current-source-directory
(lambda (s)
"Return the absolute name of the current directory, or #f if it could not
-be determined."
+be determined. Do not follow symlinks if FOLLOW-SYMLINKS? is false (the default)."
+ (define (source-directory follow-symlinks?)
+ (match (assq 'filename (or (syntax-source s) '()))
+ (('filename . (? string? file-name))
+ ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
+ ;; can be relative. In that case, we try to find out at run time
+ ;; the absolute file name by looking at %LOAD-PATH; doing this at
+ ;; run time rather than expansion time is necessary to allow files
+ ;; to be moved on the file system.
+ (if (string-prefix? "/" file-name)
+ (dirname (if follow-symlinks?
+ (canonicalize-path file-name)
+ file-name))
+ #`(absolute-dirname #,file-name #,follow-symlinks?)))
+ ((or ('filename . #f) #f)
+ ;; raising an error would upset Geiser users
+ #f)))
(syntax-case s ()
- ((_)
- (match (assq 'filename (or (syntax-source s) '()))
- (('filename . (? string? file-name))
- ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
- ;; can be relative. In that case, we try to find out at run time
- ;; the absolute file name by looking at %LOAD-PATH; doing this at
- ;; run time rather than expansion time is necessary to allow files
- ;; to be moved on the file system.
- (if (string-prefix? "/" file-name)
- (dirname file-name)
- #`(absolute-dirname #,file-name)))
- ((or ('filename . #f) #f)
- ;; raising an error would upset Geiser users
- #f))))))
+ ((_) (source-directory #f))
+ ((_ follow-symlinks?)
+ (source-directory (syntax->datum #'follow-symlinks?))))))
;;;
diff --git a/tests/gexp.scm b/tests/gexp.scm
index b35bfc920f..8f267214cd 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -292,6 +292,39 @@ (define %extension-package
(equal? (scandir (string-append dir "/tests"))
'("." ".." "gexp.scm"))))))
+(test-assert "local-file, load through symlink"
+ ;; See <https://issues.guix.gnu.org/72867>.
+ (call-with-temporary-directory
+ (lambda (tmp-dir)
+ (chdir tmp-dir)
+ ;; create content file
+ (call-with-output-file "content"
+ (lambda (port) (display "Hi!" port)))
+ ;; Create module that call 'local-file'
+ ;; with the content file and returns its
+ ;; absolute file-name. An error is raised
+ ;; if the content file can't be found.
+ (call-with-output-file "test-local-file.scm"
+ (lambda (port) (display "\
+(define-module (test-local-file)
+ #:use-module (guix gexp))
+(define file (local-file \"content\" \"test-file\"))
+(local-file-absolute-file-name file)" port)))
+ (mkdir "dir")
+ (chdir "dir")
+ (symlink "../test-local-file.scm" "test-local-file.scm")
+ ;; 'local-file' in turn calls 'current-source-directory'
+ ;; which has an 'if' branching condition depending on whether
+ ;; 'file-name' is absolute or relative path. To test both
+ ;; of these branches we execute 'test-local-file.scm' symlink
+ ;; first as a module (corresponds to relative path):
+ (eval (begin
+ (add-to-load-path ".")
+ (use-modules (test-local-file)))
+ (current-module))
+ ;; and then as a regular code (corresponds to absolute path):
+ (load (string-append tmp-dir "/dir/test-local-file.scm")))))
+
(test-assert "one plain file"
(let* ((file (plain-file "hi" "Hello, world!"))
(exp (gexp (display (ungexp file))))
base-commit: 4c49cd171e2aa06af05cf52403050b18f100867a
--
2.45.2
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Thu, 05 Sep 2024 05:09:01 GMT) Full text and rfc822 format available.Message #41 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> Cc: 72867 <at> debbugs.gnu.org Subject: Re: [PATCH v3] gexp: Make 'local-file' follow symlinks. Date: Thu, 5 Sep 2024 10:06:22 +0500
Hello Florian, pelzflorian (Florian Pelz) wrote: > I also would favor to simplify `current-source-directory' and not add an > optional follow-symlinks? argument. I believe processing profiles is > the only reasonable case that unconditionally following symlinks would > break, and people do not do profile processing in outside code. Why do you think that making 'current-source-directory' to always follow symlinks would not break Guix's own code as well? What are these people whose code would be broken supposed to do? I think they would need to write their own 'current-source-directory' from scratch. Why not help them by providing 'follow-symlinks?' argument? Instead of 'current-source-directory' simplification we can also consider changing the default for 'follow-symlinks?' to #t. Regards, Nigko
guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Fri, 06 Sep 2024 04:33:02 GMT) Full text and rfc822 format available.Message #44 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: 72867 <at> debbugs.gnu.org Cc: Florian Pelz <pelzflorian <at> pelzflorian.de>, Nigko Yerden <nigko.yerden <at> gmail.com> Subject: [PATCH v5] gexp: Make 'local-file' follow symlinks. Date: Fri, 6 Sep 2024 09:17:42 +0500
Fixes <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html>
While the issue can be easily fixed (a one line change in 'absolute-dirname')
by changing 'current-source-directory' so that it always follows symlinks,
such a change may break someone else's code. Instead, this patch keeps the
original behavior of 'current-source-directory' macro and adds optional
'follow-symlinks?' argument to it.
;;; Copyright © 2024 Nigko Yerden <nigko.yerden <at> gmail.com>
* guix/utils.scm (absolute-dirname): Add 'follow-symlinks?' mandatory
argument.
(%guix-source-root-directory): Pass #f to 'absolute-dirname'
'follow-symlinks?' argument.
(current-source-directory): Add 'follow-symlinks?' optional argument.
* guix/gexp.scm (local-file): Pass #t to 'current-source-directory'
'follow-symlinks?' argument.
* tests/gexp.scm ("local-file, load through symlink"): New test.
Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59
---
Using of 'eval' in test from v4 is wrong. It does not play any role there.
Most importantly it does not prevent spoiling of '%load-path' for the
rest of 'tests/gexp.scm' module. Here is the better version of the test
that uses 'dynamic-wind'.
guix/gexp.scm | 2 +-
guix/utils.scm | 53 ++++++++++++++++++++++++++++----------------------
tests/gexp.scm | 33 +++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/guix/gexp.scm b/guix/gexp.scm
index 74b4c49f90..5911ca4815 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -508,7 +508,7 @@ (define-syntax local-file
(string? (syntax->datum #'file))
;; FILE is a literal, so resolve it relative to the source directory.
#'(%local-file file
- (delay (absolute-file-name file (current-source-directory)))
+ (delay (absolute-file-name file (current-source-directory #t)))
rest ...))
((_ (assume-valid-file-name file) rest ...)
;; FILE is not a literal, so resolve it relative to the current
diff --git a/guix/utils.scm b/guix/utils.scm
index d8ce6ed886..ea3d80707e 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -1110,41 +1110,48 @@ (define (canonical-newline-port port)
(define (%guix-source-root-directory)
"Return the source root directory of the Guix found in %load-path."
- (dirname (absolute-dirname "guix/packages.scm")))
+ (dirname (absolute-dirname "guix/packages.scm" #f)))
(define absolute-dirname
;; Memoize to avoid repeated 'stat' storms from 'search-path'.
- (mlambda (file)
+ (mlambda (file follow-symlinks?)
"Return the absolute name of the directory containing FILE, or #f upon
-failure."
+failure. Follow symlinks if FOLLOW-SYMLINKS? is true."
(match (search-path %load-path file)
(#f #f)
((? string? file)
- ;; If there are relative names in %LOAD-PATH, FILE can be relative and
- ;; needs to be canonicalized.
- (if (string-prefix? "/" file)
- (dirname file)
- (canonicalize-path (dirname file)))))))
+ (if follow-symlinks?
+ (dirname (canonicalize-path file))
+ ;; If there are relative names in %LOAD-PATH, FILE can be relative
+ ;; and needs to be canonicalized.
+ (if (string-prefix? "/" file)
+ (dirname file)
+ (canonicalize-path (dirname file))))))))
(define-syntax current-source-directory
(lambda (s)
"Return the absolute name of the current directory, or #f if it could not
-be determined."
+be determined. Do not follow symlinks if FOLLOW-SYMLINKS? is false (the default)."
+ (define (source-directory follow-symlinks?)
+ (match (assq 'filename (or (syntax-source s) '()))
+ (('filename . (? string? file-name))
+ ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
+ ;; can be relative. In that case, we try to find out at run time
+ ;; the absolute file name by looking at %LOAD-PATH; doing this at
+ ;; run time rather than expansion time is necessary to allow files
+ ;; to be moved on the file system.
+ (if (string-prefix? "/" file-name)
+ (dirname (if follow-symlinks?
+ (canonicalize-path file-name)
+ file-name))
+ #`(absolute-dirname #,file-name #,follow-symlinks?)))
+ ((or ('filename . #f) #f)
+ ;; raising an error would upset Geiser users
+ #f)))
(syntax-case s ()
- ((_)
- (match (assq 'filename (or (syntax-source s) '()))
- (('filename . (? string? file-name))
- ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME
- ;; can be relative. In that case, we try to find out at run time
- ;; the absolute file name by looking at %LOAD-PATH; doing this at
- ;; run time rather than expansion time is necessary to allow files
- ;; to be moved on the file system.
- (if (string-prefix? "/" file-name)
- (dirname file-name)
- #`(absolute-dirname #,file-name)))
- ((or ('filename . #f) #f)
- ;; raising an error would upset Geiser users
- #f))))))
+ ((_) (source-directory #f))
+ ((_ follow-symlinks?)
+ (source-directory (syntax->datum #'follow-symlinks?))))))
;;;
diff --git a/tests/gexp.scm b/tests/gexp.scm
index b35bfc920f..eec0f6e7ca 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -292,6 +292,39 @@ (define %extension-package
(equal? (scandir (string-append dir "/tests"))
'("." ".." "gexp.scm"))))))
+(test-assert "local-file, load through symlink"
+ ;; See <https://issues.guix.gnu.org/72867>.
+ (call-with-temporary-directory
+ (lambda (tmp-dir)
+ (chdir tmp-dir)
+ ;; create content file
+ (call-with-output-file "content"
+ (lambda (port) (display "Hi!" port)))
+ ;; Create module that call 'local-file'
+ ;; with the content file and returns its
+ ;; absolute file-name. An error is raised
+ ;; if the content file can't be found.
+ (call-with-output-file "test-local-file.scm"
+ (lambda (port) (display "\
+(define-module (test-local-file)
+ #:use-module (guix gexp))
+(define file (local-file \"content\" \"test-file\"))
+(local-file-absolute-file-name file)" port)))
+ (mkdir "dir")
+ (chdir "dir")
+ (symlink "../test-local-file.scm" "test-local-file.scm")
+ ;; 'local-file' in turn calls 'current-source-directory'
+ ;; which has an 'if' branching condition depending on whether
+ ;; 'file-name' is absolute or relative path. To test both
+ ;; of these branches we execute 'test-local-file.scm' symlink
+ ;; first as a module (corresponds to relative path):
+ (dynamic-wind
+ (lambda () (set! %load-path (cons "." %load-path)))
+ (lambda () (use-modules (test-local-file)))
+ (lambda () (set! %load-path (cdr %load-path))))
+ ;; and then as a regular code (corresponds to absolute path):
+ (load (string-append tmp-dir "/dir/test-local-file.scm")))))
+
(test-assert "one plain file"
(let* ((file (plain-file "hi" "Hello, world!"))
(exp (gexp (display (ungexp file))))
base-commit: 4c49cd171e2aa06af05cf52403050b18f100867a
--
2.45.2
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Sat, 07 Sep 2024 07:37:02 GMT) Full text and rfc822 format available.Message #47 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> To: Nigko Yerden <nigko.yerden <at> gmail.com> Cc: Tobias Geerinckx-Rice <me <at> tobias.gr>, Ludovic Courtès <ludo <at> gnu.org>, 72867 <at> debbugs.gnu.org, Attila Lendvai <attila <at> lendvai.name> Subject: Re: [bug#72867] [PATCH v5] gexp: Make 'local-file' follow symlinks. Date: Sat, 07 Sep 2024 09:35:54 +0200
Hello Nigko, Tobias, Ludo, Attila (putting them in Cc again). Nigko Yerden <nigko.yerden <at> gmail.com> writes: > Using of 'eval' in test from v4 is wrong. It does not play any role there. > Most importantly it does not prevent spoiling of '%load-path' for the > rest of 'tests/gexp.scm' module. Here is the better version of the test > that uses 'dynamic-wind'. The test is beautiful. It makes clear why each canonicalize-path is needed. I had not understood the issue entirely before I read it. When we follow symlinks, both calling the real "../test-local-file.scm" and the symlink to it behaves the same. When not following symlinks, we get different results depending on what we run. I see no reason to ever want that except displaying info or debugging. And except that not following symlinks is faster (fewer stat syscalls). But Ludovic wrapped absolute-dirname in a memoizing mlambda in commit 87b711d200ad13eaef284bdd1ab77f85618b0498, which reduces the difference. Regarding the code, if we kept the old code when `follow-symlinks?' is false (we should not, but if we did), it remains surprising that we do follow some symlinks. > + (if follow-symlinks? > + (dirname (canonicalize-path file)) > + ;; If there are relative names in %LOAD-PATH, FILE can be relative > + ;; and needs to be canonicalized. > + (if (string-prefix? "/" file) > + (dirname file) > + (canonicalize-path (dirname file)))))))) In the new use-modules part of the test, `file-name' in `current-source-directory' is "./test-local-file.scm" code, which means if we look at what happens if we not follow-symlinks, we took the latter (canonicalize-path (dirname file)) path in > (if (string-prefix? "/" file) > (dirname file) > (canonicalize-path (dirname file)))))))) which does not follow the symlink in the basename and fails. But it would follow symlinks in the directory part. Regards, Florian
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Wed, 25 Sep 2024 05:18:02 GMT) Full text and rfc822 format available.Message #50 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> To: Nigko Yerden <nigko.yerden <at> gmail.com> Cc: Ludovic Courtès <ludo <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, 72867 <at> debbugs.gnu.org, Attila Lendvai <attila <at> lendvai.name> Subject: Re: [bug#72867] [PATCH v5] gexp: Make 'local-file' follow symlinks. Date: Wed, 25 Sep 2024 07:16:45 +0200
[Message part 1 (text/plain, inline)]
Hello Nigko.
[news (text/plain, inline)]
diff --git a/etc/news.scm b/etc/news.scm
index a90f92a9ff..5a32eee7f5 100644
--- a/etc/news.scm
+++ b/etc/news.scm
@@ -33,6 +33,18 @@
(channel-news
(version 0)
+ (entry (commit "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+ (title
+ (en "local-file behaves consistently for symlinks"))
+ (body
+ (en "Previous behavior differed between whether someone
+loaded the symlink or the actual scheme file. One of them had to be
+broken, at least when loading a channel module.
+
+Affected users who expected paths relative to the symlink would need
+to append "/../.." or similar or the relative path to the “real” file
+the symlink points to to the path.")))
+
(entry (commit "2fae63df2138b74d30e120364f0f272871595862")
(title
(en "Core packages updated")
[Message part 3 (text/plain, inline)]
They would not have to write code like `current-source-directory'. Also note that such affected users had broken code when running the real file. `local-file' with absolute paths always did `(canonicalize-path (dirname` and does not change. `dirname' being called in a special case of `current-source-directory', `canonicalize-path' as part of `absolute-file-name'. Could we finish this bug report by applying your nice test code, but changing only `absolute-dirname' to do (canonicalize-path (dirname)) in all cases? Then either add no news item, or write the above? Regards, Florian
guix <at> cbaines.net, dev <at> jpoiret.xyz, ludo <at> gnu.org, othacehe <at> gnu.org, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Thu, 26 Sep 2024 07:56:01 GMT) Full text and rfc822 format available.Message #53 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Nigko Yerden <nigko.yerden <at> gmail.com> To: 72867 <at> debbugs.gnu.org Cc: Attila Lendvai <attila <at> lendvai.name>, pelzflorian <pelzflorian <at> pelzflorian.de>, Nigko Yerden <nigko.yerden <at> gmail.com> Subject: [PATCH v6] gexp: Make 'local-file' follow symlinks. Date: Thu, 26 Sep 2024 12:07:56 +0500
Fix <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html>
via making 'current-source-directory' always follow symlinks.
* guix/utils.scm (absolute-dirname, current-source-directory): Make
them follow symlinks.
* tests/gexp.scm ("local-file, load through symlink"): New test.
Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59
---
Hello all,
This version of patch advocated by Florian changes 'current-source-directory'
to always follow symlinks.
Regards,
Nigko
guix/utils.scm | 8 ++------
tests/gexp.scm | 33 +++++++++++++++++++++++++++++++++
2 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/guix/utils.scm b/guix/utils.scm
index f161cb4ef3..d4591caced 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -1121,11 +1121,7 @@ (define absolute-dirname
(match (search-path %load-path file)
(#f #f)
((? string? file)
- ;; If there are relative names in %LOAD-PATH, FILE can be relative and
- ;; needs to be canonicalized.
- (if (string-prefix? "/" file)
- (dirname file)
- (canonicalize-path (dirname file)))))))
+ (dirname (canonicalize-path file))))))
(define-syntax current-source-directory
(lambda (s)
@@ -1141,7 +1137,7 @@ (define-syntax current-source-directory
;; run time rather than expansion time is necessary to allow files
;; to be moved on the file system.
(if (string-prefix? "/" file-name)
- (dirname file-name)
+ (dirname (canonicalize-path file-name))
#`(absolute-dirname #,file-name)))
((or ('filename . #f) #f)
;; raising an error would upset Geiser users
diff --git a/tests/gexp.scm b/tests/gexp.scm
index e066076c5c..cd502a1fb2 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -298,6 +298,39 @@ (define %extension-package
(equal? (scandir (string-append dir "/tests"))
'("." ".." "gexp.scm"))))))
+(test-assert "local-file, load through symlink"
+ ;; See <https://issues.guix.gnu.org/72867>.
+ (call-with-temporary-directory
+ (lambda (tmp-dir)
+ (chdir tmp-dir)
+ ;; create content file
+ (call-with-output-file "content"
+ (lambda (port) (display "Hi!" port)))
+ ;; Create module that call 'local-file'
+ ;; with the content file and returns its
+ ;; absolute file-name. An error is raised
+ ;; if the content file can't be found.
+ (call-with-output-file "test-local-file.scm"
+ (lambda (port) (display "\
+(define-module (test-local-file)
+ #:use-module (guix gexp))
+(define file (local-file \"content\" \"test-file\"))
+(local-file-absolute-file-name file)" port)))
+ (mkdir "dir")
+ (chdir "dir")
+ (symlink "../test-local-file.scm" "test-local-file.scm")
+ ;; 'local-file' in turn calls 'current-source-directory'
+ ;; which has an 'if' branching condition depending on whether
+ ;; 'file-name' is absolute or relative path. To test both
+ ;; of these branches we execute 'test-local-file.scm' symlink
+ ;; first as a module (corresponds to relative path):
+ (dynamic-wind
+ (lambda () (set! %load-path (cons "." %load-path)))
+ (lambda () (use-modules (test-local-file)))
+ (lambda () (set! %load-path (cdr %load-path))))
+ ;; and then as a regular code (corresponds to absolute path):
+ (load (string-append tmp-dir "/dir/test-local-file.scm")))))
+
(test-assert "one plain file"
(let* ((file (plain-file "hi" "Hello, world!"))
(exp (gexp (display (ungexp file))))
base-commit: 404dbd894c69c94b483c6139d2a39b1c1eaddf36
--
2.46.0
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Wed, 02 Oct 2024 16:16:02 GMT) Full text and rfc822 format available.Message #56 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Nigko Yerden <nigko.yerden <at> gmail.com> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, pelzflorian <pelzflorian <at> pelzflorian.de>, Christopher Baines <guix <at> cbaines.net>, Attila Lendvai <attila <at> lendvai.name>, 72867 <at> debbugs.gnu.org Subject: Re: [bug#72867] [PATCH v6] gexp: Make 'local-file' follow symlinks. Date: Wed, 02 Oct 2024 18:15:45 +0200
Hi Nigko,
Nigko Yerden <nigko.yerden <at> gmail.com> skribis:
> Fix <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html>
> via making 'current-source-directory' always follow symlinks.
>
> * guix/utils.scm (absolute-dirname, current-source-directory): Make
> them follow symlinks.
> * tests/gexp.scm ("local-file, load through symlink"): New test.
>
> Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59
[...]
> --- a/guix/utils.scm
> +++ b/guix/utils.scm
> @@ -1121,11 +1121,7 @@ (define absolute-dirname
> (match (search-path %load-path file)
> (#f #f)
> ((? string? file)
> - ;; If there are relative names in %LOAD-PATH, FILE can be relative and
> - ;; needs to be canonicalized.
> - (if (string-prefix? "/" file)
> - (dirname file)
> - (canonicalize-path (dirname file)))))))
> + (dirname (canonicalize-path file))))))
Am I right that we cannot keep the ‘if’ here, as it would perform
“lexical” dot-dot resolution instead of Unix resolution (accounting for
symlinks), right?
> @@ -1141,7 +1137,7 @@ (define-syntax current-source-directory
> ;; run time rather than expansion time is necessary to allow files
> ;; to be moved on the file system.
> (if (string-prefix? "/" file-name)
> - (dirname file-name)
> + (dirname (canonicalize-path file-name))
Note that ‘current-source-directory’ is a macro; using
‘canonicalize-path’ here could lead to an exception being thrown at
macro-expansion time, if ‘file-name’ doesn’t exist. This normally
doesn’t happen but maybe we should handle this gracefully?
The downside of these two changes is that this leads to potentially many
‘canonicalize-path’ calls, which are expensive (see the output of
‘strace’). This could become a problem if, for example, a channel has
many package definitions that refer to patches and auxiliary files via
‘local-file’.
Can this be avoided?
Another issue is that it changes the semantics of
‘current-source-directory’ in the presence of symlinks. That’s the
whole point, but I wonder if that’s always desirable (see below).
> +(test-assert "local-file, load through symlink"
> + ;; See <https://issues.guix.gnu.org/72867>.
> + (call-with-temporary-directory
> + (lambda (tmp-dir)
> + (chdir tmp-dir)
Below is another way to write this test:
1. Using ‘with-directory-excursion’ so the current directory is
switched back to what it was after this test.
2. Using ‘resolve-module’ instead of ‘use-modules’ (the latter should
only be used at the top level).
3. Tweaked the comments.
--8<---------------cut here---------------start------------->8---
(test-assert "local-file, load through symlink"
;; See <https://issues.guix.gnu.org/72867>.
(call-with-temporary-directory
(lambda (tmp-dir)
(with-directory-excursion tmp-dir
;; create content file
(call-with-output-file "content"
(lambda (port) (display "Hi!" port)))
;; Create a module that calls 'local-file' with the "content" file and
;; returns its absolute file name. An error is raised if the "content"
;; file can't be found.
(call-with-output-file "test-local-file.scm"
(lambda (port) (display "\
(define-module (test-local-file)
#:use-module (guix gexp))
(define file (local-file \"content\" \"test-file\"))
(local-file-absolute-file-name file)" port)))
(mkdir "dir")
(symlink "../test-local-file.scm" "dir/test-local-file.scm")
;; 'local-file' in turn calls 'current-source-directory' which has an
;; 'if' branching condition depending on whether 'file-name' is
;; absolute or relative file name. To test both of these branches we
;; execute 'test-local-file.scm' symlink first as a module (corresponds
;; to relative file name):
(dynamic-wind
(lambda () (set! %load-path (cons "dir" %load-path)))
(lambda () (resolve-module '(test-local-file) #:ensure #f))
(lambda () (set! %load-path (cdr %load-path))))
;; and then as a regular code (corresponds to absolute file name):
(load (string-append tmp-dir "/dir/test-local-file.scm"))))))
--8<---------------cut here---------------end--------------->8---
But… here we have:
/tmpdir
|
+--- test-local-file.scm
+--- content
+--- dir
|
+--- test-local-file.scm
To me, it’s not unreasonable for (local-file "content") to fail when
loading ‘dir/test-local-file.scm’. I would say that this is what most
people would expect.
So maybe we should go back to the actual use case and take a step back:
https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html
I didn’t hit this problem, presumably because my GUILE_LOAD_PATH does
not contain ‘~/.config/guix/current/share/guile/site/3.0’ (I use Guile
and Shepherd as channels¹).
Is there anything else we can do to address this?
Sorry for providing more questions that answers!
Thanks,
Ludo’.
¹ (append (list (channel
(name 'shepherd)
(url "https://git.savannah.gnu.org/git/shepherd.git")
(branch "devel")
(introduction
(make-channel-introduction
"788a6d6f1d5c170db68aa4bbfb77024fdc468ed3"
(openpgp-fingerprint
"3CE464558A84FDC69DB40CFB090B11993D9AEBB5"))))
(channel
(name 'guile)
(url "https://git.savannah.gnu.org/git/guile.git")
(branch "main")))
%default-channels)
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Thu, 03 Oct 2024 13:24:01 GMT) Full text and rfc822 format available.Message #59 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> To: Ludovic Courtès <ludo <at> gnu.org> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Nigko Yerden <nigko.yerden <at> gmail.com>, Christopher Baines <guix <at> cbaines.net>, Attila Lendvai <attila <at> lendvai.name>, 72867 <at> debbugs.gnu.org Subject: Re: [bug#72867] [PATCH v6] gexp: Make 'local-file' follow symlinks. Date: Thu, 03 Oct 2024 15:22:56 +0200
Hello Ludo, in my opinion,
Ludovic Courtès <ludo <at> gnu.org> writes:
> To me, it’s not unreasonable for (local-file "content") to fail when
> loading ‘dir/test-local-file.scm’. I would say that this is what most
> people would expect.
Yes, the present situation is that from the real file and the symlink,
one of them will not run.
Note that the test here is the converse situation of the guile channel
[1] described in the cookbook. There, the symlink is in the outer
directory and the real file in the inner directory, which refers to
(source (local-file "../.." "guile-checkout"
Regards,
Florian
guix-patches <at> gnu.org:bug#72867; Package guix-patches.
(Sun, 06 Oct 2024 07:10:01 GMT) Full text and rfc822 format available.Message #62 received at 72867 <at> debbugs.gnu.org (full text, mbox):
From: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de> To: Ludovic Courtès <ludo <at> gnu.org> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Nigko Yerden <nigko.yerden <at> gmail.com>, Christopher Baines <guix <at> cbaines.net>, Attila Lendvai <attila <at> lendvai.name>, 72867 <at> debbugs.gnu.org Subject: Re: [bug#72867] [PATCH v6] gexp: Make 'local-file' follow symlinks. Date: Sun, 06 Oct 2024 09:09:00 +0200
My reply was lacking. I accidentally deleted footnote [1] https://git.savannah.gnu.org/cgit/guile.git/tree/ Ludovic Courtès <ludo <at> gnu.org> writes: >> --- a/guix/utils.scm >> +++ b/guix/utils.scm >> @@ -1121,11 +1121,7 @@ (define absolute-dirname >> (match (search-path %load-path file) >> (#f #f) >> ((? string? file) >> - ;; If there are relative names in %LOAD-PATH, FILE can be relative and >> - ;; needs to be canonicalized. >> - (if (string-prefix? "/" file) >> - (dirname file) >> - (canonicalize-path (dirname file))))))) >> + (dirname (canonicalize-path file)))))) > > Am I right that we cannot keep the ‘if’ here, as it would perform > “lexical” dot-dot resolution instead of Unix resolution (accounting for > symlinks), right? Yes, exactly. Does not the mlambda Ludo had put in absolute-dirname resolve all canonicalize-path concerns? There are many patches, but all have the same file. Regards, Florian
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.