[PATCH] gexp: Make 'local-file' follow symlinks.

Open

Details

5 participants

Attila Lendvai
Ludovic Courtès
Tobias Geerinckx-Rice
Nigko Yerden
pelzflorian (Florian Pelz)

Owner: unassigned

Submitted by: Nigko Yerden

Severity: normal

Nigko Yerden wrote on 29 Aug 08:06 +0200

Recipients:(address . guix-patches@gnu.org)(name . Nigko Yerden)(address . nigko.yerden@gmail.com)

Message-ID:e2bf165fc2905bcc8d33d23293eb3d31f3fbe4b8.1724911574.git.nigko.yerden@gmail.com

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@pelzflorian.de> and

Nigko Yerden <nigko.yerden@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(-)

Toggle diff (92 lines)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

Attila Lendvai wrote on 29 Aug 09:01 +0200

when should local-file and current-source-directory not follow symlinks?

Recipients:(name . 72867@debbugs.gnu.org)(address . 72867@debbugs.gnu.org)

Message-ID:TOTHbuZY2D8kbEufD-xK2Gfwm277iOpbXgTLLaG8_SqglJR5FP_H4mPv7huc8iTj9cn6LCGED2AS92L_nsMtzDUUWH7srYQaGCLVL43FizY=@lendvai.name

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'

Ludovic Courtès wrote on 29 Aug 11:00 +0200

Re: [bug#72867] [PATCH] gexp: Make 'local-file' follow symlinks.

Recipients:(name . Nigko Yerden)(address . nigko.yerden@gmail.com)

Message-ID:87frqnhffj.fsf@gnu.org

Hi Nigko,

Nigko Yerden <nigko.yerden@gmail.com> skribis:

Toggle quote (12 lines)> 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@pelzflorian.de> and
> Nigko Yerden <nigko.yerden@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’.

pelzflorian (Florian Pelz) wrote on 29 Aug 12:10 +0200

Recipients:

Message-ID:87v7zjbpxa.fsf@pelzflorian.de

Hello all. Thank you to Nigko for sending the patch.

Nigko Yerden <nigko.yerden@gmail.com> writes:

Toggle quote (4 lines)

> This patch is the result of collective work of

> Florian Pelz <pelzflorian@pelzflorian.de> and

> Nigko Yerden <nigko.yerden@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@gnu.org> writes:

Toggle quote (4 lines)

> 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:

Toggle quote (24 lines)> 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@3.0.99-git
>
> throws an error ("No such file or directory" "GUILE-VERSION"). However,
>
> $ GUILE_LOAD_PATH= guix build guile@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

Nigko Yerden wrote on 30 Aug 14:07 +0200

Recipients:(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)(address . 72867@debbugs.gnu.org)

Message-ID:6650f73f-e3d7-47ed-86df-ad9fa5f8cf7d@gmail.com

Hello Florian,

Toggle quote (1 lines)

> 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.

Toggle quote (3 lines)

> 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:

Toggle quote (48 lines)> Hello all.  Thank you to Nigko for sending the patch.
> 
> Nigko Yerden <nigko.yerden@gmail.com> writes:
>> This patch is the result of collective work of
>> Florian Pelz <pelzflorian@pelzflorian.de> and
>> Nigko Yerden <nigko.yerden@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@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@3.0.99-git
>>
>> throws an error ("No such file or directory" "GUILE-VERSION"). However,
>>
>> $ GUILE_LOAD_PATH= guix build guile@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

Nigko Yerden wrote on 30 Aug 16:00 +0200

[bug#72867] when should local-file and current-source-directory not follow symlinks?

Recipients:(name . Attila Lendvai)(address . attila@lendvai.name)(address . 72867@debbugs.gnu.org)

Message-ID:486161ee-563b-4de8-8cef-a970f5862fd0@gmail.com

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:

Toggle quote (13 lines)> 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'

pelzflorian (Florian Pelz) wrote on 31 Aug 19:10 +0200

Recipients:(name . Nigko Yerden)(address . nigko.yerden@gmail.com)

Message-ID:87v7zg1uvm.fsf@pelzflorian.de

Nigko Yerden <nigko.yerden@gmail.com> writes:

Toggle quote (9 lines)

> 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

Tobias Geerinckx-Rice wrote on 1 Sep 16:13 +0200

Recipients:(address . 72867@debbugs.gnu.org)

Message-ID:87a5grihr2.fsf@nckx

Hi,

pelzflorian (Florian Pelz) ???

Toggle quote (3 lines)

> 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.

-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCZtR2oQ0cbWVAdG9iaWFz

LmdyAAoJEA2w/4hPVW15Iy8A+gMcdyts/fQfPlp90x7hTWHEnzIlMcFbsOogOsPl

qJTfAP4ruByQ7ynQFtHzFe/f+SBw0iCqsqDEnDAPpx2AGr+ZBA==

=lDYg

-----END PGP SIGNATURE-----

Nigko Yerden wrote on 2 Sep 06:41 +0200

[PATCH v2] gexp: Make 'local-file' follow symlinks.

Recipients:(address . 72867@debbugs.gnu.org)

Message-ID:e857ab34801ae7d1270eff9fe8b6376d039af5d6.1725252117.git.nigko.yerden@gmail.com

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@pelzflorian.de> and

Nigko Yerden <nigko.yerden@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(-)

Toggle diff (126 lines)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

Nigko Yerden wrote on 2 Sep 09:53 +0200

[PATCH v3] gexp: Make 'local-file' follow symlinks.

Recipients:(address . 72867@debbugs.gnu.org)

Message-ID:2d4b593eaea4fed1fb08031a599aaab032442041.1725263612.git.nigko.yerden@gmail.com

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@pelzflorian.de> and

Nigko Yerden <nigko.yerden@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(-)

Toggle diff (127 lines)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

pelzflorian (Florian Pelz) wrote on 3 Sep 17:05 +0200

Recipients:(name . Nigko Yerden)(address . nigko.yerden@gmail.com)(address . 72867@debbugs.gnu.org)

Message-ID:87zfoord4n.fsf@pelzflorian.de

Hello Nigko.

Nigko Yerden <nigko.yerden@gmail.com> writes:

Toggle quote (4 lines)

> This patch is the result of collective work of

> Florian Pelz <pelzflorian@pelzflorian.de> and

> Nigko Yerden <nigko.yerden@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.

Toggle quote (2 lines)

> * 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

Nigko Yerden wrote on 5 Sep 06:16 +0200

[PATCH v4] gexp: Make 'local-file' follow symlinks.

Recipients:(address . 72867@debbugs.gnu.org)

Message-ID:0676272a670d00cacf351da05bf909fec5435bea.1725509811.git.nigko.yerden@gmail.com

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.

* 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:

Toggle quote (8 lines)

>> * 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').

Toggle quote (5 lines)

>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(-)

Toggle diff (137 lines)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

Nigko Yerden wrote on 5 Sep 07:06 +0200

Re: [PATCH v3] gexp: Make 'local-file' follow symlinks.

Recipients:(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)(address . 72867@debbugs.gnu.org)

Message-ID:4be8c4ec-4cc5-4b2e-8485-940b8d0f17f2@gmail.com

Hello Florian,

pelzflorian (Florian Pelz) wrote:

Toggle quote (4 lines)

> 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

Nigko Yerden wrote on 6 Sep 06:17 +0200

[PATCH v5] gexp: Make 'local-file' follow symlinks.

Recipients:(address . 72867@debbugs.gnu.org)

Message-ID:6e87ddd086b9188539eecfc83bdd6712aaf53a1a.1725596262.git.nigko.yerden@gmail.com

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.

* 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(-)

Toggle diff (137 lines)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

pelzflorian (Florian Pelz) wrote on 7 Sep 09:35 +0200

Recipients:(name . Nigko Yerden)(address . nigko.yerden@gmail.com)

Message-ID:87r09vsyp1.fsf@pelzflorian.de

Hello Nigko, Tobias, Ludo, Attila (putting them in Cc again).

Nigko Yerden <nigko.yerden@gmail.com> writes:

Toggle quote (5 lines)

> 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.

Toggle quote (8 lines)

> + (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

Toggle quote (3 lines)

> (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

Your comment

Commenting via the web interface is currently disabled.

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

To respond to this issue using the mumi CLI, first switch to it

mumi current 72867

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date