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

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

[Nigko Yerden]

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

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?)))))
 
 \f
 ;;;

base-commit: 4c49cd171e2aa06af05cf52403050b18f100867a
-- 
2.45.2

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

[Attila Lendvai]

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'

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

[Ludovic Courtès]

Hi Nigko,

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

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

[pelzflorian (Florian Pelz)]

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

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

[Nigko Yerden]

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

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

[Nigko Yerden]

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'

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

[pelzflorian (Florian Pelz)]

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

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

[Tobias Geerinckx-Rice via Guix-patches via]

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

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

[Nigko Yerden]

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

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?)))))
 
 \f
 ;;;
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

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

[Nigko Yerden]

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

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?))))))
 
 \f
 ;;;
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

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

[pelzflorian (Florian Pelz)]

Hello Nigko.

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>

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

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

[Nigko Yerden]

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

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

[Nigko Yerden]

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@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?))))))
 
 \f
 ;;;
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

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

[Nigko Yerden]

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@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?))))))
 
 \f
 ;;;
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

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

[pelzflorian (Florian Pelz)]

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

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

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

[pelzflorian (Florian Pelz)]

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

Hello Nigko.


[-- Attachment #2: news --]
[-- Type: text/plain, Size: 833 bytes --]

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

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



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

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

[Nigko Yerden via Guix-patches]

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

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

[Ludovic Courtès]

Hi Nigko,

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

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

[pelzflorian (Florian Pelz)]

Hello Ludo, in my opinion,

Ludovic Courtès <ludo@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

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

[pelzflorian (Florian Pelz)]

My reply was lacking.  I accidentally deleted footnote
[1] https://git.savannah.gnu.org/cgit/guile.git/tree/

Ludovic Courtès <ludo@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