diff mbox series

[bug#38612] Pass system and target arguments to gexp->file.

Message ID 87imm468rr.fsf@gmail.com
State Superseded
Headers show
Series [bug#38612] Pass system and target arguments to gexp->file. | expand

Commit Message

Mathieu Othacehe Dec. 25, 2019, 9:42 a.m. UTC 
Hello,

Turns out two more patches in the same vein are needed in (gnu services)
and (gnu system). They are attached here.

Thanks,

Mathieu

Comments

Ludovic Courtès Dec. 26, 2019, 6:04 p.m. UTC | #1 
Hello,

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> From a05baf4f4328ce2ca6da6860f6e596cd7559a08a Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <m.othacehe@gmail.com>
> Date: Tue, 24 Dec 2019 18:24:37 +0100
> Subject: [PATCH 1/2] system: operating-system-boot-parameters-file: Fix
>  cross-compilation.
>
> * gnu/system.scm (operating-system-boot-parameters-file): Add system and
> target arguments and pass them to gexp->file call,
> (operating-system-directory-base-entries): pass current system and target to
> operating-system-boot-parameters-file procedure.
> ---
>  gnu/system.scm | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/gnu/system.scm b/gnu/system.scm
> index abdbb081e6..e7af7e7b47 100644
> --- a/gnu/system.scm
> +++ b/gnu/system.scm
> @@ -470,7 +470,10 @@ value of the SYSTEM-SERVICE-TYPE service."
>    (let ((locale (operating-system-locale-directory os)))
>      (mlet %store-monad ((kernel -> (operating-system-kernel os))
>                          (initrd -> (operating-system-initrd-file os))
> -                        (params    (operating-system-boot-parameters-file os)))
> +                        (params    (operating-system-boot-parameters-file
> +                                    os
> +                                    #:system (%current-system)
> +                                    #:target (%current-target-system))))

In general, in monadic code, we should refer to (current-system) and
(current-target-system), not to the SRFI-39 parameters.

> -(define* (operating-system-boot-parameters-file os
> -                                                #:key system-kernel-arguments?)
> +(define* (operating-system-boot-parameters-file
> +          os
> +          #:key
> +          system-kernel-arguments?
> +          system
> +          target)
>     "Return a file that describes the boot parameters of OS.  The primary use of
>  this file is the reconstruction of GRUB menu entries for old configurations.
>  
> @@ -1085,7 +1092,9 @@ being stored into the \"parameters\" file)."
>                       (device
>                        #$(device->sexp (boot-parameters-store-device params)))
>                       (mount-point #$(boot-parameters-store-mount-point params))))
> -                 #:set-load-path? #f)))
> +                 #:set-load-path? #f
> +                 #:system system
> +                 #:target target)))

By default, ‘gexp->file’ now uses the current system and target, so this
change shouldn’t be necessary if you just want to use those.  Am I
missing something?

The general guideline is that it would be good if only primitives
(monadic procedures in (guix gexp) as well as gexp compilers) would have
an explicit #:system and #:target parameter.

> From 0ce67afc4f33074e20824751c22ba01cf6a3e184 Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <m.othacehe@gmail.com>
> Date: Wed, 25 Dec 2019 09:49:53 +0100
> Subject: [PATCH 2/2] services: Fix cross-compilation.
>
> * gnu/services.scm (system-derivation): Pass current system and target at bind
> time to lower-object,
> (compute-boot-script): also pass current system and target at bind time to
> gexp->file.

[...]

>  (define (system-derivation mentries mextensions)
>    "Return as a monadic value the derivation of the 'system' directory
>  containing the given entries."
> -  (mlet %store-monad ((entries    mentries)
> +  (mlet %store-monad ((system (current-system))
> +                      (target (current-target-system))
> +                      (entries    mentries)
>                        (extensions (sequence %store-monad mextensions)))

Please alight the RHS of ‘mlet’ bindings.  :-)

>      (lower-object
>       (file-union "system"
> -                 (append entries (concatenate extensions))))))
> +                 (append entries (concatenate extensions)))
> +     system
> +     #:target target)))

I guess this is needed here because ‘lower-object’ has #:target default
to #f.

> +  (mlet %store-monad ((system (current-system))
> +                      (target (current-target-system)))
> +    (gexp->file "boot"
> +                ;; Clean up and activate the system, then spawn shepherd.
> +                #~(begin #$@(reverse gexps))
> +                #:system system
> +                #:target target)))

This one is unnecessary now that ‘gexp->file’ honors the current system
and target, right?

Thanks,
Ludo’.
Mathieu Othacehe Dec. 26, 2019, 6:54 p.m. UTC | #2 
Hey,

> In general, in monadic code, we should refer to (current-system) and
> (current-target-system), not to the SRFI-39 parameters.

Ok!

> By default, ‘gexp->file’ now uses the current system and target, so this
> change shouldn’t be necessary if you just want to use those.  Am I
> missing something?

Well turns out that gexp->file takes #f as default target (as
gexp->script and lower-object).

Using %current-target-system as default target argument (as gexp->sexp)
makes those two patches useless.

Would you have any objection to use %current-target-system as default
target argument in gexp->file and lower-object?

Thanks,

Mathieu
Ludovic Courtès Dec. 27, 2019, 6:05 p.m. UTC | #3 
Hello!

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

>> In general, in monadic code, we should refer to (current-system) and
>> (current-target-system), not to the SRFI-39 parameters.
>
> Ok!
>
>> By default, ‘gexp->file’ now uses the current system and target, so this
>> change shouldn’t be necessary if you just want to use those.  Am I
>> missing something?
>
> Well turns out that gexp->file takes #f as default target (as
> gexp->script and lower-object).

Oh, right.

> Using %current-target-system as default target argument (as gexp->sexp)
> makes those two patches useless.
>
> Would you have any objection to use %current-target-system as default
> target argument in gexp->file and lower-object?

As written above, it should default to ‘current-target-system’ rather
than ‘%current-target-system’.  Kind of annoying, but heh…

Also, we should probably get rid of the monadic style in (gnu services).
That would simplify things, but there are compatibility considerations…

Thanks,
Ludo’.
diff mbox series

Patch

From 0ce67afc4f33074e20824751c22ba01cf6a3e184 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Wed, 25 Dec 2019 09:49:53 +0100
Subject: [PATCH 2/2] services: Fix cross-compilation.

* gnu/services.scm (system-derivation): Pass current system and target at bind
time to lower-object,
(compute-boot-script): also pass current system and target at bind time to
gexp->file.
---
 gnu/services.scm | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/gnu/services.scm b/gnu/services.scm
index e7a3a95e43..e6f8ae0fb0 100644
--- a/gnu/services.scm
+++ b/gnu/services.scm
@@ -1,6 +1,7 @@ 
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015, 2016, 2017, 2018, 2019 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2016 Chris Marusich <cmmarusich@gmail.com>
+;;; Copyright © 2019 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -321,11 +322,15 @@  This is a shorthand for (map (lambda (svc) ...) %base-services)."
 (define (system-derivation mentries mextensions)
   "Return as a monadic value the derivation of the 'system' directory
 containing the given entries."
-  (mlet %store-monad ((entries    mentries)
+  (mlet %store-monad ((system (current-system))
+                      (target (current-target-system))
+                      (entries    mentries)
                       (extensions (sequence %store-monad mextensions)))
     (lower-object
      (file-union "system"
-                 (append entries (concatenate extensions))))))
+                 (append entries (concatenate extensions)))
+     system
+     #:target target)))
 
 (define system-service-type
   ;; This is the ultimate service type, the root of the service DAG.  The
@@ -346,9 +351,13 @@  system profile, boot script, and so on.")))
   ;; order.  That is, user extensions would come first, and extensions added
   ;; by 'essential-services' (e.g., running shepherd) are guaranteed to come
   ;; last.
-  (gexp->file "boot"
-              ;; Clean up and activate the system, then spawn shepherd.
-              #~(begin #$@(reverse gexps))))
+  (mlet %store-monad ((system (current-system))
+                      (target (current-target-system)))
+    (gexp->file "boot"
+                ;; Clean up and activate the system, then spawn shepherd.
+                #~(begin #$@(reverse gexps))
+                #:system system
+                #:target target)))
 
 (define (boot-script-entry mboot)
   "Return, as a monadic value, an entry for the boot script in the system
-- 
2.24.1