Package: guix-patches;
Reported by: typ22 <at> foxmail.com
Date: Wed, 31 Aug 2022 05:26:02 UTC
Severity: normal
Tags: patch
Done: Julien Lepiller <julien <at> lepiller.eu>
Bug is archived. No further changes may be made.
Message #23 received at 57496 <at> debbugs.gnu.org (full text, mbox):
From: Julien Lepiller <julien <at> lepiller.eu> To: tiantian <typ22 <at> foxmail.com> Cc: 57496 <at> debbugs.gnu.org Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader. Date: Wed, 31 Aug 2022 21:34:06 +0200
Le Thu, 1 Sep 2022 01:55:34 +0800,
tiantian <typ22 <at> foxmail.com> a écrit :
> Dear Mr/Ms Lepiller,
>
> I'm sorry. I didn't notice the wrong sender name.
You don't have to apologize. I received your email and I didn't even
notice the sender name :)
>
> Because my patch was replied so quickly for the first time, I am
> excited and hope to reply as soon as possible.
>
> After the last installation of icedove, the mail client on my
> computer, I don't know why icedove lost all configuration information
> this time, so I log in again. Icedove defaults to the desktop user
> name I set for testing the guix system. In a hurry, I didn't notice
> the mistake.
>
> I checked the contents of the email and tried to make the meaning
> accurate. Without noticing that the name of the sender of the mail is
> wrong.
>
> I'm not sure if the previous mail was intercepted in the trash, so I
> forward it.
>
> My mistake was really stupid. I sincerely apologize for disturbing
> you.
>
> Sincerely,
> tiantian
>
> -------- Forwarded Message --------
> Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend
> `<menu-entry>' for chain-loader. Date: Wed, 31 Aug 2022 20:22:55 +0800
> From: user in guix system <typ22 <at> foxmail.com>
> To: Julien Lepiller <julien <at> lepiller.eu>, 57496 <at> debbugs.gnu.org
>
> Hi,
>
> On 2022/8/31 15:18, Julien Lepiller wrote:
> > Hi tiantian!
> >
> > Thanks for the patches. It's a bit hard to follow since I'm not at a
> > computer, but here are some preliminary remarks.
> >
> > First, thanks for documenting the change in the manual, it's not
> > something even I think about all the time ^^'. We'll have to rework
> > the English a bit but it's understandable :)
> >
>
> My English is not good. Thank you for correcting the document.
Let's try something like this:
@item @code{chain-loader} (default: @code{#f})
A string that can be accepted by @code{grub}'s @code{chainloader}
directive. This has no effect if either @code{linux} or
@code{linux-multiboot} fields are specified. The following is an
example of chainloading a different GNU/Linux system.
>
> > I'm wondering why there are two patches? The first patch below seems
> > documented but the second patch does not have a changelog text.
> > Shouldn't they be merged? They seem to be for the same thing.
> >
>
> I have little submission experience and my English isn't good, so I
> refer to the submission history of multiboot. As you can see, my
> submission also imitates it. However, I did not modify
> <boot-parameters> like it. On the one hand, I did not understand the
> role of <boot-parameters>. on the other hand, it has successfully
> passed the tests on my computer, such as reconfigure,
> switch-generation and roll-back.
I didn't know about boot-parameters. It sounds like they are related to
guix deploy, which I don't use. I think we can still go with your
patch, and later add chainloading in boot-parameters if needed.
>
> If it is better to merge into the first, there is no problem. How can
> I do this? Should I send v2 here after local merge, or other?
>
> commit 1244491a0d5334e1589159a2ff67bbc967b9648b
> Author: Jan (janneke) Nieuwenhuizen <janneke <at> gnu.org>
> Date: Tue May 26 18:09:01 2020 +0200
>
> bootloader: grub: Add support for multiboot.
>
> * gnu/bootloader/grub.scm (grub-configuration-file): Add support
> for multiboot.
>
> commit 912b857ede450828805e09bb718658f79c40703a
> Author: Jan (janneke) Nieuwenhuizen <janneke <at> gnu.org>
> Date: Tue May 26 17:38:30 2020 +0200
>
> system: Add 'multiboot-modules' field to <boot-parameters>.
>
> * gnu/system.scm (<boot-parameters>)[multiboot-modules]: New
> field. (read-boot-parameters): Initialize it.
> (operating-system-multiboot-modules, hurd-multiboot-modules):
> New procedure. (operating-system-boot-parameters): Cater for
> multiboot the Hurd and initialize it; avoid initrd in that case.
> (operating-system-kernel-file): Cater for for Gnumach (the Hurd)
> besides Linux. (boot-parameters->menu-entry): Use it to support a
> multiboot <menu-entry>.
>
> commit 21acd8d6c11b85d06c82b168807b35cb7d2d0adf
> Author: Jan (janneke) Nieuwenhuizen <janneke <at> gnu.org>
> Date: Tue May 26 16:54:18 2020 +0200
>
> bootloader: Extend `<menu-entry>' for multiboot.
>
> * gnu/bootloader.scm
> (<menu-entry>)[multiboot-kernel,multiboot-arguments,
> multiboot-modules]: New fields. [linux,initrd]: Add default value
> '#f'. (menu-entry->sexp, sexp->menu-entry): Support multiboot entry.
> * doc/guix.texi (Bootloader Configuration): Document them.
>
OK, I see now. I don't really understand why they were separate, but
let's keep them separate for now.
> > From what I understand, specifying chainloader with at least linux
> > or linux-multiboot will lead to chainloader being silently dropped.
> > Maybe we should have an error message instead?
> >
>
> Yes. Thank you for asking this question. I didn't think about it
> before. What I can think of now is to complete pattern as follows.
> When the chainloader and Linux or multiboot are specified at the same
> time, it will report an error message.
>
> But I feel that this is not elegant, it will make code difficult to
> read. I am a beginner of scheme. Could you give me some advice?
>
> --- >8 ---
>
> (match entry
> (($ <menu-entry> label device mount-point linux linux-arguments
> initrd #f () () #f)
> `(menu-entry (version 0)
> (label ,label)
> (device ,(device->sexp device))
> (device-mount-point ,mount-point)
> (linux ,linux)
> (linux-arguments ,linux-arguments)
> (initrd ,initrd)))
> (($ <menu-entry> label device mount-point #f () #f
> multiboot-kernel multiboot-arguments
> multiboot-modules #f)
> `(menu-entry (version 0)
> (label ,label)
> (device ,(device->sexp device))
> (device-mount-point ,mount-point)
> (multiboot-kernel ,multiboot-kernel)
> (multiboot-arguments ,multiboot-arguments)
> (multiboot-modules ,multiboot-modules)))
> (($ <menu-entry> label device mount-point #f () #f #f () ()
> chain-loader)
> `(menu-entry (version 0)
> (label ,label)
> (device ,(device->sexp device))
> (device-mount-point ,mount-point)
> (chain-loader ,chain-loader))))
>
> --- <8 ---
I prefer this variant where the pattern is explicit.
As with what we have today, if the user specifies more than one of
linux, linux-multiboot and chainloader, they get an unhelpful "no
matching pattern" error.
This could be done later if you don't have time, but I would suggest to
fix it by adding a default case that matches all incorrect cases, like
so:
(_ (raise (condition (&message (message (G_ "Your error message
here"))))))
Have a look at other "&message" conditions for inspiration.
Also I noticed that if all of linux, linux-multiboot and chainloader
are #f, then the first pattern matches and will lead to a different
error message. I haven't tested so I'm not sure what we get, but it
might be interresting to match on all of them being #f, and print a
different message. Again, this can be done later.
>
> > I'm not too familiar with chainloading. Is using grub partition
> > names like (hd0,gpt1) the only way? We try to discourage using
> > these names as they can easily vary between reboots and your system
> > unbootable.
>
> It can also use device to specify the disk partition. The following is
> the menu-entry that I am using.
>
> --- >8 ---
>
> (menu-entries
> (list
> (menu-entry
> (label "ArchLinux")
> (device (uuid "1C31-A17C" 'fat))
> (chain-loader "/EFI/ArchLinux/grubx64.efi"))))
>
> --- <8 ---
>
> The examples in the document were written before the bug#57307 was
> fixed. At that time, only this example passed the test on my
> computer. I didn't take into account that the example was bad. I'm
> sorry.
This new example is perfect. Could you add it to your next patch?
>
> I'm sorry, because I don't know how to paste the code snippet in the
> mail. After seeing your reply, after a long search, no good example
> was found. I don't know if there is a problem when I write the code
> snippet like this. If there is any problem, I'm sorry.
They came out fine. I don't use anything fancy to read emails, so I
don't really care. I think emacs might have something to show snippets
of code better.
>
> Thank you very much for checking and correcting my patches.
Could you send a v2 with the changes we discussed so far?
Thanks,
Julien
>
> Thanks,
> tiantian
>
>
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.