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