diff mbox series

[bug#41066] gnu: bootloader: Support for chain loading.

Message ID 975EC414-6A81-444B-9BB0-AE303C6A9511@vodafonemail.de
State Accepted
Headers show
Series [bug#41066] gnu: bootloader: Support for chain loading. | expand

Checks

Context Check Description
cbaines/applying patch fail View Laminar job

Commit Message

Stefan Oct. 18, 2020, 11:21 a.m. UTC
* gnu/bootloader.scm (bootloader-profile): New internal function to build a
profile from packages and files with a collection of contents to install.
(bootloader-chain): New function to chain a bootloader with contents of
additional bootloader or other packages and additional files like configuration
files or device-trees.

This allows to chain GRUB with U-Boot, device-tree-files, plain configuration
files, etc. mainly for single-board-computers like this:

(operating-system
  (bootloader
    (bootloader-configurationa
      (bootloader
        (bootloader-chain grub-efi-netboot-bootloader
                          (list u-boot-my-scb
                                firmware)
                          '("libexec/u-boot.bin"
                            "firmware/")
                          (list (plain-file "config.txt"
                                            "kernel=u-boot.bin"))
                          #:installer (install-grub-efi-netboot "efi/boot"))
        (target "/boot"))))
  …)
---
 gnu/bootloader.scm | 143 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 142 insertions(+), 1 deletion(-)

Comments

Danny Milosavljevic Oct. 22, 2020, 5:46 p.m. UTC | #1
Hi Stefan,

this looks very good in general.

I have only a few doubts--mostly concerning the end-user API "bootloader-chain":

On Sun, 18 Oct 2020 13:21:28 +0200
Stefan <stefan-guix@vodafonemail.de> wrote:

>         (bootloader-chain grub-efi-netboot-bootloader
>                           (list u-boot-my-scb
>                                 firmware)
>                           '("libexec/u-boot.bin"
>                             "firmware/")
>                           (list (plain-file "config.txt"
>                                             "kernel=u-boot.bin"))
>                           #:installer (install-grub-efi-netboot "efi/boot"))

I would prefer if it was possible to do the following:

 (bootloader-chain (list firmware (plain-file "config.txt" "kernel=u-boot.bin") u-boot-my-scb) grub-efi-netboot-bootloader)

(I would put the main bootloader last because the user probably thinks of the
list of bootloaders in the order they are loaded at boot)

[maybe even

 (bootloader-chain (list u-boot-my-scb firmware (plain-file "config.txt" "kernel=u-boot.bin") grub-efi-netboot-bootloader))

-- with documentation that the order of the entries matters.  But maybe that
would be too magical--since only the last entry in that list would have their
installer called, and the actual guix generations also only go into the last
one's configuration.  But maybe that would be OK anyway]

Also, I don't like the ad hoc derivation subset selection mechanism you have.

I understand that it can be nice to be able to select a subset in general, but:

* Usually we make a special package, inherit from the original package, and then
just make it put the files we want into the derivation output directory.
The user would put "u-boot-my-scb-minimal" instead of "u-boot-my-scb" in
that case, and then only get the subset.

* In this special case of chaining bootloaders, I think that the profile hook
can take care of deleting all the unnecessary files, and of all the other weird
complications (installing FILES, moving stuff around etc--maybe even generating
or updating that config.txt one day).
One of the reasons I suggested using a profile in the first place is that
now the profile hook can do all the dirty work, even long term.

* If that isn't possible either, it would be nicer to have a helper
procedure that allows you to select a subset of the files that
are in the derivation of a package, and not have this subset selection mingled
into the innards of bootloader-chain.  (separation of concerns)
Maybe there could even be a package transformation to do that.

I know that there are probably good reasons why you did it like you did.

But still, I think a profile hook is exactly the right kind of tool to hide
the extra setup complexity that my API requires, and the API would be less
complicated to use and more stable (less likely to ever need to be changed).

What do you think?

Also, if it is difficult to collect bootloader packages into a profile
automatically (without extra user-supplied information) because of the subdir
"libexec" in u-boot derivations, then we can modify all the u-boot packages
(for good) to put u-boot into "$output/" instead of "$output/libexec".
I would prefer fixing things instead of having to put workarounds into user
configuration.  Please tell me if you want that--we can do that.

>                           #:installer (install-grub-efi-netboot "efi/boot"))

That should be automatic but overridable.
This seems to be the case already.  Nice!

> +(define (bootloader-profile packages package-contents files)

If using my suggested bootloader-chain API, this would just get PACKAGES,
not PACKAGE-CONTENTS and FILES (FILES would be mixed into the PACKAGES list--which
thus should be renamed to ITEMS or something).

> +  (define (bootloader-collection manifest)
> +    (define build
> +        (with-imported-modules '((guix build utils)
> +                                 (ice-9 ftw)
> +                                 (srfi srfi-1))
> +          #~(begin
> +            (use-modules ((guix build utils)
> +                          #:select (mkdir-p strip-store-file-name))
> +                         ((ice-9 ftw)
> +                          #:select (scandir))
> +                         ((srfi srfi-1)
> +                          #:select (append-map every remove)))
> +            (define (symlink-to file directory transform)
> +              "Creates a symlink to FILE named (TRANSFORM FILE) in DIRECTORY."
> +              (when (file-exists? file)
> +                    (symlink file
> +                             (string-append directory "/" (transform file)))))
> +            (define (directory-content directory)
> +              "Creates a list of absolute path names inside DIRECTORY."
> +              (map (lambda (name)
> +                     (string-append directory name))
> +                   (sort (or (scandir directory
> +                                      (lambda (name)
> +                                        (not (member name '("." "..")))))
> +                             '())
> +                         string<?)))
> +            (define (select-names select names)
> +              "Set SELECT to 'filter' or 'remove' names ending with '/'."
> +              (select (lambda (name)
> +                        (string-suffix? "/" name))
> +                      names))
> +            (define (filter-names-without-slash names)
> +              (select-names remove names))
> +            (define (filter-names-with-slash names)
> +              (select-names filter names))

I would prefer these to be in another procedure that can be used to transform
any package (or profile?) like this.  @Ludo: What do you think?

[...]
> +    (gexp->derivation "bootloader-collection"
> +                      build
> +                      #:local-build? #t
> +                      #:substitutable? #f
> +                      #:properties
> +                      `((type . profile-hook)
> +                        (hook . bootloader-collection))))
> +
> +  (profile (content (packages->manifest packages))
> +           (name "bootloader-profile")
> +           (hooks (list bootloader-collection))
> +           (locales? #f)
> +           (allow-collisions? #f)
> +           (relative-symlinks? #f)))
> +
> +(define* (bootloader-chain
[...]

> +    (bootloader
> +     (inherit final-bootloader)
> +     (package profile)

I like that.  Smart way to reuse existing code.

> +     (installer
> +      #~(lambda (bootloader target mount-point)
> +          (#$final-installer bootloader target mount-point)
> +          (copy-recursively
> +           (string-append bootloader "/collection")
> +           (string-append mount-point target)
> +           #:follow-symlinks? #t
> +           #:log (%make-void-port "w")))))))

Thanks!
Ludovic Courtès Oct. 23, 2020, 12:48 p.m. UTC | #2
Hi,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> On Sun, 18 Oct 2020 13:21:28 +0200
> Stefan <stefan-guix@vodafonemail.de> wrote:
>
>>         (bootloader-chain grub-efi-netboot-bootloader
>>                           (list u-boot-my-scb
>>                                 firmware)
>>                           '("libexec/u-boot.bin"
>>                             "firmware/")
>>                           (list (plain-file "config.txt"
>>                                             "kernel=u-boot.bin"))
>>                           #:installer (install-grub-efi-netboot "efi/boot"))

In the example above, I think that you could merge the 2nd and 3rd
arguments by using ‘file-append’:

  (bootloader-chain grub-efi-netboot-bootloader
                    (list (file-append u-boot "/libexec/u-boot.bin")
                          (file-append firmware "/firmware")))

No?

I think we should look at how to simplify this interface.  Intuitively,
I would expect the ‘bootloader-chain’ to take a list of <bootloader>
records and return a <bootloader> record.

Is this something that can be achieved?  If not, what are the extra
constraints that need to be taken into account?

>> +(define (bootloader-profile packages package-contents files)
>
> If using my suggested bootloader-chain API, this would just get PACKAGES,
> not PACKAGE-CONTENTS and FILES (FILES would be mixed into the PACKAGES list--which
> thus should be renamed to ITEMS or something).

Yes, if it’s about building a profile, you could just use a <profile>
object.  Would that work here?

>> +  (define (bootloader-collection manifest)
>> +    (define build
>> +        (with-imported-modules '((guix build utils)
>> +                                 (ice-9 ftw)
>> +                                 (srfi srfi-1))
>> +          #~(begin
>> +            (use-modules ((guix build utils)
>> +                          #:select (mkdir-p strip-store-file-name))
>> +                         ((ice-9 ftw)
>> +                          #:select (scandir))
>> +                         ((srfi srfi-1)
>> +                          #:select (append-map every remove)))
>> +            (define (symlink-to file directory transform)
>> +              "Creates a symlink to FILE named (TRANSFORM FILE) in DIRECTORY."
>> +              (when (file-exists? file)
>> +                    (symlink file
>> +                             (string-append directory "/" (transform file)))))
>> +            (define (directory-content directory)
>> +              "Creates a list of absolute path names inside DIRECTORY."
>> +              (map (lambda (name)
>> +                     (string-append directory name))
>> +                   (sort (or (scandir directory
>> +                                      (lambda (name)
>> +                                        (not (member name '("." "..")))))
>> +                             '())
>> +                         string<?)))
>> +            (define (select-names select names)
>> +              "Set SELECT to 'filter' or 'remove' names ending with '/'."
>> +              (select (lambda (name)
>> +                        (string-suffix? "/" name))
>> +                      names))
>> +            (define (filter-names-without-slash names)
>> +              (select-names remove names))
>> +            (define (filter-names-with-slash names)
>> +              (select-names filter names))
>
> I would prefer these to be in another procedure that can be used to transform
> any package (or profile?) like this.  @Ludo: What do you think?

From a quick look at the patch, I don’t fully understand yet what’s
going on.

Stylistically, I’d suggest a few things to make the code more consistent
with the rest of the code base, and thus hopefully easier to grasp for
the rest of us:

  1. Don’t sort the result of ‘scandir’, it’s already sorted.

  2. Remove ‘select-names’ as it requires people to read more code to
     understand that we’re just filtering/removing.   Instead, declare:

       (define absolute-file-name? (cut string-suffix? "/" <>))

     and write:

       (filter absolute-file-name? names)
       (remote absolute-file-name? names)

HTH!

Ludo’.
Danny Milosavljevic Oct. 24, 2020, 1:30 a.m. UTC | #3
Hi,

On Fri, 23 Oct 2020 14:48:36 +0200
Ludovic Courtès <ludo@gnu.org> wrote:

>   (bootloader-chain grub-efi-netboot-bootloader
>                     (list (file-append u-boot "/libexec/u-boot.bin")
>                           (file-append firmware "/firmware")))

Ohhh!  That's right.  That's much better.  Can a profile be created with those
in it?  Especially because of the profile hook...

> I think we should look at how to simplify this interface.  Intuitively,
> I would expect the ‘bootloader-chain’ to take a list of <bootloader>
> records and return a <bootloader> record.
> Is this something that can be achieved?  If not, what are the extra
> constraints that need to be taken into account?

That is not easily possible, and is also logically not what happens anyway.

The use case of this entire patchset is when one (for some reason) can't boot
the final bootloader directly, then one uses some chain of bootloaders to
get the final bootloader to boot.

That especially means that all the bootloaders before the final bootloader
WILL NOT GET THE GUIX GENERATIONS MENU.

It is also pretty uncommon/impossible to use each usual bootloader installer
in order to install all the bootloaders one after another.  Just think of
what would happen if multiple x86_64 bootloaders all tried to install
themselves into the first sector of the drive.  That's not gonna work
correctly.

What actually happens is that there's some kind of payload area in the first
bootloader where you can put the second bootloader, and some kind of payload
area in the second bootloader where you can put the third bootloader... and so
on.  Except for the final bootloader, which has the Linux kernel in the payload
area (as far as the final bootloader is concerned, it can do everything as if
it was the first and only thing that was loaded at boot so far).

That means the final bootloader can use the normal config files and generally
proceed like all our standalone bootloaders do.  None of the previous bootloaders
in the chain can do that, generally.

>bootloader-profile

> Yes, if it’s about building a profile, you could just use a <profile>
> object.  Would that work here?

Huh?  Isn't he doing that already?

That's what that procedure does.  Or am I misunderstanding?

> >From a quick look at the patch, I don’t fully understand yet what’s  
> going on.

I suggested to Stefan to use a profile with a profile hook in order to
configure all those bootloaders of a bootloader chain correctly.  That's
what he does here.

Usually, Guix bootloader *packages* have a lot of junk that (1) you wouldn't
want on a esp partition (wastes space) and also stuff that would be duplicates
with other bootloaders (COPYING etc).  Therefore, it's nice to be able to
filter what files of those packages get used.  I think your suggestion in the
beginning is the best one.  (file-append u-boot "/libexec/u-boot.bin") indeed!
The profile hook can then use whatever methods to configure all those
bootloaders correctly.
Ludovic Courtès Oct. 24, 2020, 4:22 p.m. UTC | #4
Hi,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> On Fri, 23 Oct 2020 14:48:36 +0200
> Ludovic Courtès <ludo@gnu.org> wrote:
>
>>   (bootloader-chain grub-efi-netboot-bootloader
>>                     (list (file-append u-boot "/libexec/u-boot.bin")
>>                           (file-append firmware "/firmware")))
>
> Ohhh!  That's right.  That's much better.  Can a profile be created with those
> in it?  Especially because of the profile hook...

Yes, there’s first-class <profile> objects that one can use in gexps:

  (profile (content (manifest (entries …))))

>> I think we should look at how to simplify this interface.  Intuitively,
>> I would expect the ‘bootloader-chain’ to take a list of <bootloader>
>> records and return a <bootloader> record.
>> Is this something that can be achieved?  If not, what are the extra
>> constraints that need to be taken into account?
>
> That is not easily possible, and is also logically not what happens anyway.
>
> The use case of this entire patchset is when one (for some reason) can't boot
> the final bootloader directly, then one uses some chain of bootloaders to
> get the final bootloader to boot.
>
> That especially means that all the bootloaders before the final bootloader
> WILL NOT GET THE GUIX GENERATIONS MENU.
>
> It is also pretty uncommon/impossible to use each usual bootloader installer
> in order to install all the bootloaders one after another.  Just think of
> what would happen if multiple x86_64 bootloaders all tried to install
> themselves into the first sector of the drive.  That's not gonna work
> correctly.
>
> What actually happens is that there's some kind of payload area in the first
> bootloader where you can put the second bootloader, and some kind of payload
> area in the second bootloader where you can put the third bootloader... and so
> on.  Except for the final bootloader, which has the Linux kernel in the payload
> area (as far as the final bootloader is concerned, it can do everything as if
> it was the first and only thing that was loaded at boot so far).
>
> That means the final bootloader can use the normal config files and generally
> proceed like all our standalone bootloaders do.  None of the previous bootloaders
> in the chain can do that, generally.

Sorry, I don’t see why this prevents an API that more closely matches
the idea of a chain of bootloaders (but perhaps I’d just need to spend
more time studying this.)

I guess my advice is: design an interface that’s as close as possible to
the concepts at hand.  If implementation details constrain what can be
done, that’s OK, but it should be clear in that case why we end up with
an interface that’s not as simple as one could expect.

>>bootloader-profile
>
>> Yes, if it’s about building a profile, you could just use a <profile>
>> object.  Would that work here?
>
> Huh?  Isn't he doing that already?
>
> That's what that procedure does.  Or am I misunderstanding?

It’s not using code from (guix profiles) IIRC.

>> >From a quick look at the patch, I don’t fully understand yet what’s  
>> going on.
>
> I suggested to Stefan to use a profile with a profile hook in order to
> configure all those bootloaders of a bootloader chain correctly.  That's
> what he does here.
>
> Usually, Guix bootloader *packages* have a lot of junk that (1) you wouldn't
> want on a esp partition (wastes space) and also stuff that would be duplicates
> with other bootloaders (COPYING etc).  Therefore, it's nice to be able to
> filter what files of those packages get used.  I think your suggestion in the
> beginning is the best one.  (file-append u-boot "/libexec/u-boot.bin") indeed!
> The profile hook can then use whatever methods to configure all those
> bootloaders correctly.

Alrighty!

Thanks,
Ludo’.
Danny Milosavljevic Oct. 25, 2020, 12:33 a.m. UTC | #5
Hi Ludo,

On Sat, 24 Oct 2020 18:22:48 +0200
Ludovic Courtès <ludo@gnu.org> wrote:

> >>   (bootloader-chain grub-efi-netboot-bootloader
> >>                     (list (file-append u-boot "/libexec/u-boot.bin")
> >>                           (file-append firmware "/firmware")))  
> >
> > Ohhh!  That's right.  That's much better.  Can a profile be created with those
> > in it?  Especially because of the profile hook...  
> 
> Yes, there’s first-class <profile> objects that one can use in gexps:
> 
>   (profile (content (manifest (entries …))))

Nice!

I haven't used those before, so no idea whether it would be better here.

> Sorry, I don’t see why this prevents an API that more closely matches
> the idea of a chain of bootloaders (but perhaps I’d just need to spend
> more time studying this.)

It doesn't prevent that API--but this narrow use case here doesn't need it.

And I do not intend to implement the general use case--we all decided against
general chainloading and then introduced full support for bootloaders other
than grub (which then do not chainload grub--they totally could have!).

But this use case with lots of bootloaders on an ESP partition (or Raspberry Pi
equivalent) is easy enough.  Maybe we should change the name of the main
procedure to be less general, though.  "chain-esp-bootloaders" ?

If you could check it out more, that would help.  I think how the patch is
now is fine for the narrow use case: chainloading the normal guix bootloader
using other bootloaders (or whatever else!  Maybe a turtle is loading the final
guix bootloader--who knows ;) ).

The code here can only chain bootloaders in the ESP partition (actually, the
Raspberry Pi equivalent of the latter).

> >>bootloader-profile  
> >  
> >> Yes, if it’s about building a profile, you could just use a <profile>
> >> object.  Would that work here?  
> >
> > Huh?  Isn't he doing that already?
> >
> > That's what that procedure does.  Or am I misunderstanding?  
> 
> It’s not using code from (guix profiles) IIRC.

From the patch:

>+(define (bootloader-profile packages package-contents files)
>[...]
>+  (profile (content (packages->manifest packages))
>+           (name "bootloader-profile")
>+           (hooks (list bootloader-collection))
>+           (locales? #f)
>+           (allow-collisions? #f)
>+           (relative-symlinks? #f)))

Maybe I don't understand what you mean... but that "profile" is from
(guix profiles).

In any case, maybe we should just call it "esp-bootloader-chain" or
maybe just "raspberry-pi-bootloader-chain".
Stefan Oct. 25, 2020, 4:58 p.m. UTC | #6
Hi Danny and Ludo!

I tried to consider your comments and modified the code as far as I could grasp the suggestions. Thanks!

Now the API looks like this:

(bootloader-chain
          (list (file-append firmware "/boot/")
                (file-append u-boot-my-scb "/libexec/u-boot.bin")
                (plain-file "config.txt"
                            "kernel=u-boot.bin"))
          grub-efi-netboot-bootloader
          #:hook my-special-bootloader-profile-manipulator
          #:installer (install-grub-efi-netboot "efi/boot"))

The suggestion to use file-append simplified a lot, also for the implementation of the bootloader-collection profile hook. I also added an optional hook function to do customised manipulations of the profile before invoking the installer.

Regarding this kind of chain-loading: The ARM world seems to consolidate onto an UEFI firmware, supporting either device-trees or ACPI. There are two main options for an UEFI firmware to chose from: TianoCore/EDK II and U-Boot.

Some platforms come with an EEPROM or NAND storage to install e.g. U-Boot with embedded device-tree information as an UEFI firmware. From a distribution’s point of view this can make using GRUB-EFI the default choice. And it becomes arguable if the distribution is responsible to install/update this firmware, if you compare to a BIOS.

Other platforms just boot from some FAT partition requiring some blobs and don’t offer an UEFI firmware. But copying e.g. U-Boot and some more files onto this FAT partition makes them appear like a system with an UEFI firmware, giving a kind of compatibility to the future.


Bye

Stefan
Ludovic Courtès Oct. 26, 2020, 10:37 a.m. UTC | #7
Hi,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

>> Sorry, I don’t see why this prevents an API that more closely matches
>> the idea of a chain of bootloaders (but perhaps I’d just need to spend
>> more time studying this.)
>
> It doesn't prevent that API--but this narrow use case here doesn't need it.
>
> And I do not intend to implement the general use case--we all decided against
> general chainloading and then introduced full support for bootloaders other
> than grub (which then do not chainload grub--they totally could have!).
>
> But this use case with lots of bootloaders on an ESP partition (or Raspberry Pi
> equivalent) is easy enough.  Maybe we should change the name of the main
> procedure to be less general, though.  "chain-esp-bootloaders" ?
>
> If you could check it out more, that would help.  I think how the patch is
> now is fine for the narrow use case: chainloading the normal guix bootloader
> using other bootloaders (or whatever else!  Maybe a turtle is loading the final
> guix bootloader--who knows ;) ).
>
> The code here can only chain bootloaders in the ESP partition (actually, the
> Raspberry Pi equivalent of the latter).

Oh got it, I thought it was about bootloader chaining “in general”,
apologies for the confusion!

> From the patch:
>
>>+(define (bootloader-profile packages package-contents files)
>>[...]
>>+  (profile (content (packages->manifest packages))
>>+           (name "bootloader-profile")
>>+           (hooks (list bootloader-collection))
>>+           (locales? #f)
>>+           (allow-collisions? #f)
>>+           (relative-symlinks? #f)))
>
> Maybe I don't understand what you mean... but that "profile" is from
> (guix profiles).

Oops, you’re right; my bad!

> In any case, maybe we should just call it "esp-bootloader-chain" or
> maybe just "raspberry-pi-bootloader-chain".

Yes, maybe that’d be clearer (perhaps the former, unless there’s
something really RPi-specific).

Thanks,
Ludo’.
Danny Milosavljevic Nov. 16, 2020, 9:33 a.m. UTC | #8
Pushed to guix master as commit 74eeb11daee906cb012f10b6bb3afd254f9ea5c2,
after renaming bootloader-chain to efi-bootloader-chain.
Stefan Nov. 17, 2020, 2:26 p.m. UTC | #9
Hi Danny!

> Pushed to guix master as commit 74eeb11daee906cb012f10b6bb3afd254f9ea5c2,
> after renaming bootloader-chain to efi-bootloader-chain.

You missed my last patch-set, unfortunately. Is it possible to push that as well?

https://lists.gnu.org/archive/html/guix-patches/2020-11/msg00212.html
https://lists.gnu.org/archive/html/guix-patches/2020-11/msg00213.html


Bye

Stefan
Danny Milosavljevic Nov. 17, 2020, 3:47 p.m. UTC | #10
Hi Stefan,

oops.

I've reviewed and pushed the first part (s;HOOK;HOOKS;) now as guix master
commit ede4117f7f18e118003f2599f5c8e985dfbdf9a5.

But I'm not sure whether the COPY-FILES? thing is a nice API (and I mean not just
the flag).

I would prefer if the user would just change the INSTALLER in the case he wants
to not use the profile (which is kinda weird?!) or pack it or whatever.

In case the user wanted to index the profile content, the user would use a HOOK
to do that.

It really depends on what exactly efi-bootloader-chain is being presented as.

Is it a profile ?  Then it shouldn't have weird flags like
that--if possible--and instead use the standard methods.

For example you could do instead (I cut&pasted to show the idea--untested!):

(define* (efi-bootloader-chain files
                               final-bootloader
                               #:key
                               (hooks '())
                               installer)

  (let* ((final-installer (or installer
                              (lambda (bootloader target mount-point)
                                ((bootloader-installer bootloader) bootloader target mount-point)
            (let* ((mount-point/target (string-append mount-point target))
                     ;; When installing Guix, it's common to mount TARGET below
                     ;; MOUNT-POINT rather than below the root directory.
                     (bootloader-target (if (file-exists? mount-point/target)
                                            mount-point/target
                                            target)))
              (when (eq? (and=> (stat bootloader-target #f) stat:type)
                        'directory)
                (copy-recursively (string-append bootloader "/collection")
                                  bootloader-target
                                  #:follow-symlinks? #t
                                  #:log (%make-void-port "w")))))))))))))
         (profile (efi-bootloader-profile files
                                          (bootloader-package final-bootloader)
                                          (if (list? hooks)
                                              hooks
                                              (list hooks)))))
    (bootloader
     (inherit final-bootloader)
     (package profile)
     (installer
      #~(lambda (bootloader target mount-point)
          (#$final-installer bootloader target mount-point))))

This way the weird flag COPY-FILES? is gone with no loss of functionality to
the customizer.  It's possible for the customizer to read the bootloader
package (profile), so it's still possible to copy stuff if it's required
(pass a custom INSTALLER which does the copying and also some custom
installation).

I have a few questions:

(1) Why is there a $output/collection subdirectory?  Is there something
else than that in the profile output?  
If there are no good reasons to do it like that, I'd just put the
profile into $output directly instead--it's easier to understand, and also it's
how other profiles are being used.

(2) The COPY-FILES? flag is kinda weird.
I would prefer if INSTALLER just defaulted to a procedure that: does copy
files, and then calls the final bootloader installer.
If the user doesn't want it then the user could still pass a INSTALLER
that doesn't (for example the user could pass #:installer
(bootloader-installer final-bootloader)).

(3) Why isn't the final bootloader installed last?  I would have expected
it to be installed last so that if it does packing of the profile contents
in order to quickly find it at boot, it would have to have all the files
of the profiles already, no?

Could you explain what this is for in your use case?  I don't yet understand
the reason for this complexity.
Danny Milosavljevic Nov. 17, 2020, 4:17 p.m. UTC | #11
Also, please keep in mind Ludo's suggestion of using the first class profiles
of G-Expressions.  I'm not sure it's applicable in this case (!)--but still, it
would be nice, for a future version of raspberry efi netboot or of similar
things, to be able to do this, right from config.scm:

  (operating-system
    (bootloader
      (bootloader-configuration
        (bootloader ; field in bootloader-configuration
          (bootloader ; bootloader record constructor
            (inherit grub-efi-netboot-bootloader) ; so we have the installer
            (package ; override bootloader's package
             (profile ; create a profile
              (content
               (list (file-append firmware "/boot/")
                     (file-append u-boot-my-scb "/libexec/u-boot.bin")
                     (plain-file "config.txt"
                                 "kernel=u-boot.bin")
                     (bootloader-package grub-efi-netboot-bootloader))))) ; and put the package we've overridden back into the profile
            (target "/boot"))))

This way, maybe the procedures efi-bootloader-chain and
efi-bootloader-profile would be superfluous.

Note: I haven't used this myself yet.
Stefan Nov. 17, 2020, 8:27 p.m. UTC | #12
Hi Danny!

> I've reviewed and pushed the first part (s;HOOK;HOOKS;) now as guix master
> commit ede4117f7f18e118003f2599f5c8e985dfbdf9a5.

OK, thanks.

> Could you explain what this is for in your use case?  I don't yet understand
> the reason for this complexity.

Sure.

> (1) Why is there a $output/collection subdirectory?  Is there something
> else than that in the profile output?  

The profile contains first of all the GRUB package with all its tools and files. This profile is the ‘package’ argument to the GRUB installer. Having the profile hook create the collection folder eases the task for an installer. The simple idea is to copy all the additional files along with the installed GRUB bootloader into /boot. Without the collection/ the installer would require the list of files as well and reimplement the same functionality as already inside the hook, to copy the files into the /boot/ folder.

For example the profile contains at least these folders from the grub package, plus the collection/:

/gnu/store/…-bootloader-profile/collection
/gnu/store/…-bootloader-profile/etc
/gnu/store/…-bootloader-profile/share
/gnu/store/…-bootloader-profile/lib
/gnu/store/…-bootloader-profile/bin
/gnu/store/…-bootloader-profile/sbin

But the /boot/ folder finally contains something like this, with most of it being content from the collection/, where the GRUB files get installed belaw /boot/efi/:

/boot/bcm2710-rpi-3-b.dtb
/boot/bootcode.bin
/boot/bootloader.txt
/boot/config.txt
/boot/custom.txt
/boot/efi/
/boot/fixup.dat
/boot/gnu/
/boot/grub/
/boot/overlays/
/boot/u-boot.bin

Actually one could say that the profile hook is a kind of ‘pre-installer’ for everything not related to GRUB, able to prepare everything, but unable to write into the /boot folder.

> If there are no good reasons to do it like that, I'd just put the
> profile into $output directly instead--it's easier to understand, and also it's
> how other profiles are being used.

Not having the collection folder would mean that the installer would need to assume much more about the result of the profile hook, to copy the right files to /boot.

> (2) The COPY-FILES? flag is kinda weird.
> I would prefer if INSTALLER just defaulted to a procedure that: does copy
> files, and then calls the final bootloader installer.
> If the user doesn't want it then the user could still pass a INSTALLER
> that doesn't (for example the user could pass #:installer
> (bootloader-installer final-bootloader)).

Agreed. Another possibility would be to remove the collection folder within a hook.

> I would prefer if the user would just change the INSTALLER in the case he wants
> to not use the profile (which is kinda weird?!) or pack it or whatever.

OK, I see, in case of a custom installer we can skip the copying completely. That makes sense. 

> (3) Why isn't the final bootloader installed last?  I would have expected
> it to be installed last so that if it does packing of the profile contents
> in order to quickly find it at boot, it would have to have all the files
> of the profiles already, no?

I thought about the order as well. My conclusion was that a file from the collection should be able to overwrite a file installed from final-bootloader, for example to install own device-tree files.

What do think?


Bye

Stefan
Danny Milosavljevic Nov. 18, 2020, 6:05 p.m. UTC | #13
Hi Stefan,

On Tue, 17 Nov 2020 21:27:42 +0100
Stefan <stefan-guix@vodafonemail.de> wrote:

> For example the profile contains at least these folders from the grub package, plus the collection/:
> 
> /gnu/store/…-bootloader-profile/collection
> /gnu/store/…-bootloader-profile/etc
> /gnu/store/…-bootloader-profile/share
> /gnu/store/…-bootloader-profile/lib
> /gnu/store/…-bootloader-profile/bin
> /gnu/store/…-bootloader-profile/sbin

Ohhh, that's right.  It actually contains the grub installer--which it got from
the grub-efi derivation.  The actual boot EFI file is not in the grub-efi
derivation.

I wonder if it is possible (one day!) to have a package that actually contains
the EFI boot file that the grub installer generated.  That would be a lot
nicer--though I'm not sure whether that stuff depends on the system and thus
cannot be "pre-generated".

> But the /boot/ folder finally contains something like this, with most of it being content from the collection/, where the GRUB files get installed belaw /boot/efi/:
> 
> /boot/bcm2710-rpi-3-b.dtb
> /boot/bootcode.bin
> /boot/bootloader.txt
> /boot/config.txt
> /boot/custom.txt
> /boot/efi/
> /boot/fixup.dat
> /boot/gnu/
> /boot/grub/
> /boot/overlays/
> /boot/u-boot.bin
> 
> Actually one could say that the profile hook is a kind of ‘pre-installer’ for everything not related to GRUB, able to prepare everything, but unable to write into the /boot folder.
> 
> > If there are no good reasons to do it like that, I'd just put the
> > profile into $output directly instead--it's easier to understand, and also it's
> > how other profiles are being used.  
> 
> Not having the collection folder would mean that the installer would need to assume much more about the result of the profile hook, to copy the right files to /boot.

I agree.  Let's leave it inside subdir "collection".

> > (2) The COPY-FILES? flag is kinda weird.
> > I would prefer if INSTALLER just defaulted to a procedure that: does copy
> > files, and then calls the final bootloader installer.
> > If the user doesn't want it then the user could still pass a INSTALLER
> > that doesn't (for example the user could pass #:installer
> > (bootloader-installer final-bootloader)).  
> 
> Agreed.

> Another possibility would be to remove the collection folder within a hook.

I don't like that that much because it's mutating too much and thus
composability goes down.

> > I would prefer if the user would just change the INSTALLER in the case he wants
> > to not use the profile (which is kinda weird?!) or pack it or whatever.  
> 
> OK, I see, in case of a custom installer we can skip the copying completely. That makes sense. 

Could you send an updated patch that does it like that?  Or do you rather want
the old variant ?

> > (3) Why isn't the final bootloader installed last?  I would have expected
> > it to be installed last so that if it does packing of the profile contents
> > in order to quickly find it at boot, it would have to have all the files
> > of the profiles already, no?  
> 
> I thought about the order as well. My conclusion was that a file from the collection should be able to overwrite a file installed from final-bootloader, for example to install own device-tree files.

Yeah, that makes sense--if a little unusual (thus should be documented).
Stefan Nov. 18, 2020, 6:20 p.m. UTC | #14
Hi Danny!

> Could you send an updated patch that does it like that?

Sure, it’ll just need a little while.


Bye

Stefan
diff mbox series

Patch

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 2eebb8e9d9..745618f204 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -22,6 +22,8 @@ 
 
 (define-module (gnu bootloader)
   #:use-module (guix discovery)
+  #:use-module (guix gexp)
+  #:use-module (guix profiles)
   #:use-module (guix records)
   #:use-module (guix ui)
   #:use-module (srfi srfi-1)
@@ -66,7 +68,9 @@ 
             bootloader-configuration-additional-configuration
 
             %bootloaders
-            lookup-bootloader-by-name))
+            lookup-bootloader-by-name
+
+            bootloader-chain))
 
 ^L
 ;;;
@@ -227,3 +231,140 @@  record."
               (eq? name (bootloader-name bootloader)))
             (force %bootloaders))
       (leave (G_ "~a: no such bootloader~%") name)))
+
+(define (bootloader-profile packages package-contents files)
+  "Creates a profile with PACKAGES and additional FILES.  The new profile will
+contain a directory collection/ with links to selected PACKAGE-CONTENTS and
+FILES.  This collection is meant to be used by the bootloader installer.
+
+PACKAGE-CONTENTS is a list of file or directory names relative to the PACKAGES,
+which will be symlinked into the collection/ directory.  If a directory name
+ends with '/', then the directory content instead of the directory itself will
+be symlinked into the collection/ directory.
+
+FILES is a list of file like objects produced by functions like plain-file,
+local-file, etc., which will be symlinked into the collection/ directory, too."
+  (define (bootloader-collection manifest)
+    (define build
+        (with-imported-modules '((guix build utils)
+                                 (ice-9 ftw)
+                                 (srfi srfi-1))
+          #~(begin
+            (use-modules ((guix build utils)
+                          #:select (mkdir-p strip-store-file-name))
+                         ((ice-9 ftw)
+                          #:select (scandir))
+                         ((srfi srfi-1)
+                          #:select (append-map every remove)))
+            (define (symlink-to file directory transform)
+              "Creates a symlink to FILE named (TRANSFORM FILE) in DIRECTORY."
+              (when (file-exists? file)
+                    (symlink file
+                             (string-append directory "/" (transform file)))))
+            (define (directory-content directory)
+              "Creates a list of absolute path names inside DIRECTORY."
+              (map (lambda (name)
+                     (string-append directory name))
+                   (sort (or (scandir directory
+                                      (lambda (name)
+                                        (not (member name '("." "..")))))
+                             '())
+                         string<?)))
+            (define (select-names select names)
+              "Set SELECT to 'filter' or 'remove' names ending with '/'."
+              (select (lambda (name)
+                        (string-suffix? "/" name))
+                      names))
+            (define (filter-names-without-slash names)
+              (select-names remove names))
+            (define (filter-names-with-slash names)
+              (select-names filter names))
+            (let* ((collection (string-append #$output "/collection"))
+                   (packages '#$(map (lambda (entry)
+                                       (manifest-entry-item entry))
+                                     (manifest-entries manifest)))
+                   ;; As the profile content will only be collected after this
+                   ;; hook function was executed, the CONTENTS varibale will
+                   ;; contain all permutations of elements from PACKAGES and
+                   ;; PACKAGE-CONTENTS.
+                   (contents (append-map
+                               (lambda (name)
+                                 (map (lambda (package)
+                                        (string-append package "/" name))
+                                      packages))
+                               '#$package-contents))
+                   (directories (filter-names-with-slash contents))
+                   (names-from-directories
+                    (append-map (lambda (directory)
+                                  (directory-content directory))
+                                directories))
+                   (names (append names-from-directories
+                                  (filter-names-without-slash contents))))
+              (mkdir-p collection)
+              (for-each (lambda (name)
+                          (symlink-to name collection basename))
+                        names)
+              (for-each (lambda (file)
+                          (symlink-to file collection strip-store-file-name))
+                        '#$files)
+              ;; Ensure that all elements of PACKEGE-CONTENTS got collected.
+              (if (every (lambda (content)
+                           (file-exists? (string-append collection "/"
+                                         (basename content))))
+                         '#$package-contents)
+                  #t
+                  #f)))))
+
+    (gexp->derivation "bootloader-collection"
+                      build
+                      #:local-build? #t
+                      #:substitutable? #f
+                      #:properties
+                      `((type . profile-hook)
+                        (hook . bootloader-collection))))
+
+  (profile (content (packages->manifest packages))
+           (name "bootloader-profile")
+           (hooks (list bootloader-collection))
+           (locales? #f)
+           (allow-collisions? #f)
+           (relative-symlinks? #f)))
+
+(define* (bootloader-chain final-bootloader
+                           bootloader-packages
+                           bootloader-package-contents
+                           #:optional (files '())
+                           #:key installer)
+  "Defines a bootloader chain with the FINAL-BOOTLOADER as the final bootloader
+and certain directories and files given in the BOOTLOADER-PACKAGE-CONTENTS list
+relative to list of BOOTLOADER-PACKAGES and additional FILES.
+
+Along with the installation of the FINAL-BOOTLOADER these additional FILES and
+BOOTLOADER-PACKAGE-CONTENTS will be copied into the bootloader target directory.
+
+If a directory name in BOOTLOADER-PACKAGE-CONTENTS ends with '/', then the
+directory content instead of the directory itself will be copied.
+
+FILES is a list of file like objects produced by functions like plain-file,
+local-file, etc.
+
+If the INSTALLER argument is used, then this will be used as the bootloader
+installer.  Otherwise the intaller of the FINAL-BOOTLOADER will be used."
+  (let* ((final-installer (or installer
+                              (bootloader-installer final-bootloader)))
+         (profile (bootloader-profile
+                   (cons (bootloader-package final-bootloader)
+                         bootloader-packages)
+                   bootloader-package-contents
+                   files)))
+    (bootloader
+     (inherit final-bootloader)
+     (package profile)
+     (installer
+      #~(lambda (bootloader target mount-point)
+          (#$final-installer bootloader target mount-point)
+          (copy-recursively
+           (string-append bootloader "/collection")
+           (string-append mount-point target)
+           #:follow-symlinks? #t
+           #:log (%make-void-port "w")))))))