[bug#50960,04/10] DRAFT shell: By default load the local 'guix.scm' or 'manifest.scm' file.

DRAFT: Add doc.

* guix/scripts/shell.scm (parse-args): Add call to 'auto-detect-manifest'.
(find-file-in-parent-directories, auto-detect-manifest): New procedures.
* tests/guix-shell.sh: Add test.
---
 guix/scripts/shell.scm | 44 ++++++++++++++++++++++++++++++++++++++++--
 tests/guix-shell.sh    | 16 +++++++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)

Hi,

Am Samstag, den 02.10.2021, 12:22 +0200 schrieb Ludovic Courtès:
> [...]
> +(define (auto-detect-manifest opts)
> +  "If OPTS do not specify packages or a manifest, load a
> \"guix.scm\" or
> +\"manifest.scm\" file from the current directory or one of its
> ancestors.
> +Return the modified OPTS."
> +  (define (options-contain-payload? opts)
> +    (match opts
> +      (() #f)
> +      ((('package . _) . _) #t)
> +      ((('load . _) . _) #t)
> +      ((('manifest . _) . _) #t)
> +      ((('expression . _) . _) #t)
> +      ((_ . rest) (options-contain-payload? rest))))
> +
> +  (if (options-contain-payload? opts)
> +      opts
> +      (match (find-file-in-parent-directories '("guix.scm"
> "manifest.scm"))
> +        (#f
> +         (warning (G_ "no packages specified; creating an empty
> environment~%"))
> +         opts)
> +        (file
> +         (info (G_ "loading environment from '~a'...~%") file)
> +         (match (basename file)
> +           ("guix.scm"
> +            (alist-cons 'load `(package ,file) opts))
> +           ("manifest.scm"
> +            (alist-cons 'manifest file opts)))))))
> [...]
What would happen on the top-level of the Guix source tree or deep
inside the tree of a guile package that deals with manifests, that
aren't necessarily related to Guix?  I think we should try searching
for something less ambiguous first (".guix-shell/manifest" perhaps?)
and maybe also provide further options after manifest.scm (e.g. build-
aux/guix.scm or etc/guix.scm)

WDYT?

Hi,

Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:

> Am Samstag, den 02.10.2021, 12:22 +0200 schrieb Ludovic Courtès:
>> [...]
>> +(define (auto-detect-manifest opts)
>> +  "If OPTS do not specify packages or a manifest, load a
>> \"guix.scm\" or
>> +\"manifest.scm\" file from the current directory or one of its
>> ancestors.
>> +Return the modified OPTS."
>> +  (define (options-contain-payload? opts)
>> +    (match opts
>> +      (() #f)
>> +      ((('package . _) . _) #t)
>> +      ((('load . _) . _) #t)
>> +      ((('manifest . _) . _) #t)
>> +      ((('expression . _) . _) #t)
>> +      ((_ . rest) (options-contain-payload? rest))))
>> +
>> +  (if (options-contain-payload? opts)
>> +      opts
>> +      (match (find-file-in-parent-directories '("guix.scm"
>> "manifest.scm"))
>> +        (#f
>> +         (warning (G_ "no packages specified; creating an empty
>> environment~%"))
>> +         opts)
>> +        (file
>> +         (info (G_ "loading environment from '~a'...~%") file)
>> +         (match (basename file)
>> +           ("guix.scm"
>> +            (alist-cons 'load `(package ,file) opts))
>> +           ("manifest.scm"
>> +            (alist-cons 'manifest file opts)))))))
>> [...]
> What would happen on the top-level of the Guix source tree or deep
> inside the tree of a guile package that deals with manifests, that
> aren't necessarily related to Guix?

You mean a directory that contains a file named ‘guix.scm’ or
‘manifest.scm’ but that happens to do something completely unrelated?

We can never rule this out, but I’d say it’s unlikely (these two
conventions are rather well established) and it’s up to the user to pay
attention.

WDYT?

Thanks,
Ludo’.

Ludovic Courtès schreef op za 02-10-2021 om 12:22 [+0200]:
> +(define (find-file-in-parent-directories candidates)
> +  "Find one of CANDIDATES in the current directory or one of its ancestors."
> +  (let loop ((directory (getcwd)))
> +    (and (= (stat:uid (stat directory)) (getuid))
> +         (or (any (lambda (candidate)
> +                    (let ((candidate (string-append directory "/" candidate)))
> +                      (and (file-exists? candidate) candidate)))
> +                  candidates)
> +             (loop (string-append directory "/..")))))) ;Unix ".." resolution

I do not recommend this.  What would happen if someone creates a temporary directory
"/tmp/stuff" do things in to throw away later (setting permissions appropriately),
tries to create a guix.scm in that directory but misspells it as, say, guix.sm, and runs
"guix shell" from within /tmp/stuff?  Then find-file-in-parent-directories would
load /tmp/guix.scm (possibly created by a local attacker, assuming a multi-user system),
-- if it weren't for the (= (stat:uid (stat directory)) (getuid)).

Because of the (= (stat:uid ...) (getuid)), this attack method is not possible.
However, it causes other issues.  Now it isn't possible for two users (that trust
each other), to set up a directory writable by both (e.g. with ACLs, or by making
the directory group-writable and placing the two users in the same group), for
working together, with a guix.scm usable by both.

These can be two users on the same machine, or remotely via something like NFS,
or a single person having multiple user accounts used for different purposes.

(I once created multiple user accounts on Debian: one regular purpose, one for reading
and games, and one for school, and made the ‘for-reading’ and ‘school’ home directory
readable by the ‘regular-purpose’ account.  It was occasionally useful.)

Greetings,
Maxime.

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op za 02-10-2021 om 12:22 [+0200]:
>> +(define (find-file-in-parent-directories candidates)
>> +  "Find one of CANDIDATES in the current directory or one of its ancestors."
>> +  (let loop ((directory (getcwd)))
>> +    (and (= (stat:uid (stat directory)) (getuid))
>> +         (or (any (lambda (candidate)
>> +                    (let ((candidate (string-append directory "/" candidate)))
>> +                      (and (file-exists? candidate) candidate)))
>> +                  candidates)
>> +             (loop (string-append directory "/..")))))) ;Unix ".." resolution
>
> I do not recommend this.  What would happen if someone creates a temporary directory
> "/tmp/stuff" do things in to throw away later (setting permissions appropriately),
> tries to create a guix.scm in that directory but misspells it as, say, guix.sm, and runs
> "guix shell" from within /tmp/stuff?  Then find-file-in-parent-directories would
> load /tmp/guix.scm (possibly created by a local attacker, assuming a multi-user system),
> -- if it weren't for the (= (stat:uid (stat directory)) (getuid)).
>
> Because of the (= (stat:uid ...) (getuid)), this attack method is not possible.

Right.  :-)

In libgit2, ‘find_repo’ (called by ‘git_repository_discover’) stops at
device boundaries, which is wise.  But it doesn’t stop when the parent
has a different owner (!).

Unlike the code above, it does lexical “..” resolution after first
calling realpath(3) on the directory name; not sure what to think about
this.  (The code of Git itself is harder to read for me.)

> However, it causes other issues.  Now it isn't possible for two users (that trust
> each other), to set up a directory writable by both (e.g. with ACLs, or by making
> the directory group-writable and placing the two users in the same group), for
> working together, with a guix.scm usable by both.
>
> These can be two users on the same machine, or remotely via something like NFS,
> or a single person having multiple user accounts used for different purposes.

Well, sure, but that’s a very uncommon scenario, isn’t it?

I was actually hesitant about this find-in-parent behavior.  I find it
convenient that ‘git’ does that, for instance, so I thought it might be
nice as well.

Thoughts?

Ludo’.

Ludovic Courtès schreef op za 02-10-2021 om 15:43 [+0200]:
> Hi,
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:
> 
> > Am Samstag, den 02.10.2021, 12:22 +0200 schrieb Ludovic Courtès:
> > > [...]
> > > +(define (auto-detect-manifest opts)
> > > +  "If OPTS do not specify packages or a manifest, load a
> > > \"guix.scm\" or
> > > +\"manifest.scm\" file from the current directory or one of its
> > > ancestors.
> > > +Return the modified OPTS."
> > > +  (define (options-contain-payload? opts)
> > > +    (match opts
> > > +      (() #f)
> > > +      ((('package . _) . _) #t)
> > > +      ((('load . _) . _) #t)
> > > +      ((('manifest . _) . _) #t)
> > > +      ((('expression . _) . _) #t)
> > > +      ((_ . rest) (options-contain-payload? rest))))
> > > +
> > > +  (if (options-contain-payload? opts)
> > > +      opts
> > > +      (match (find-file-in-parent-directories '("guix.scm"
> > > "manifest.scm"))
> > > +        (#f
> > > +         (warning (G_ "no packages specified; creating an empty
> > > environment~%"))
> > > +         opts)
> > > +        (file
> > > +         (info (G_ "loading environment from '~a'...~%") file)
> > > +         (match (basename file)
> > > +           ("guix.scm"
> > > +            (alist-cons 'load `(package ,file) opts))
> > > +           ("manifest.scm"
> > > +            (alist-cons 'manifest file opts)))))))
> > > [...]
> > What would happen on the top-level of the Guix source tree or deep
> > inside the tree of a guile package that deals with manifests, that
> > aren't necessarily related to Guix?
> 
> You mean a directory that contains a file named ‘guix.scm’ or
> ‘manifest.scm’ but that happens to do something completely unrelated?
> 
> We can never rule this out, but I’d say it’s unlikely (these two
> conventions are rather well established) and it’s up to the user to pay
> attention.
> 
> WDYT?

Guix itself doesn't follow this convention: the guix source tree has an unrelated
"guix.scm" file, that doesn't evaluate to a package.  I'd expect that running
"guix shell" within the guix source tree would be equivalent to
"guix environment guix", but currently this doesn't seem to be the case.

Greetings,
Maxime.

Ludovic Courtès schreef op za 02-10-2021 om 12:22 [+0200]:
> DRAFT: Add doc.
> 
> * guix/scripts/shell.scm (parse-args): Add call to 'auto-detect-manifest'.
> (find-file-in-parent-directories, auto-detect-manifest): New procedures.
> * tests/guix-shell.sh: Add test.
> ---
>  guix/scripts/shell.scm | 44 ++++++++++++++++++++++++++++++++++++++++--
>  tests/guix-shell.sh    | 16 +++++++++++++++
>  2 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/guix/scripts/shell.scm b/guix/scripts/shell.scm
> index 6a4b7a5092..2f15befbd3 100644
> --- a/guix/scripts/shell.scm
> +++ b/guix/scripts/shell.scm
> @@ -22,6 +22,8 @@
>    #:autoload   (guix scripts build) (show-build-options-help)
>    #:autoload   (guix transformations) (show-transformation-options-help)
>    #:use-module (guix scripts)
> +  #:use-module (guix packages)
> +  #:use-module (guix profiles)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-26)
>    #:use-module (srfi srfi-37)
> @@ -121,13 +123,51 @@ interactive shell in that environment.\n"))
>    ;; The '--' token is used to separate the command to run from the rest of
>    ;; the operands.
>    (let ((args command (break (cut string=? "--" <>) args)))
> -    (let ((opts (parse-command-line args %options (list %default-options)
> -                                    #:argument-handler handle-argument)))
> +    (let ((opts (auto-detect-manifest
> +                 (parse-command-line args %options (list %default-options)
> +                                     #:argument-handler handle-argument))))
>        (match command
>          (() opts)
>          (("--") opts)
>          (("--" command ...) (alist-cons 'exec command opts))))))
>  
> +(define (find-file-in-parent-directories candidates)
> +  "Find one of CANDIDATES in the current directory or one of its ancestors."
> +  (let loop ((directory (getcwd)))
> +    (and (= (stat:uid (stat directory)) (getuid))
> +         (or (any (lambda (candidate)
> +                    (let ((candidate (string-append directory "/" candidate)))
> +                      (and (file-exists? candidate) candidate)))
> +                  candidates)
> +             (loop (string-append directory "/..")))))) ;Unix ".." resolution
> +
> +(define (auto-detect-manifest opts)
> +  "If OPTS do not specify packages or a manifest, load a \"guix.scm\" or
> +\"manifest.scm\" file from the current directory or one of its ancestors.
> +Return the modified OPTS."
> +  (define (options-contain-payload? opts)
> +    (match opts
> +      (() #f)
> +      ((('package . _) . _) #t)
> +      ((('load . _) . _) #t)
> +      ((('manifest . _) . _) #t)
> +      ((('expression . _) . _) #t)
> +      ((_ . rest) (options-contain-payload? rest))))
> +
> +  (if (options-contain-payload? opts)
> +      opts
> +      (match (find-file-in-parent-directories '("guix.scm" "manifest.scm"))
> +        (#f
> +         (warning (G_ "no packages specified; creating an empty environment~%"))
> +         opts)
> +        (file
> +         (info (G_ "loading environment from '~a'...~%") file)

Could we have nice ‘curly quotes’ here:

  (info (G_ "loading environment from ‘~a’...~%") file)

Greetings,
Maxime.

Maxime Devos <maximedevos@telenet.be> skribis:

> Guix itself doesn't follow this convention: the guix source tree has an unrelated
> "guix.scm" file, that doesn't evaluate to a package.

Indeed, but that’s because Guix predates the convention.  :-)

> I'd expect that running "guix shell" within the guix source tree would
> be equivalent to "guix environment guix", but currently this doesn't
> seem to be the case.

Right, it would fail poorly in this particular case.

Ludo’.