[PATCH] system: Move "--load" and other guix-specific parameters from the grub module to the generic system module.

(operating-system-parameters-file): Rename to ...

(operating-system-boot-parameters-file): ... this and split off ...

(operating-system-boot-parameters): ... new variable.

(operating-system-bootcfg): Use operating-system-boot-parameters.

(operating-system-directory-base-entries):

Use operating-system-boot-parameters-file instead of

operating-system-parameters-file.

(operating-system-all-kernel-arguments): New variable. Export it.

(operating-system-kernel-arguments): Remove export.

(read-boot-parameters-file): New variable. Export it.

(read-boot-parameters): Remove export.

(grub-configuration-file): Use boot-parameters->menu-entry.

operating-system-all-kernel-arguments.

(profile-boot-parameters): ... this and return boot-parameters directly.

(reinstall-grub): Use read-boot-parameters-file.

Use profile-boot-parameters instead of profile-grub-entries.

(display-system-generation): Use read-boot-parameters-file.

Use profile-boot-parameters instead of profile-grub-entries.

---

gnu/system.scm | 116 ++++++++++++++++++++++++++++++++----------------

gnu/system/grub.scm | 13 +++++-

gnu/system/vm.scm | 10 ++---

guix/scripts/system.scm | 56 +++++++----------------

4 files changed, 109 insertions(+), 86 deletions(-)

Hi Danny,

I'll review it more carefully tomorrow, but here are first comments :

Commented lines.

This line is way over 80 chars.

Thanks,

Mathieu

Hi Danny,

Another quick preliminary review…

Could you explain the “big picture”, the rationale behind this change?

Danny Milosavljevic <dannym@scratchpost.org> skribis:

Please add a docstring to top-level procedures:

https://www.gnu.org/software/guix/manual/html_node/Formatting-Code.html

For services, we have:

operating-system-user-services

operating-system-services

So I suggest stick to this convention and thus have:

operating-system-user-kernel-arguments ;arguments specified by the user

operating-system-kernel-arguments ;all the arguments

I think it would be clearer to rename in a separate patch.

Please use full words in variable names, typically ‘system’ here.

Thanks,

Ludo’.

Hi Ludo,

Currenly, (gnu system grub) prepends the "--system", "--load" and "--root" on its own. But that's something specific to the guix system and has nothing to do with bootloaders (it's not even a bootloader option - it's just passing it through to the Linux kernel and *that* is passing some through to Shepherd).

In connection with the effort to support other bootloaders we could of course copy such a prepension-block to each of the bootloader modules, but in my opinion it's better if it's moved to (bootloader-independent) boot-parameters so everyone just automatically uses the correct kernel arguments, including which guix system guix should boot.

I suspect that this wasn't done before because the boot-parameters are serialized to a file "parameters" in the "system" directory. But then "--system=<system>" in that file would have to contain the hash value of the system, which is ... impossible (or at least very very hard to do).

Therefore, this patch makes sure the in-memory <boot-parameters> instances do contain "--system" with the correct hash value, but the stored "parameters" doesn't.

I think that that's the best of both worlds. Clients in guix can use data read from <boot-parameters> without second-guessing everything and the stored file still doesn't have to solve a hash-functions-really-try-to-prevent-you-from-doing-that problem - the stored file doesn't contain the hash value of itself (and now doesn't need to).

I've successfully tested it by "guix system reconfigure" on my machine and also by "guix system vm" (full-boot and non-full-boot) on my machine, starting from master.

Something like this?

"Prepend extra arguments to KERNEL-ARGUMENTS that allow the guix system to boot SYSTEM on ROOT-DEVICE."

Hmm, I don't think it's really directly specified by the user, is it? I'm just trying to avoid having to store the file's (more or less) own hash value into the file.

I'll try.

Ok. Should the docstring say "SYSTEM (system or generation)" or just "SYSTEM"?

Hello!

Danny Milosavljevic <dannym@scratchpost.org> skribis:

OK, that makes a lot of sense, indeed.

OK, I see. I’ll look more closely at the patch but I think it’s fine to

treat --system specially, for the reasons you gave.

Yeah, something like that (“that allow SYSTEM to be botted from

ROOT-DEVICE” maybe.)

I’ll have to check.

The former is clearer IMO.

Thank you,

Ludo’.

(operating-system-boot-parameters-file): ... this.

(operating-system-directory-base-entries): Adapt call site.

---

gnu/system.scm | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

(operating-system-user-kernel-arguments): ... this.

(<operating-system>): Adapt accordingly.

(operating-system-bootcfg): Adapt accordingly.

(operating-system-parameters-file): Adapt accordingly.

accordingly.

---

gnu/system.scm | 8 ++++----

gnu/system/vm.scm | 2 +-

2 files changed, 5 insertions(+), 5 deletions(-)

Danny Milosavljevic (8):

system: Rename operating-system-kernel-arguments to

operating-system-user-kernel-arguments.

system: Rename operating-system-parameters-file to

operating-system-boot-parameters-file.

system: Factorize operating-system-boot-parameters-file.

system: Introduce operating-system-kernel-arguments and use it.

system: Introduce read-boot-parameters-file.

system: vm: Use operating-system-kernel-arguments.

system: Use operating-system-boot-parameters directly.

system: grub: Use boot-parameters instead of menu-entry where

possible.

gnu/system.scm | 133 +++++++++++++++++++++++++++++++-----------------

gnu/system/grub.scm | 14 ++++-

gnu/system/vm.scm | 7 +--

guix/scripts/system.scm | 24 ++++-----

4 files changed, 110 insertions(+), 68 deletions(-)

(operating-system-boot-parameters-file): Modify.

---

gnu/system.scm | 61 ++++++++++++++++++++++++++++++++++++++--------------------

1 file changed, 40 insertions(+), 21 deletions(-)

(operating-system-kernel-arguments): New variable.

(operating-system-bootcfg): Use operating-system-kernel-arguments.

(operating-system-boot-parameters): Use operating-system-kernel-arguments.

---

gnu/system.scm | 30 ++++++++++++++++++++++++++----

1 file changed, 26 insertions(+), 4 deletions(-)

(read-boot-parameters-file): New variable. Export it.

read-boot-parameters-file.

(profile-grub-entries): Use read-boot-parameters-file.

(reinstall-grub): Use read-boot-parameters-file.

(display-system-generation): Use read-boot-parameters-file.

---

gnu/system.scm | 22 +++++++++++++++++++++-

guix/scripts/system.scm | 10 ++++------

2 files changed, 25 insertions(+), 7 deletions(-)

Use operating-system-kernel-arguments.

---

gnu/system/vm.scm | 7 ++-----

1 file changed, 2 insertions(+), 5 deletions(-)

operating-system-boot-parameters directly.

---

gnu/system.scm | 28 ++++------------------------

1 file changed, 4 insertions(+), 24 deletions(-)

(grub-configuration-file): Use boot-parameters

instead of menu-entry where possible.

(reinstall-grub): Use profile-boot-parameters.

(perform-action): Use profile-boot-parameters.

---

gnu/system/grub.scm | 14 ++++++++++++--

guix/scripts/system.scm | 22 +++++++++-------------

2 files changed, 21 insertions(+), 15 deletions(-)

Hi Danny,

Most of the serie LGTM.

However, I noticed that the grub menuentries in /boot/grub/grub.cfg

produced during a reconfigure are loosing their labels (#1, ...).

Thanks,

Mathieu

Danny Milosavljevic writes:

LGTM!

Danny Milosavljevic writes:

LGTM!

Danny Milosavljevic writes:

This line is too long.

Otherwise LGTM !

You could precise in this docstring that system is a derivation.

Mathieu

LGTM!

profile-grub-entries is now unsued, this serie could remove it.

LGTM!

LGTM!

Danny Milosavljevic writes:

Danny Milosavljevic (9):

system: Rename operating-system-parameters-file to

operating-system-boot-parameters-file.

system: Factorize operating-system-boot-parameters-file.

system: Introduce operating-system-kernel-arguments and use it.

system: Introduce read-boot-parameters-file.

scripts: Make label include generation number and time.

system: vm: Use operating-system-kernel-arguments.

system: Use operating-system-boot-parameters directly.

system: grub: Use boot-parameters instead of menu-entry where

possible.

scripts: Remove profile-grub-entries.

gnu/system.scm | 133 +++++++++++++++++++++++++++++++-----------------

gnu/system/grub.scm | 14 ++++-

gnu/system/vm.scm | 7 +--

guix/scripts/system.scm | 64 +++++------------------

4 files changed, 114 insertions(+), 104 deletions(-)

(operating-system-boot-parameters-file): ... this.

(operating-system-directory-base-entries): Adapt call site.

---

gnu/system.scm | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

(operating-system-boot-parameters-file): Modify.

---

gnu/system.scm | 64 +++++++++++++++++++++++++++++++++++++++-------------------

1 file changed, 43 insertions(+), 21 deletions(-)

(operating-system-kernel-arguments): New variable.

(operating-system-bootcfg): Use operating-system-kernel-arguments.

(operating-system-boot-parameters): Use operating-system-kernel-arguments.

---

gnu/system.scm | 25 ++++++++++++++++++++++---

1 file changed, 22 insertions(+), 3 deletions(-)

(read-boot-parameters-file): New variable. Export it.

read-boot-parameters-file.

(profile-grub-entries): Use read-boot-parameters-file.

(reinstall-grub): Use read-boot-parameters-file.

(display-system-generation): Use read-boot-parameters-file.

---

gnu/system.scm | 22 +++++++++++++++++++++-

guix/scripts/system.scm | 14 ++++----------

2 files changed, 25 insertions(+), 11 deletions(-)

generation number and time.

---

guix/scripts/system.scm | 6 +++++-

1 file changed, 5 insertions(+), 1 deletion(-)

Use operating-system-kernel-arguments.

---

gnu/system/vm.scm | 7 ++-----

1 file changed, 2 insertions(+), 5 deletions(-)

operating-system-boot-parameters directly.

---

gnu/system.scm | 28 ++++------------------------

1 file changed, 4 insertions(+), 24 deletions(-)

(grub-configuration-file): Use boot-parameters

instead of menu-entry where possible.

(reinstall-grub): Use profile-boot-parameters.

(perform-action): Use profile-boot-parameters.

---

gnu/system/grub.scm | 14 ++++++++++++--

guix/scripts/system.scm | 11 ++++++-----

2 files changed, 18 insertions(+), 7 deletions(-)

---

guix/scripts/system.scm | 37 -------------------------------------

1 file changed, 37 deletions(-)

(operating-system-user-kernel-arguments): ... this.

(<operating-system>): Adapt accordingly.

(operating-system-bootcfg): Adapt accordingly.

(operating-system-parameters-file): Adapt accordingly.

accordingly.

---

gnu/system.scm | 8 ++++----

gnu/system/vm.scm | 2 +-

2 files changed, 5 insertions(+), 5 deletions(-)

Danny Milosavljevic (10):

system: Rename operating-system-kernel-arguments to

operating-system-user-kernel-arguments.

system: Rename operating-system-parameters-file to

operating-system-boot-parameters-file.

system: Factorize operating-system-boot-parameters-file.

system: Introduce operating-system-kernel-arguments and use it.

system: Introduce read-boot-parameters-file.

scripts: Make boot-parameters label include generation number and

time.

system: vm: Use operating-system-kernel-arguments.

system: Use operating-system-boot-parameters directly.

system: grub: Use boot-parameters instead of menu-entry where

possible.

scripts: Remove profile-grub-entries.

gnu/system.scm | 133 +++++++++++++++++++++++++++++++-----------------

gnu/system/grub.scm | 14 ++++-

gnu/system/vm.scm | 7 +--

guix/scripts/system.scm | 64 +++++------------------

4 files changed, 114 insertions(+), 104 deletions(-)

(operating-system-boot-parameters-file): ... this.

(operating-system-directory-base-entries): Adapt call site.

---

gnu/system.scm | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

(operating-system-boot-parameters-file): Modify.

---

gnu/system.scm | 64 +++++++++++++++++++++++++++++++++++++++-------------------

1 file changed, 43 insertions(+), 21 deletions(-)

(operating-system-kernel-arguments): New variable.

(operating-system-bootcfg): Use operating-system-kernel-arguments.

(operating-system-boot-parameters): Use operating-system-kernel-arguments.

---

gnu/system.scm | 25 ++++++++++++++++++++++---

1 file changed, 22 insertions(+), 3 deletions(-)

(read-boot-parameters-file): New variable. Export it.

read-boot-parameters-file.

(profile-grub-entries): Use read-boot-parameters-file.

(reinstall-grub): Use read-boot-parameters-file.

(display-system-generation): Use read-boot-parameters-file.

---

gnu/system.scm | 22 +++++++++++++++++++++-

guix/scripts/system.scm | 14 ++++----------

2 files changed, 25 insertions(+), 11 deletions(-)

generation number and time.

---

guix/scripts/system.scm | 9 +++++++--

1 file changed, 7 insertions(+), 2 deletions(-)

Use operating-system-kernel-arguments.

---

gnu/system/vm.scm | 7 ++-----

1 file changed, 2 insertions(+), 5 deletions(-)

operating-system-boot-parameters directly.

---

gnu/system.scm | 28 ++++------------------------

1 file changed, 4 insertions(+), 24 deletions(-)

(grub-configuration-file): Use boot-parameters

instead of menu-entry where possible.

(reinstall-grub): Use profile-boot-parameters.

(perform-action): Use profile-boot-parameters.

---

gnu/system/grub.scm | 14 ++++++++++++--

guix/scripts/system.scm | 8 ++++----

2 files changed, 16 insertions(+), 6 deletions(-)

---

guix/scripts/system.scm | 37 -------------------------------------

1 file changed, 37 deletions(-)

Thanks for v3 and v4 !

It seems to me that my remarks have been addressed, except for the

old-entries label ? I can't found a patch fixing this issue.

I know that it is not common on this list, but a brief changelog for

each serie would be nice :)

Thanks,

Mathieu

Danny Milosavljevic writes:

Hi Mathieu,

On Fri, 21 Apr 2017 15:04:48 +0200

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

PATCH v4 06/10 should also fix it. Doesn't it? I tested it here and it seems fine.

Or do I misunderstand which problem you mean?

I can do that for large patchsets such as this one.

Personally, I diff the diffs in such cases (i.e. "diff -u v3_a.patch v4_a.patch" etc). It's a bit jarring at first but it helps :)

Hi,

Nope, I just didn't received v4 06/10 when I wrote that, sorry.

I just tested v4 serie (reconfigure, list-generation, switch-generation

and vm), everything seems fine for me !

Thanks,

Mathieu

Applied this patch to master as af98d25a1286e246e8da36d6d63b4d66e58f2cf8 and pushed.

Applied this patch to master as 71d04202026e2061f898a142a8381d55bee5fb00 and pushed.

Applied this patch to master as af98d25a1286e246e8da36d6d63b4d66e58f2cf8 and pushed.

Applied this patch to master as 71d04202026e2061f898a142a8381d55bee5fb00 and pushed.

I'll hold off merging the other patches in the series until more time for review has passed (and more testing has been done).

I've merged the first two patches since they are just self-contained renames we have to do anyway.

If you have the time (and a way to recover a potential non-booting system - although it works for Mathieu and me), please test.

On Fri, 21 Apr 2017 14:37:07 +0200

Danny Milosavljevic <dannym@scratchpost.org> wrote:

Might make sense to call SYSTEM "SYSTEM.DRV" instead - since it's either a derivation or #f.

Might want to elaborate on how SYSTEM is optional (#f stands for "not given").

SYSTEM is not used anywhere in the procedure. The next patch rectifies that - but I didn't feel like messing up the function signature just to fix it up one patch later. But even after only patch 03/10 you can give SYSTEM (and patch 03/10 operating-system-boot-parameters-file does that already) - it will just be ignored.

On Fri, 21 Apr 2017 14:37:10 +0200

Danny Milosavljevic <dannym@scratchpost.org> wrote:

Every time I see that I wonder whether I don't make some other part (like read-boot-parameters-file or something) to do the label extension. In theory that would be nicer than fixing it up later - I do the latter here.

And on first sight, one might be able to do that for the old generations (because the timestr in the label is just the mtime of the system directory). But the label should also contain the generation number - which is currently usually being extracted by the procedure "generation-numbers" for an entire profile (but the caller of system also is able to override the generation numbers of profile-boot-parameters and profile-grub-entries - not sure why); I don't find generation-numbers exactly straightforward - I'd rather not mess with it. Also, the caller zips the result together with SYSTEMS somehow and it would be easy for me to mess up the association.

Therefore, I opted for this version - which is a lot less risky.

If someone is more familiar with generation-numbers and knows why it's possible to override the generation numbers (usually by a one-element list), please feel free to post a patch that integrates this part into read-boot-parameters-file later.

Note: The current generation doesn't need its label extended.

I've been testing this for quite some time now and will be pushing it in a few hours if there are no objections.

Note: If you get a guile backtrace like the following on "guix system reconfigure", you should run "make" in guix. It will work just fine then.

Other than that, it's uneventful.

...

In ice-9/r4rs.scm:

39: 8 [call-with-values #<procedure b7e74b0 at ice-9/eval.scm:416:20 ()> ...]

39: 7 [call-with-values #<procedure 39ff210 at ice-9/eval.scm:416:20 ()> ...]

39: 6 [call-with-values #<procedure 39ff1b0 at ice-9/eval.scm:416:20 ()> ...]

39: 5 [call-with-values #<procedure bdda1b0 at ice-9/eval.scm:416:20 ()> ...]

39: 4 [call-with-values #<procedure bdda150 at ice-9/eval.scm:416:20 ()> ...]

In ice-9/eval.scm:

386: 3 [eval #<memoized ((<3> <1>) <0>)> (# # # ...)]

387: 2 [eval # #]

387: 1 [eval # #]

In unknown file:

?: 0 ["GNU with Linux-Libre 4.11 (beta)" "GNU with Linux-Libre 4.11 (beta)"]

ERROR: In procedure GNU with Linux-Libre 4.11 (beta):

ERROR: Wrong type to apply: "GNU with Linux-Libre 4.11 (beta)"

The guile that guix used is:

/gnu/store/k1bmz7glblfxdgaaas12q707lpjjk4mz-profile/bin/guile --no-auto-compile

guile 2.0.14

To reproduce, there all the ".go" files from guix, then invoke "guix system reconfigure ...". It will fail with the above.

When you then run "make", wait until it's done, then invoke "guix system reconfigure ...", it will work.

Hello!

Danny Milosavljevic <dannym@scratchpost.org> skribis:

Awesome. If “make check-system” doesn’t show anything bad either, then

that’d good.

Thanks,

Ludo’.

I've merged this patchset to master: [PATCH v4 00/10] system: Move "--load" and other guix-specific parameters from the grub module to the generic system module.

Please make sure to run "make" in guix after pulling.