Subject: Re: bug#58812: [PATCH 0/5] Add --symlink option to 'guix shell'.

From: Ludovic Courtès <[email-masked]>
Date: Wed, 09 Nov 2022 22:06:16 +0100

Maxim Cournoyer <[email-masked]> skribis:

> * gnu/build/install.scm (evaluate-populate-directive): By default, error when
> the target of a symlink doesn't exist.  Always ensure TARGET ends with "/".
> (populate-root-file-system): Call evaluate-populate-directive with
>  #:error-on-dangling-symlink #t and add comment.

[...]

> +  (define target* (if (string-suffix? "/" target)
> +                      target
> +                      (string-append target "/")))

Maybe make it:

  (let ((target (if …)))
    …)

so there’s only one ‘target’ in scope (and no ‘target*’); otherwise it’s
easy to forget the ‘*’ and refer to wrong one.

> +           (let ((new* (string-append target* new)))

Likewise.

> +                   (when error-on-dangling-symlink?
> +                     ;; When the symbolic link points to a relative path,
> +                     ;; checking if its target exists must be done relative to
> +                     ;; the link location.
> +                     (with-directory-excursion (if (string-prefix? "/" old)
> +                                                   (getcwd)
> +                                                   (dirname new*)) ;relative
> +                       (unless (file-exists? old)
> +                         (error (format #f "symlink `~a' points to nonexistent \
> +file `~a'" new* old)))))
> +                   (symlink old new*))

I would avoid the directory excursion when unnecessary:

  (unless (if (string-prefix? "/" old)
              (file-exists? old)
              (with-directory-excursion (dirname new)
                (file-exists? old)))
    …)

(We could use ‘lstat’ instead of ‘file-exists?’ if we want to allow
symlinks to dangling symlinks…)

Ludo’.