[bug#34638] [PATCH 0/4] Isolated inferiors.

[bug#34638] [PATCH 0/4] Isolated inferiors.

[Christopher Baines]

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

These patches form a prototype for Guix inferiors, that are
isolated. Access to the inferior Guix is done through running a REPL as
a separate process. These patches provide a way of launching that REPL
in an isolated environment through Linux namespaces, providing some
isolation from the wider system.

These patches should work, at least enough to get the derivations for
packages within the inferior Guix, as well as doing 'guix pull' within
the inferior Guix.

They're not ready to be merged just yet though. I think some of the
approaches are a little odd (e.g. using (ice-9 popen) internals) and
I've got no idea if the isolation is actually working properly.


Christopher Baines (4):
  utils: Add #:base-directory to call-with-temporary-directory.
  linux-container: Add 'start-child-in-container'.
  inferior: Add a shared-directory field to <inferior>
  inferior: Add 'open-inferior/container'.

 gnu/build/linux-container.scm | 82 +++++++++++++++++++++++++++++++
 guix/inferior.scm             | 90 ++++++++++++++++++++++++++++++-----
 guix/utils.scm                |  4 +-
 3 files changed, 163 insertions(+), 13 deletions(-)

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

[bug#34638] [PATCH 1/4] utils: Add #:base-directory to call-with-temporary-directory.

[Christopher Baines]

This allows more easily creating temporary directories within a specific
directory. This is motivated by using this in inferior-eval-with-store.

* guix/utils.scm (call-with-temporary-directory): Add optional keyword
argument, base-directory.
---
 guix/utils.scm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/guix/utils.scm b/guix/utils.scm
index ed1a418cca..abeb156f40 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -620,10 +620,10 @@ call."
         (false-if-exception (close out))
         (false-if-exception (delete-file template))))))
 
-(define (call-with-temporary-directory proc)
+(define* (call-with-temporary-directory proc #:key base-directory)
   "Call PROC with a name of a temporary directory; close the directory and
 delete it when leaving the dynamic extent of this call."
-  (let* ((directory (or (getenv "TMPDIR") "/tmp"))
+  (let* ((directory (or base-directory (getenv "TMPDIR") "/tmp"))
          (template  (string-append directory "/guix-directory.XXXXXX"))
          (tmp-dir   (mkdtemp! template)))
     (dynamic-wind
-- 
2.20.1

[bug#34638] [PATCH 2/4] linux-container: Add 'start-child-in-container'.

[Christopher Baines]

This new procedure is similar to open-pipe* in (ice-9 popen), but using
run-container from (gnu build linux-container).

* gnu/build/linux-container.scm (start-child-in-container): New procedure.
---
 gnu/build/linux-container.scm | 82 +++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/gnu/build/linux-container.scm b/gnu/build/linux-container.scm
index 65e1325577..63c83902e4 100644
--- a/gnu/build/linux-container.scm
+++ b/gnu/build/linux-container.scm
@@ -32,6 +32,7 @@
             setgroups-supported?
             %namespaces
             run-container
+            start-child-in-container
             call-with-container
             container-excursion
             container-excursion*))
@@ -210,6 +211,87 @@ corresponds to the symbols in NAMESPACES."
                ('net  CLONE_NEWNET))
               namespaces)))
 
+(define* (start-child-in-container command
+                                   #:key read? write?
+                                   (root 'temporary)
+                                   (mounts '())
+                                   (namespaces %namespaces)
+                                   (host-uids 1)
+                                   (extra-environment-variables '()))
+  (define (with-root-directory f)
+    (if (eq? root 'temporary)
+        (call-with-temporary-directory f)
+        (f root)))
+
+  ;; (ice-9 popen) internals
+  (define make-rw-port (@@ (ice-9 popen) make-rw-port))
+  (define pipe-guardian (@@ (ice-9 popen) pipe-guardian))
+  (define make-pipe-info (@@ (ice-9 popen) make-pipe-info))
+
+  ;; car is the inport port, cdr is the output port. You write to the output
+  ;; port, and read from the input port.
+  (define child-to-parent-pipe
+    (if read?
+        (pipe)
+        #f))
+
+  (define parent-to-child-pipe
+    (if write?
+        (pipe)
+        #f))
+
+  (define (run-program)
+    (when read?
+      (match child-to-parent-pipe
+        ((input-port . output-port)
+         ;; close the output part of the child-to-parent-pipe, as this is used
+         ;; by the parent process
+         (close-port input-port)
+
+         ;; Make the input part of the child-to-parent-pipe the standard
+         ;; output of this process
+         (dup2 (fileno output-port) 1))))
+
+    (when write?
+      (match parent-to-child-pipe
+        ((input-port . output-port)
+         ;; close the input part of the parent-to-child-pipe, as this is used
+         ;; by the parent processs
+         (close-port output-port)
+
+         ;; Make the output part of the parent-to-child-pipe the standard
+         ;; input of this process
+         (dup2 (fileno input-port) 0))))
+
+    ;; TODO Maybe close all file descriptors, as start_child in Guile does?
+
+    (for-each putenv extra-environment-variables)
+
+    (apply execlp command))
+
+  (with-root-directory
+   (lambda (root)
+     (let ((pid (run-container root mounts namespaces host-uids run-program)))
+       ;; Catch SIGINT and kill the container process.
+       (sigaction SIGINT
+         (lambda (signum)
+           (false-if-exception
+            (kill pid SIGKILL))))
+
+       (let* ((read-port (and=> child-to-parent-pipe car))
+              (write-port (and=> parent-to-child-pipe cdr))
+
+              (port (or (and read-port write-port
+                             (make-rw-port read-port write-port))
+                        read-port
+                        write-port))
+              (pipe-info (make-pipe-info pid)))
+
+         (pipe-guardian pipe-info)
+         (%set-port-property! port 'popen-pipe-info pipe-info)
+
+         port)))))
+
 (define (run-container root mounts namespaces host-uids thunk)
   "Run THUNK in a new container process and return its PID.  ROOT specifies
 the root directory for the container.  MOUNTS is a list of <file-system>
-- 
2.20.1

[bug#34638] [PATCH 2/4] linux-container: Add 'start-child-in-container'.

[Ludovic Courtès]

Hello!

Christopher Baines <mail@cbaines.net> skribis:

> This new procedure is similar to open-pipe* in (ice-9 popen), but using
> run-container from (gnu build linux-container).
>
> * gnu/build/linux-container.scm (start-child-in-container): New procedure.

[...]

 +(define* (start-child-in-container command
> +                                   #:key read? write?
> +                                   (root 'temporary)
> +                                   (mounts '())
> +                                   (namespaces %namespaces)
> +                                   (host-uids 1)
> +                                   (extra-environment-variables '()))

We could even call that ‘open-pipe/container’, for clarity.

> +  (define (with-root-directory f)
> +    (if (eq? root 'temporary)
> +        (call-with-temporary-directory f)
> +        (f root)))
> +
> +  ;; (ice-9 popen) internals
> +  (define make-rw-port (@@ (ice-9 popen) make-rw-port))
> +  (define pipe-guardian (@@ (ice-9 popen) pipe-guardian))
> +  (define make-pipe-info (@@ (ice-9 popen) make-pipe-info))

So this is the funky part.  ;-)

What if we did something like:

  (call-with-container mounts
    (lambda ()
      ;; Somehow act as a proxy between the output process
      ;; and the one spawned by ‘open-pipe*’.
      (open-pipe* …)))

?  Would that work?

That’s create an extra process, but if it works, it’s probably safer and
a lesser maintenance burden.

Now, I think that Guile should expose some of the popen internals
somehow so we can do things like you did, but that’s another story.

Ludo’.

[bug#34638] [PATCH 2/4] linux-container: Add 'start-child-in-container'.

[Christopher Baines]

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


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

> Hello!
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> This new procedure is similar to open-pipe* in (ice-9 popen), but using
>> run-container from (gnu build linux-container).
>>
>> * gnu/build/linux-container.scm (start-child-in-container): New procedure.
>
> [...]
>
>  +(define* (start-child-in-container command
>> +                                   #:key read? write?
>> +                                   (root 'temporary)
>> +                                   (mounts '())
>> +                                   (namespaces %namespaces)
>> +                                   (host-uids 1)
>> +                                   (extra-environment-variables '()))
>
> We could even call that ‘open-pipe/container’, for clarity.

I've made some changes (see below) that move this a little further away
from open-pipe in terms of behaviour now.

>> +  (define (with-root-directory f)
>> +    (if (eq? root 'temporary)
>> +        (call-with-temporary-directory f)
>> +        (f root)))
>> +
>> +  ;; (ice-9 popen) internals
>> +  (define make-rw-port (@@ (ice-9 popen) make-rw-port))
>> +  (define pipe-guardian (@@ (ice-9 popen) pipe-guardian))
>> +  (define make-pipe-info (@@ (ice-9 popen) make-pipe-info))
>
> So this is the funky part.  ;-)
>
> What if we did something like:
>
>   (call-with-container mounts
>     (lambda ()
>       ;; Somehow act as a proxy between the output process
>       ;; and the one spawned by ‘open-pipe*’.
>       (open-pipe* …)))
>
> ?  Would that work?
>
> That’s create an extra process, but if it works, it’s probably safer and
> a lesser maintenance burden.
>
> Now, I think that Guile should expose some of the popen internals
> somehow so we can do things like you did, but that’s another story.

I'm hesitant to try that, as the additional process in the middle seems
a bit awkward to me.

I've made another pass over the code, removed all the uses of (ice-9
popen) internals, and sent another set of patches. For the make-rw-port
function, I just copied that over. The pipe-guardian isn't being used
now, and instead of returning a <pipe-info> record, the port and pid are
returned instead. This works with the inferior use case, as the close
function provided to port->inferior does the right thing, closing the
port and then waiting for the child process to exit, just like popen.

I'm still more interested in getting something working than it being
perfect in any particular way, but let me know what you think.

Thanks,

Chris

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

[bug#34638] [PATCH 3/4] inferior: Add a shared-directory field to <inferior>

[Christopher Baines]

---
 guix/inferior.scm | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/guix/inferior.scm b/guix/inferior.scm
index 027418a98d..cf72454426 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -97,14 +97,15 @@
 
 ;; Inferior Guix process.
 (define-record-type <inferior>
-  (inferior pid socket close version packages table)
+  (inferior pid socket close shared-directory version packages table)
   inferior?
-  (pid      inferior-pid)
-  (socket   inferior-socket)
-  (close    inferior-close-socket)               ;procedure
-  (version  inferior-version)                    ;REPL protocol version
-  (packages inferior-package-promise)            ;promise of inferior packages
-  (table    inferior-package-table))             ;promise of vhash
+  (pid              inferior-pid)
+  (socket           inferior-socket)
+  (close            inferior-close-socket)       ;procedure
+  (shared-directory inferior-shared-directory)
+  (version          inferior-version)            ;REPL protocol version
+  (packages         inferior-package-promise)    ;promise of inferior packages
+  (table            inferior-package-table))     ;promise of vhash
 
 (define (inferior-pipe directory command)
   "Return an input/output pipe on the Guix instance in DIRECTORY.  This runs
@@ -136,7 +137,7 @@ it's an old Guix."
                           ((@ (guix scripts repl) machine-repl))))))
         pipe)))
 
-(define* (port->inferior pipe #:optional (close close-port))
+(define* (port->inferior pipe shared-directory #:optional (close close-port))
   "Given PIPE, an input/output port, return an inferior that talks over PIPE.
 PIPE is closed with CLOSE when 'close-inferior' is called on the returned
 inferior."
@@ -144,7 +145,8 @@ inferior."
 
   (match (read pipe)
     (('repl-version 0 rest ...)
-     (letrec ((result (inferior 'pipe pipe close (cons 0 rest)
+     (letrec ((result (inferior 'pipe pipe close shared-directory
+                                (cons 0 rest)
                                 (delay (%inferior-packages result))
                                 (delay (%inferior-package-table result)))))
        (inferior-eval '(use-modules (guix)) result)
@@ -162,7 +164,7 @@ equivalent.  Return #f if the inferior could not be launched."
   (define pipe
     (inferior-pipe directory command))
 
-  (port->inferior pipe close-pipe))
+  (port->inferior pipe #f close-pipe))
 
 (define (close-inferior inferior)
   "Close INFERIOR."
@@ -479,7 +481,8 @@ thus be the code of a one-argument procedure that accepts a store."
          ((client . address)
           (proxy client (store-connection-socket store))))
        (close-port socket)
-       (read-inferior-response inferior)))))
+       (read-inferior-response inferior)))
+   #:base-directory (inferior-shared-directory inferior)))
 
 (define* (inferior-package-derivation store package
                                       #:optional
-- 
2.20.1

[bug#34638] [PATCH 4/4] inferior: Add 'open-inferior/container'.

[Christopher Baines]

---
 guix/inferior.scm | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/guix/inferior.scm b/guix/inferior.scm
index cf72454426..a5f773c147 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -40,6 +40,9 @@
   #:use-module (guix store)
   #:use-module (guix derivations)
   #:use-module (guix base32)
+  #:use-module (gnu system file-systems)
+  #:use-module (gnu build linux-container)
+  #:use-module (guix build syscalls)
   #:use-module (gcrypt hash)
   #:autoload   (guix cache) (maybe-remove-expired-cache-entries)
   #:autoload   (guix ui) (show-what-to-build*)
@@ -54,6 +57,7 @@
   #:use-module ((rnrs bytevectors) #:select (string->utf8))
   #:export (inferior?
             open-inferior
+            open-inferior/container
             port->inferior
             close-inferior
             inferior-eval
@@ -137,6 +141,67 @@ it's an old Guix."
                           ((@ (guix scripts repl) machine-repl))))))
         pipe)))
 
+(define* (open-inferior/container store guix-store-item
+                                  #:key
+                                  (command "bin/guix")
+                                  (share-host-network? #f)
+                                  (extra-shared-directories '())
+                                  (extra-environment-variables '()))
+  (define requisite-store-items
+    (requisites store (list guix-store-item)))
+
+  (define shared-directory
+    (mkdtemp! (string-append (or (getenv "TMPDIR") "/tmp")
+                             "/guix-inferior.XXXXXX")))
+
+  (define mappings
+    (append
+     (map (lambda (dir)
+            (file-system-mapping
+             (source dir)
+             (target dir)
+             (writable? #f)))
+          `(;; Share a directory, used in inferior-eval-with-store
+            ,shared-directory
+            ,@requisite-store-items
+            ,@extra-shared-directories))
+     (if share-host-network?
+         %network-file-mappings
+         '())))
+
+  (define mounts
+    (append %container-file-systems
+            (map file-system-mapping->bind-mount
+                 mappings)))
+
+  (define (inferior-pipe/container store
+                                   guix-store-item
+                                   shared-directory
+                                   command)
+    (start-child-in-container
+     (list (string-append guix-store-item "/bin/guix")
+           ;; TODO I'm not sure why "repl" is duplicated in the following
+           ;; command
+           "repl" "repl" "-t" "machine")
+     #:read? #t
+     #:write? #t
+     #:mounts mounts
+     #:namespaces (if share-host-network?
+                      (delq 'net %namespaces)
+                      %namespaces)
+     #:extra-environment-variables
+     `(;; Set HOME, so that the (guix profiles) module can be loaded, without it
+       ;; trying to read from /etc/passwd
+       "HOME=/tmp"
+       ,@extra-environment-variables)))
+
+  (port->inferior (inferior-pipe/container store
+                                           guix-store-item
+                                           shared-directory
+                                           command)
+                  shared-directory
+                  close-pipe))
+
 (define* (port->inferior pipe shared-directory #:optional (close close-port))
   "Given PIPE, an input/output port, return an inferior that talks over PIPE.
 PIPE is closed with CLOSE when 'close-inferior' is called on the returned
-- 
2.20.1

[bug#34638] [PATCH 0/4] Isolated inferiors.

[Ludovic Courtès]

Hello!

Christopher Baines <mail@cbaines.net> skribis:

> These patches form a prototype for Guix inferiors, that are
> isolated. Access to the inferior Guix is done through running a REPL as
> a separate process. These patches provide a way of launching that REPL
> in an isolated environment through Linux namespaces, providing some
> isolation from the wider system.
>
> These patches should work, at least enough to get the derivations for
> packages within the inferior Guix, as well as doing 'guix pull' within
> the inferior Guix.

This is really cool.

When we do this kind of thing (like also the “Compute Guix derivation”
trampoline used by ‘guix pull’), it reminds me of what the Nix people
call “recursive Nix”—the ability for a derivation’s build process to
compute other derivation.  If we had that, then basically what you’re
doing might just as well be a derivation.

BTW, thinking about it, for the Guix Data Service, would
‘gexp->derivation-in-inferior’ be of any use?  This is used, for
example, to compute the package cache when running ‘guix pull’.  I
think it’s good enough if all you want is to extract basic file
meta-data, but it’s no good if you also want to extract package
derivations and the likes.

Or we could have a new store back-end that computes derivations in
memory and eventually spits a Nar…

I’m just thinking out loud!

Thanks,
Ludo’.

[bug#34638] [PATCH v2 1/4] utils: Add #:base-directory to call-with-temporary-directory.

[Christopher Baines]

This allows more easily creating temporary directories within a specific
directory. This is motivated by using this in inferior-eval-with-store.

* guix/utils.scm (call-with-temporary-directory): Add optional keyword
argument, base-directory.
---
 guix/utils.scm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/guix/utils.scm b/guix/utils.scm
index ed1a418cca..abeb156f40 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -620,10 +620,10 @@ call."
         (false-if-exception (close out))
         (false-if-exception (delete-file template))))))
 
-(define (call-with-temporary-directory proc)
+(define* (call-with-temporary-directory proc #:key base-directory)
   "Call PROC with a name of a temporary directory; close the directory and
 delete it when leaving the dynamic extent of this call."
-  (let* ((directory (or (getenv "TMPDIR") "/tmp"))
+  (let* ((directory (or base-directory (getenv "TMPDIR") "/tmp"))
          (template  (string-append directory "/guix-directory.XXXXXX"))
          (tmp-dir   (mkdtemp! template)))
     (dynamic-wind
-- 
2.21.0

[bug#34638] [PATCH v2 2/4] linux-container: Add 'start-child-in-container'.

[Christopher Baines]

This new procedure is similar to open-pipe* in (ice-9 popen), but using
run-container from (gnu build linux-container).

* gnu/build/linux-container.scm (start-child-in-container): New procedure.
---
 gnu/build/linux-container.scm | 83 +++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/gnu/build/linux-container.scm b/gnu/build/linux-container.scm
index 3d7b52f098..88b00e00f6 100644
--- a/gnu/build/linux-container.scm
+++ b/gnu/build/linux-container.scm
@@ -32,6 +32,7 @@
             setgroups-supported?
             %namespaces
             run-container
+            start-child-in-container
             call-with-container
             container-excursion
             container-excursion*))
@@ -213,6 +214,88 @@ corresponds to the symbols in NAMESPACES."
                ('net  CLONE_NEWNET))
               namespaces)))
 
+(define* (start-child-in-container command
+                                   #:key read? write?
+                                   (root 'temporary)
+                                   (mounts '())
+                                   (namespaces %namespaces)
+                                   (host-uids 1)
+                                   (extra-environment-variables '()))
+  (define (with-root-directory f)
+    (if (eq? root 'temporary)
+        (call-with-temporary-directory f)
+        (f root)))
+
+  (define (make-rw-port read-port write-port)
+    (make-soft-port
+     (vector
+      (lambda (c) (write-char c write-port))
+      (lambda (s) (display s write-port))
+      (lambda () (force-output write-port))
+      (lambda () (read-char read-port))
+      (lambda () (close-port read-port) (close-port write-port)))
+     "r+"))
+
+  ;; car is the inport port, cdr is the output port. You write to the output
+  ;; port, and read from the input port.
+  (define child-to-parent-pipe
+    (if read?
+        (pipe)
+        #f))
+
+  (define parent-to-child-pipe
+    (if write?
+        (pipe)
+        #f))
+
+  (define (run-program)
+    (when read?
+      (match child-to-parent-pipe
+        ((input-port . output-port)
+         ;; close the output part of the child-to-parent-pipe, as this is used
+         ;; by the parent process
+         (close-port input-port)
+
+         ;; Make the input part of the child-to-parent-pipe the standard
+         ;; output of this process
+         (dup2 (fileno output-port) 1))))
+
+    (when write?
+      (match parent-to-child-pipe
+        ((input-port . output-port)
+         ;; close the input part of the parent-to-child-pipe, as this is used
+         ;; by the parent processs
+         (close-port output-port)
+
+         ;; Make the output part of the parent-to-child-pipe the standard
+         ;; input of this process
+         (dup2 (fileno input-port) 0))))
+
+    ;; TODO Maybe close all file descriptors, as start_child in Guile does?
+
+    (for-each putenv extra-environment-variables)
+
+    (apply execlp command))
+
+  (with-root-directory
+   (lambda (root)
+     (let ((pid (run-container root mounts namespaces host-uids run-program)))
+       ;; Catch SIGINT and kill the container process.
+       (sigaction SIGINT
+         (lambda (signum)
+           (false-if-exception
+            (kill pid SIGKILL))))
+
+       (let* ((read-port (and=> child-to-parent-pipe car))
+              (write-port (and=> parent-to-child-pipe cdr))
+
+              (port (or (and read-port write-port
+                             (make-rw-port read-port write-port))
+                        read-port
+                        write-port)))
+
+         (values port pid))))))
+
 (define* (run-container root mounts namespaces host-uids thunk
                         #:key (guest-uid 0) (guest-gid 0))
   "Run THUNK in a new container process and return its PID.  ROOT specifies
-- 
2.21.0

[bug#34638] [PATCH v2 2/4] linux-container: Add 'start-child-in-container'.

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> skribis:

> This new procedure is similar to open-pipe* in (ice-9 popen), but using
> run-container from (gnu build linux-container).
>
> * gnu/build/linux-container.scm (start-child-in-container): New procedure.

[...]

> +(define* (start-child-in-container command
> +                                   #:key read? write?
> +                                   (root 'temporary)
> +                                   (mounts '())
> +                                   (namespaces %namespaces)
> +                                   (host-uids 1)
> +                                   (extra-environment-variables '()))

Please add a docstring.  :-)

I’d change (extra-environment-variables '()) to:

  (environment-variables (environ))

I always find it too hard to reason about “extra” thing; it’s just more
convenient as an interface to specify the whole thing rather than a list
of “extras”.

> +    (apply execlp command))

To provide a correct argv[0] by default, you should probably change it
to:

  (match command
    ((program arguments ...)
     (execlp program program arguments)))

(That’ll also address a comment of yours in one of the subsequent
patches.)

Could you add a test to ‘tests/containers.scm’?

Thanks,
Ludo’.

[bug#34638] [PATCH v2 2/4] linux-container: Add 'start-child-in-container'.

[Christopher Baines]

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


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

> Christopher Baines <mail@cbaines.net> skribis:
>
>> This new procedure is similar to open-pipe* in (ice-9 popen), but using
>> run-container from (gnu build linux-container).
>>
>> * gnu/build/linux-container.scm (start-child-in-container): New procedure.
>
> [...]
>
>> +(define* (start-child-in-container command
>> +                                   #:key read? write?
>> +                                   (root 'temporary)
>> +                                   (mounts '())
>> +                                   (namespaces %namespaces)
>> +                                   (host-uids 1)
>> +                                   (extra-environment-variables '()))
>
> Please add a docstring.  :-)
>
> I’d change (extra-environment-variables '()) to:
>
>   (environment-variables (environ))
>
> I always find it too hard to reason about “extra” thing; it’s just more
> convenient as an interface to specify the whole thing rather than a list
> of “extras”.

I had a go at this, but I think trying to copy the environment variables
from the host Guix to the inferior one caused problems, at least this
backtrace appears when calling open-inferior/container and I'm guessing
it comes from the inferior guix.

I think calling it environment-variables and having it be '() is OK, the
only change I can see being made elsewhere is that
open-inferior/container adds HOME=/tmp, and that's just to avoid issues
with (guix profiles).

Does that make sense?


Backtrace:
           6 (apply-smob/1 #<catch-closure 7f77e0a889a0>)
In ice-9/boot-9.scm:
    705:2  5 (call-with-prompt ("prompt") #<procedure 7f77e0a9f560 at ice-9/eval.scm:330:13 ()> #<procedure default-prompt-handler (k proc)>)
In ice-9/eval.scm:
    619:8  4 (_ #(#(#<directory (guile-user) 7f77e0717140>)))
   293:34  3 (_ #(#(#<directory (guile-user) 7f77e0717140>) ("/gnu/store/ain1rvg7vrrcr85v0fgpyjm8k2sflxpz-guix-1.0.1-15.0984481/bin/.guix-real" "repl" "-t" "machi?")))
    159:9  2 (_ #(#(#<directory (guile-user) 7f77e0717140>) ("/gnu/store/ain1rvg7vrrcr85v0fgpyjm8k2sflxpz-guix-1.0.1-15.0984481/bin/.guix-real" "repl" "-t" "machi?")))
In ice-9/boot-9.scm:
   2803:6  1 (resolve-interface _ #:select _ #:hide _ #:prefix _ #:renamer _ #:version _)
In unknown file:
           0 (scm-error misc-error #f "~A ~S" ("no code for module" (guix ui)) #f)

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

[bug#34638] [PATCH v2 2/4] linux-container: Add 'start-child-in-container'.

[Ludovic Courtès]

Hi!

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Christopher Baines <mail@cbaines.net> skribis:
>>
>>> This new procedure is similar to open-pipe* in (ice-9 popen), but using
>>> run-container from (gnu build linux-container).
>>>
>>> * gnu/build/linux-container.scm (start-child-in-container): New procedure.
>>
>> [...]
>>
>>> +(define* (start-child-in-container command
>>> +                                   #:key read? write?
>>> +                                   (root 'temporary)
>>> +                                   (mounts '())
>>> +                                   (namespaces %namespaces)
>>> +                                   (host-uids 1)
>>> +                                   (extra-environment-variables '()))
>>
>> Please add a docstring.  :-)
>>
>> I’d change (extra-environment-variables '()) to:
>>
>>   (environment-variables (environ))
>>
>> I always find it too hard to reason about “extra” thing; it’s just more
>> convenient as an interface to specify the whole thing rather than a list
>> of “extras”.
>
> I had a go at this, but I think trying to copy the environment variables
> from the host Guix to the inferior one caused problems, at least this
> backtrace appears when calling open-inferior/container and I'm guessing
> it comes from the inferior guix.
>
> I think calling it environment-variables and having it be '() is OK, the
> only change I can see being made elsewhere is that
> open-inferior/container adds HOME=/tmp, and that's just to avoid issues
> with (guix profiles).
>
> Does that make sense?

Ah yes, defaulting to the empty list is even better.

Thanks,
Ludo’.

[bug#34638] [PATCH v2 3/4] inferior: Add a shared-directory field to <inferior>

[Christopher Baines]

---
 guix/inferior.scm | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/guix/inferior.scm b/guix/inferior.scm
index 63c95141d7..6d18ab90e9 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -97,14 +97,15 @@
 
 ;; Inferior Guix process.
 (define-record-type <inferior>
-  (inferior pid socket close version packages table)
+  (inferior pid socket close shared-directory version packages table)
   inferior?
-  (pid      inferior-pid)
-  (socket   inferior-socket)
-  (close    inferior-close-socket)               ;procedure
-  (version  inferior-version)                    ;REPL protocol version
-  (packages inferior-package-promise)            ;promise of inferior packages
-  (table    inferior-package-table))             ;promise of vhash
+  (pid              inferior-pid)
+  (socket           inferior-socket)
+  (close            inferior-close-socket)       ;procedure
+  (shared-directory inferior-shared-directory)
+  (version          inferior-version)            ;REPL protocol version
+  (packages         inferior-package-promise)    ;promise of inferior packages
+  (table            inferior-package-table))     ;promise of vhash
 
 (define (inferior-pipe directory command)
   "Return an input/output pipe on the Guix instance in DIRECTORY.  This runs
@@ -136,7 +137,7 @@ it's an old Guix."
                           ((@ (guix scripts repl) machine-repl))))))
         pipe)))
 
-(define* (port->inferior pipe #:optional (close close-port))
+(define* (port->inferior pipe shared-directory #:optional (close close-port))
   "Given PIPE, an input/output port, return an inferior that talks over PIPE.
 PIPE is closed with CLOSE when 'close-inferior' is called on the returned
 inferior."
@@ -144,7 +145,8 @@ inferior."
 
   (match (read pipe)
     (('repl-version 0 rest ...)
-     (letrec ((result (inferior 'pipe pipe close (cons 0 rest)
+     (letrec ((result (inferior 'pipe pipe close shared-directory
+                                (cons 0 rest)
                                 (delay (%inferior-packages result))
                                 (delay (%inferior-package-table result)))))
        (inferior-eval '(use-modules (guix)) result)
@@ -162,7 +164,7 @@ equivalent.  Return #f if the inferior could not be launched."
   (define pipe
     (inferior-pipe directory command))
 
-  (port->inferior pipe close-pipe))
+  (port->inferior pipe #f close-pipe))
 
 (define (close-inferior inferior)
   "Close INFERIOR."
@@ -479,7 +481,8 @@ thus be the code of a one-argument procedure that accepts a store."
          ((client . address)
           (proxy client (store-connection-socket store))))
        (close-port socket)
-       (read-inferior-response inferior)))))
+       (read-inferior-response inferior)))
+   #:base-directory (inferior-shared-directory inferior)))
 
 (define* (inferior-package-derivation store package
                                       #:optional
-- 
2.21.0

[bug#34638] [PATCH v2 3/4] inferior: Add a shared-directory field to <inferior>

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> skribis:

> ---
>  guix/inferior.scm | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)

Commit log please.  :-)

> +  (pid              inferior-pid)
> +  (socket           inferior-socket)
> +  (close            inferior-close-socket)       ;procedure
> +  (shared-directory inferior-shared-directory)

Please add a margin comment like “#f | directory”.

> -(define* (port->inferior pipe #:optional (close close-port))
> +(define* (port->inferior pipe shared-directory #:optional (close close-port))
>    "Given PIPE, an input/output port, return an inferior that talks over PIPE.
>  PIPE is closed with CLOSE when 'close-inferior' is called on the returned
>  inferior."

Make ‘shared-directory’ a keyword argument?

(Otherwise there’s a user in (guix ssh) that needs to be updated.)

>           ((client . address)
>            (proxy client (store-connection-socket store))))
>         (close-port socket)
> -       (read-inferior-response inferior)))))
> +       (read-inferior-response inferior)))
> +   #:base-directory (inferior-shared-directory inferior)))

What if ‘inferior-shared-directory’ returns #f?

Otherwise LGTM.

Ludo’.

[bug#34638] [PATCH v2 4/4] inferior: Add 'open-inferior/container'.

[Christopher Baines]

---
 guix/inferior.scm | 76 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/guix/inferior.scm b/guix/inferior.scm
index 6d18ab90e9..8238c7fb38 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -19,6 +19,7 @@
 (define-module (guix inferior)
   #:use-module (srfi srfi-9)
   #:use-module (srfi srfi-9 gnu)
+  #:use-module (srfi srfi-11)
   #:use-module ((guix utils)
                 #:select (%current-system
                           source-properties->location
@@ -40,6 +41,9 @@
   #:use-module (guix store)
   #:use-module (guix derivations)
   #:use-module (guix base32)
+  #:use-module (gnu system file-systems)
+  #:use-module (gnu build linux-container)
+  #:use-module (guix build syscalls)
   #:use-module (gcrypt hash)
   #:autoload   (guix cache) (maybe-remove-expired-cache-entries)
   #:autoload   (guix ui) (show-what-to-build*)
@@ -54,6 +58,7 @@
   #:use-module ((rnrs bytevectors) #:select (string->utf8))
   #:export (inferior?
             open-inferior
+            open-inferior/container
             port->inferior
             close-inferior
             inferior-eval
@@ -137,6 +142,77 @@ it's an old Guix."
                           ((@ (guix scripts repl) machine-repl))))))
         pipe)))
 
+(define* (open-inferior/container store guix-store-item
+                                  #:key
+                                  (command "bin/guix")
+                                  (share-host-network? #f)
+                                  (extra-shared-directories '())
+                                  (extra-environment-variables '()))
+  (define requisite-store-items
+    (requisites store (list guix-store-item)))
+
+  (define shared-directory
+    (mkdtemp! (string-append (or (getenv "TMPDIR") "/tmp")
+                             "/guix-inferior.XXXXXX")))
+
+  (define mappings
+    (append
+     (map (lambda (dir)
+            (file-system-mapping
+             (source dir)
+             (target dir)
+             (writable? #f)))
+          `(;; Share a directory, used in inferior-eval-with-store
+            ,shared-directory
+            ,@requisite-store-items
+            ,@extra-shared-directories))
+     (if share-host-network?
+         %network-file-mappings
+         '())))
+
+  (define mounts
+    (append %container-file-systems
+            (map file-system-mapping->bind-mount
+                 mappings)))
+
+  (define (inferior-pipe/container store
+                                   guix-store-item
+                                   shared-directory
+                                   command)
+    (start-child-in-container
+     (list (string-append guix-store-item "/bin/guix")
+           ;; TODO I'm not sure why "repl" is duplicated in the following
+           ;; command
+           "repl" "repl" "-t" "machine")
+     #:read? #t
+     #:write? #t
+     #:mounts mounts
+     #:namespaces (if share-host-network?
+                      (delq 'net %namespaces)
+                      %namespaces)
+     #:extra-environment-variables
+     `(;; Set HOME, so that the (guix profiles) module can be loaded, without it
+       ;; trying to read from /etc/passwd
+       "HOME=/tmp"
+       ,@extra-environment-variables)))
+
+  (let*-values
+      (((pipe pid)
+        (inferior-pipe/container store
+                                 guix-store-item
+                                 shared-directory
+                                 command))
+       ((close-inferior-pipe)
+        (lambda (pipe*)
+          (unless (eq? pipe pipe*)
+            (error "wrong pipe being closed"))
+          (close-port pipe)
+          (cdr (waitpid pid)))))
+
+      (port->inferior pipe
+                      shared-directory
+                      close-inferior-pipe)))
+
 (define* (port->inferior pipe shared-directory #:optional (close close-port))
   "Given PIPE, an input/output port, return an inferior that talks over PIPE.
 PIPE is closed with CLOSE when 'close-inferior' is called on the returned
-- 
2.21.0

[bug#34638] [PATCH v2 4/4] inferior: Add 'open-inferior/container'.

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> skribis:

> ---
>  guix/inferior.scm | 76 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)

[...]

> +(define* (open-inferior/container store guix-store-item
> +                                  #:key
> +                                  (command "bin/guix")
> +                                  (share-host-network? #f)
> +                                  (extra-shared-directories '())
> +                                  (extra-environment-variables '()))

Please add a docstring.  Same comment as before regarding “extras”.  :-)

> +    (start-child-in-container
> +     (list (string-append guix-store-item "/bin/guix")
> +           ;; TODO I'm not sure why "repl" is duplicated in the following
> +           ;; command
> +           "repl" "repl" "-t" "machine")

This is the argv[0] issue mentioned earlier.

I think it’s not really feasible to write a test for this one, or at
least I don’t see how.

Otherwise LGTM, thanks!

Ludo’.

[bug#34638] [PATCH v2 1/4] utils: Add #:base-directory to call-with-temporary-directory.

[Ludovic Courtès]

Hello,

Christopher Baines <mail@cbaines.net> skribis:

> This allows more easily creating temporary directories within a specific
> directory. This is motivated by using this in inferior-eval-with-store.
>
> * guix/utils.scm (call-with-temporary-directory): Add optional keyword
> argument, base-directory.

LGTM.