[bug#58812,4/5] guix: shell: Add '--symlink' option.

* guix/scripts/pack.scm (%options): Extract symlink parsing logic to...
(symlink-spec-option-parser): ... here.
(self-contained-tarball/builder): Extract symlink->directives logic to...
* gnu/build/install.scm (make-symlink->directives): ... here.  Add a comment
mentioning why a relative file name is used for the link target.
* guix/scripts/environment.scm (show-environment-options-help): Document new
--symlink option.
(%default-options): Add default value for symlinks.
(%options): Register new symlink option.
(launch-environment/container): Add #:symlinks argument and extend doc.
Create symlinks using evaluate-populate-directive and
make-symlink->directives.
(guix-environment*): Pass symlinks arguments to launch-environment/container.
* doc/guix.texi (Invoking guix shell): Document it.
* tests/guix-shell.sh: Test it.
---
 doc/guix.texi                |  9 +++++-
 gnu/build/install.scm        | 18 ++++++++++++
 guix/scripts/environment.scm | 38 +++++++++++++++++-------
 guix/scripts/pack.scm        | 57 +++++++++++++++---------------------
 tests/guix-shell.sh          | 17 +++++++++++
 5 files changed, 94 insertions(+), 45 deletions(-)

Hi,

That looks like a useful improvement!  Some comments below.

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> +@item --symlink=@var{spec}
> +@itemx -S @var{spec}
> +For containers, create the symbolic links specified by @var{spec}, as
> +documented in @ref{pack-symlink-option}.

We should refrain from using @ref in sentences (info "(texinfo) @ref").
Instead, I’d write:

  documented for @command{guix pack} (@pxref{pack-symlink-option}).

>  (define-module (gnu build install)
> +  #:use-module ((guix build union) #:select (relative-file-name))
>    #:use-module (guix build syscalls)
>    #:use-module (guix build utils)
>    #:use-module (guix build store-copy)
> @@ -26,6 +27,7 @@ (define-module (gnu build install)
>    #:use-module (ice-9 match)
>    #:export (install-boot-config
>              evaluate-populate-directive
> +            make-symlink->directives
>              populate-root-file-system
>              install-database-and-gc-roots
>              populate-single-profile-directory
> @@ -124,6 +126,22 @@ (define target* (if (string-suffix? "/" target)
>                  directive)
>          (apply throw args)))))
>  
> +(define (make-symlink->directives directory)
> +  "Return a procedure that turn symlinks specs into directives that target
> +DIRECTORY."
> +  (match-lambda
> +    ((source '-> target)
> +     (let ((target (string-append directory "/" target))
> +           (parent (dirname source)))
> +       ;; Never add a 'directory' directive for "/" so as to preserve its
> +       ;; ownership and avoid adding the same entries multiple times.
> +       `(,@(if (string=? parent "/")
> +               '()
> +               `((directory ,parent)))
> +         ;; Note: a relative file name is used for compatibility with
> +         ;; relocatable packs.
> +         (,source -> ,(relative-file-name parent target)))))))

I think it’s a case where I would refrain from factorizing because this
procedure, as shown by the comments and the use of ‘relative-file-name’,
is specifically tailored for the needs to ‘guix pack -f tarball’.

I’d prefer to have a similar but independently maintained variant of
this procedure in (guix scripts environment) to avoid difficulties down
the road.

> +++ b/guix/scripts/environment.scm
> @@ -33,8 +33,10 @@ (define-module (guix scripts environment)
>    #:use-module ((guix gexp) #:select (lower-object))
>    #:use-module (guix scripts)
>    #:use-module (guix scripts build)
> +  #:use-module ((guix scripts pack) #:select (symlink-spec-option-parser))

You can turn this into #:autoload so we don’t pay the price when not
using ‘--symlink’.

> +++ b/tests/guix-shell.sh
> @@ -20,6 +20,8 @@
>  # Test the 'guix shell' alias.
>  #
>  
> +. tests/utils.sh
> +
>  guix shell --version
>  
>  configdir="t-guix-shell-config-$$"
> @@ -32,6 +34,21 @@ export XDG_CONFIG_HOME
>  
>  guix shell --bootstrap --pure guile-bootstrap -- guile --version
>  
> +# '--symlink' can only be used with --container.
> +! guix shell --bootstrap guile-bootstrap -S /dummy=bin/guile
> +
> +if has_container_support; then
> +    # '--symlink' works.
> +    echo "TESTING SYMLINK IN CONTAINER"
> +    guix shell --bootstrap guile-bootstrap --container \
> +         --symlink=/usr/bin/guile=bin/guile -- \
> +         /usr/bin/guile --version

This should go to ‘tests/guix-environment-container.sh’, which has all
the container-related tests.

Thanks,
Ludo’.

Hi Ludo!

Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> That looks like a useful improvement!  Some comments below.

Thanks!

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> +@item --symlink=@var{spec}
>> +@itemx -S @var{spec}
>> +For containers, create the symbolic links specified by @var{spec}, as
>> +documented in @ref{pack-symlink-option}.
>
> We should refrain from using @ref in sentences (info "(texinfo) @ref").
> Instead, I’d write:
>
>   documented for @command{guix pack} (@pxref{pack-symlink-option}).

I've heard that from you before, but is there a reason against?  I like
to know the rationale for doing things a certain way, lest I forget :-).
From info '(texinfo) @ref':

--8<---------------cut here---------------start------------->8---
6.6 '@ref'
==========

'@ref' is nearly the same as '@xref' except that it does not generate a
'See' in the printed output, just the reference itself.  This makes it
useful as the last part of a sentence.

For example,

     For more information, @pxref{This}, and @ref{That}.

produces in Info:

     For more information, *note This::, and *note That::.
--8<---------------cut here---------------end--------------->8---

>>  (define-module (gnu build install)
>> +  #:use-module ((guix build union) #:select (relative-file-name))
>>    #:use-module (guix build syscalls)
>>    #:use-module (guix build utils)
>>    #:use-module (guix build store-copy)
>> @@ -26,6 +27,7 @@ (define-module (gnu build install)
>>    #:use-module (ice-9 match)
>>    #:export (install-boot-config
>>              evaluate-populate-directive
>> +            make-symlink->directives
>>              populate-root-file-system
>>              install-database-and-gc-roots
>>              populate-single-profile-directory
>> @@ -124,6 +126,22 @@ (define target* (if (string-suffix? "/" target)
>>                  directive)
>>          (apply throw args)))))
>>
>> +(define (make-symlink->directives directory)
>> +  "Return a procedure that turn symlinks specs into directives that target
>> +DIRECTORY."
>> +  (match-lambda
>> +    ((source '-> target)
>> +     (let ((target (string-append directory "/" target))
>> +           (parent (dirname source)))
>> +       ;; Never add a 'directory' directive for "/" so as to preserve its
>> +       ;; ownership and avoid adding the same entries multiple times.
>> +       `(,@(if (string=? parent "/")
>> +               '()
>> +               `((directory ,parent)))
>> +         ;; Note: a relative file name is used for compatibility with
>> +         ;; relocatable packs.
>> +         (,source -> ,(relative-file-name parent target)))))))
>
> I think it’s a case where I would refrain from factorizing because this
> procedure, as shown by the comments and the use of ‘relative-file-name’,
> is specifically tailored for the needs to ‘guix pack -f tarball’.
>
> I’d prefer to have a similar but independently maintained variant of
> this procedure in (guix scripts environment) to avoid difficulties down
> the road.

I considered to duplicate it, but I opted to reuse it in the end because
I care that the behavior is exactly the same between the two actions
(guix shell --symlink vs guix pack --symlink).  If the way we handle
this is to be changed in the future, I'd want both to be changed at
once, so they remain consistent.  Does this make sense?

>> +++ b/guix/scripts/environment.scm
>> @@ -33,8 +33,10 @@ (define-module (guix scripts environment)
>>    #:use-module ((guix gexp) #:select (lower-object))
>>    #:use-module (guix scripts)
>>    #:use-module (guix scripts build)
>> +  #:use-module ((guix scripts pack) #:select (symlink-spec-option-parser))
>
> You can turn this into #:autoload so we don’t pay the price when not
> using ‘--symlink’.

Done!  Could Guile simply always use lazy loading (autoload by default)?
Otherwise, when is it OK to use autoload and when is it not?

>> +++ b/tests/guix-shell.sh
>> @@ -20,6 +20,8 @@
>>  # Test the 'guix shell' alias.
>>  #
>>
>> +. tests/utils.sh
>> +
>>  guix shell --version
>>
>>  configdir="t-guix-shell-config-$$"
>> @@ -32,6 +34,21 @@ export XDG_CONFIG_HOME
>>
>>  guix shell --bootstrap --pure guile-bootstrap -- guile --version
>>
>> +# '--symlink' can only be used with --container.
>> +! guix shell --bootstrap guile-bootstrap -S /dummy=bin/guile
>> +
>> +if has_container_support; then
>> +    # '--symlink' works.
>> +    echo "TESTING SYMLINK IN CONTAINER"
>> +    guix shell --bootstrap guile-bootstrap --container \
>> +         --symlink=/usr/bin/guile=bin/guile -- \
>> +         /usr/bin/guile --version
>
> This should go to ‘tests/guix-environment-container.sh’, which has all
> the container-related tests.

Done, for the "has_container_support" conditional tests.

Thanks for taking a peek!

Maxim

Hi Maxim!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>> We should refrain from using @ref in sentences (info "(texinfo) @ref").
>> Instead, I’d write:
>>
>>   documented for @command{guix pack} (@pxref{pack-symlink-option}).
>
> I've heard that from you before, but is there a reason against?  I like
> to know the rationale for doing things a certain way, lest I forget :-).
> From info '(texinfo) @ref':

It’s right below the bit you quoted:

     The '@ref' command can tempt writers to express themselves in a
  manner that is suitable for a printed manual but looks awkward in the
  Info format.  Bear in mind that your audience could be using both the
  printed and the Info format.  For example: […]

>>> +(define (make-symlink->directives directory)
>>> +  "Return a procedure that turn symlinks specs into directives that target
>>> +DIRECTORY."
>>> +  (match-lambda
>>> +    ((source '-> target)
>>> +     (let ((target (string-append directory "/" target))
>>> +           (parent (dirname source)))
>>> +       ;; Never add a 'directory' directive for "/" so as to preserve its
>>> +       ;; ownership and avoid adding the same entries multiple times.
>>> +       `(,@(if (string=? parent "/")
>>> +               '()
>>> +               `((directory ,parent)))
>>> +         ;; Note: a relative file name is used for compatibility with
>>> +         ;; relocatable packs.
>>> +         (,source -> ,(relative-file-name parent target)))))))
>>
>> I think it’s a case where I would refrain from factorizing because this
>> procedure, as shown by the comments and the use of ‘relative-file-name’,
>> is specifically tailored for the needs to ‘guix pack -f tarball’.
>>
>> I’d prefer to have a similar but independently maintained variant of
>> this procedure in (guix scripts environment) to avoid difficulties down
>> the road.
>
> I considered to duplicate it, but I opted to reuse it in the end because
> I care that the behavior is exactly the same between the two actions
> (guix shell --symlink vs guix pack --symlink).  If the way we handle
> this is to be changed in the future, I'd want both to be changed at
> once, so they remain consistent.  Does this make sense?

They don’t have to be consistent.  Use of ‘relative-file-name’ here for
example is dictated by the needs of relocatable packs.  It doesn’t have
to be this way here.

I think it’s best to keep separate copies here (they likely won’t be
exactly the same).

>>> +++ b/guix/scripts/environment.scm
>>> @@ -33,8 +33,10 @@ (define-module (guix scripts environment)
>>>    #:use-module ((guix gexp) #:select (lower-object))
>>>    #:use-module (guix scripts)
>>>    #:use-module (guix scripts build)
>>> +  #:use-module ((guix scripts pack) #:select (symlink-spec-option-parser))
>>
>> You can turn this into #:autoload so we don’t pay the price when not
>> using ‘--symlink’.
>
> Done!  Could Guile simply always use lazy loading (autoload by default)?

#:select could be synonymous with #:autoload, if that’s what you mean,
but in general Guile cannot know whether autoloading is semantically
equivalent to eagerly loading: there might be side-effects happening
when the top-level of the module runs.

> Otherwise, when is it OK to use autoload and when is it not?

#:autoload exists as a way to amortize initialization costs and make
sure only necessary functionality gets loaded, thereby reducing CPU and
memory usage.

Only the module user can tell whether #:autoload is appropriate.  In
general you’d use it for optional functionality that has a
non-negligible memory footprint or that would noticeably degrade startup
time.

Ludo’.

Hi Ludo!

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Maxim!
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>> We should refrain from using @ref in sentences (info "(texinfo) @ref").
>>> Instead, I’d write:
>>>
>>>   documented for @command{guix pack} (@pxref{pack-symlink-option}).
>>
>> I've heard that from you before, but is there a reason against?  I like
>> to know the rationale for doing things a certain way, lest I forget :-).
>> From info '(texinfo) @ref':
>
> It’s right below the bit you quoted:
>
>      The '@ref' command can tempt writers to express themselves in a
>   manner that is suitable for a printed manual but looks awkward in the
>   Info format.  Bear in mind that your audience could be using both the
>   printed and the Info format.  For example: […]

Yes, and I don't get it :-)

--8<---------------cut here---------------start------------->8---
   The '@ref' command can tempt writers to express themselves in a
manner that is suitable for a printed manual but looks awkward in the
Info format.  Bear in mind that your audience could be using both the
printed and the Info format.  For example:

     Sea surges are described in @ref{Hurricanes}.

looks ok in the printed output:

     Sea surges are described in Section 6.7 [Hurricanes], page 72.

but is awkward to read in Info, "note" being a verb:

     Sea surges are described in *note Hurricanes::.
--8<---------------cut here---------------end--------------->8---

I don't see a "note" in the final sentence that should make it awkward?
It's lacking a "see " prefix though, which could help to make things a
bit clearer, I guess.

It looks the same in info as in the pxref example given above:

--8<---------------cut here---------------start------------->8---
For example,

     For more information, @pxref{This}, and @ref{That}.

produces in Info:

     For more information, *note This::, and *note That::.
--8<---------------cut here---------------end--------------->8---

I'm also unsure where the "see" comes before That:: above.  Is it a
mistake in the manual?

>>>> +(define (make-symlink->directives directory)
>>>> +  "Return a procedure that turn symlinks specs into directives that target
>>>> +DIRECTORY."
>>>> +  (match-lambda
>>>> +    ((source '-> target)
>>>> +     (let ((target (string-append directory "/" target))
>>>> +           (parent (dirname source)))
>>>> +       ;; Never add a 'directory' directive for "/" so as to preserve its
>>>> +       ;; ownership and avoid adding the same entries multiple times.
>>>> +       `(,@(if (string=? parent "/")
>>>> +               '()
>>>> +               `((directory ,parent)))
>>>> +         ;; Note: a relative file name is used for compatibility with
>>>> +         ;; relocatable packs.
>>>> +         (,source -> ,(relative-file-name parent target)))))))
>>>
>>> I think it’s a case where I would refrain from factorizing because this
>>> procedure, as shown by the comments and the use of ‘relative-file-name’,
>>> is specifically tailored for the needs to ‘guix pack -f tarball’.
>>>
>>> I’d prefer to have a similar but independently maintained variant of
>>> this procedure in (guix scripts environment) to avoid difficulties down
>>> the road.
>>
>> I considered to duplicate it, but I opted to reuse it in the end because
>> I care that the behavior is exactly the same between the two actions
>> (guix shell --symlink vs guix pack --symlink).  If the way we handle
>> this is to be changed in the future, I'd want both to be changed at
>> once, so they remain consistent.  Does this make sense?
>
> They don’t have to be consistent.  Use of ‘relative-file-name’ here for
> example is dictated by the needs of relocatable packs.  It doesn’t have
> to be this way here.
>
> I think it’s best to keep separate copies here (they likely won’t be
> exactly the same).

OK, I see you point about relative-file-name not being needed.  I'll make
the change.

>>>> +++ b/guix/scripts/environment.scm
>>>> @@ -33,8 +33,10 @@ (define-module (guix scripts environment)
>>>>    #:use-module ((guix gexp) #:select (lower-object))
>>>>    #:use-module (guix scripts)
>>>>    #:use-module (guix scripts build)
>>>> +  #:use-module ((guix scripts pack) #:select (symlink-spec-option-parser))
>>>
>>> You can turn this into #:autoload so we don’t pay the price when not
>>> using ‘--symlink’.
>>
>> Done!  Could Guile simply always use lazy loading (autoload by default)?
>
> #:select could be synonymous with #:autoload, if that’s what you mean,
> but in general Guile cannot know whether autoloading is semantically
> equivalent to eagerly loading: there might be side-effects happening
> when the top-level of the module runs.

Perhaps there could be a strict execution mode where it is assumed that
side effects are not used when modules run?  That seems a seldom used
feature anyway, and could enable making lazy loading of modules the
default.

>> Otherwise, when is it OK to use autoload and when is it not?
>
> #:autoload exists as a way to amortize initialization costs and make
> sure only necessary functionality gets loaded, thereby reducing CPU and
> memory usage.
>
> Only the module user can tell whether #:autoload is appropriate.  In
> general you’d use it for optional functionality that has a
> non-negligible memory footprint or that would noticeably degrade startup
> time.
>
> Ludo’.

Thank you for the explanations and review!  I'll send a v3 shortly.

--
Maxim

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi Ludo!
>
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi Maxim!
>>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>
>>>> We should refrain from using @ref in sentences (info "(texinfo) @ref").
>>>> Instead, I’d write:
>>>>
>>>>   documented for @command{guix pack} (@pxref{pack-symlink-option}).
>>>
>>> I've heard that from you before, but is there a reason against?  I like
>>> to know the rationale for doing things a certain way, lest I forget :-).
>>> From info '(texinfo) @ref':
>>
>> It’s right below the bit you quoted:
>>
>>      The '@ref' command can tempt writers to express themselves in a
>>   manner that is suitable for a printed manual but looks awkward in the
>>   Info format.  Bear in mind that your audience could be using both the
>>   printed and the Info format.  For example: […]
>
> Yes, and I don't get it :-)

To be more concrete, this is what it looks currently:

--8<---------------cut here---------------start------------->8---
‘--symlink=SPEC’
‘-S SPEC’
     For containers, create the symbolic links specified by SPEC, as
     documented in *note pack-symlink-option::.
--8<---------------cut here---------------end--------------->8---

This is what it'd look if I use (see: @pxref ...) instead:
--8<---------------cut here---------------start------------->8---
‘--symlink=SPEC’
‘-S SPEC’
     For containers, create the symbolic links specified by SPEC (see:
     *note pack-symlink-option::).
--8<---------------cut here---------------end--------------->8---

Contrary to what the Texinfo manual says, pxref seems to be the one
introducing the awkward "*note" verb in the resulting info.

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Contrary to what the Texinfo manual says, pxref seems to be the one
> introducing the awkward "*note" verb in the resulting info.

If you read it in Emacs, it looks a bit different; I think info.el adds
removes “note” and adds “see” in some cases (e.g., see
‘Info-hide-note-references’).

Ludo’.

Hi Maxim,

On Wed, 09 Nov 2022 at 21:58, Ludovic Courtès <ludo@gnu.org> wrote:

>> +@item --symlink=@var{spec}
>> +@itemx -S @var{spec}
>> +For containers, create the symbolic links specified by @var{spec}, as
>> +documented in @ref{pack-symlink-option}.
>
> We should refrain from using @ref in sentences (info "(texinfo) @ref").
> Instead, I’d write:
>
>   documented for @command{guix pack} (@pxref{pack-symlink-option}).

Well, for what it is worth, I have marked this email [1] by Eli
Zaretskii from Emacs.  Somehow, the message provides some rules of thumb
to write Texinfo. :-) Quoting about cross-reference:

     5. Cross-references:

        As a separate sentence: @xref{Node name}, for the details.
        In the middle of a sentence ... see @ref{Node name}, for more.
        In parentheses: Some text (@pxref{Some node}) more text.

1: https://lists.gnu.org/archive/html/emacs-devel/2017-11/msg00525.html


Cheers,
simon

Hi Simon,

zimoun <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Wed, 09 Nov 2022 at 21:58, Ludovic Courtès <ludo@gnu.org> wrote:
>
>>> +@item --symlink=@var{spec}
>>> +@itemx -S @var{spec}
>>> +For containers, create the symbolic links specified by @var{spec}, as
>>> +documented in @ref{pack-symlink-option}.
>>
>> We should refrain from using @ref in sentences (info "(texinfo) @ref").
>> Instead, I’d write:
>>
>>   documented for @command{guix pack} (@pxref{pack-symlink-option}).
>
> Well, for what it is worth, I have marked this email [1] by Eli
> Zaretskii from Emacs.  Somehow, the message provides some rules of thumb
> to write Texinfo. :-) Quoting about cross-reference:
>
>      5. Cross-references:
>
>         As a separate sentence: @xref{Node name}, for the details.
>         In the middle of a sentence ... see @ref{Node name}, for more.
>         In parentheses: Some text (@pxref{Some node}) more text.
>
> 1: https://lists.gnu.org/archive/html/emacs-devel/2017-11/msg00525.html

I like it, it takes the occult out of the equation :-).