Re: [PATCH 01/10] * gnu/system.scm (<boot-parameters>): Add 'store-device' and 'store-fs-mount-point'.

[Chris Marusich <cmmarusich@gmail.com>]

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

ludo@gnu.org (Ludovic Courtès) writes:

> General remark on commit logs: I can fix details there, but it would
> help me if you could more closely follow conventions.

Understood.  I read the manual ((standards) Change Logs) and tried to
make it look like existing commits, but I see I missed some stuff, so
I'll do better next time.  Thank you for pointing out the spots that
need improvement.

>>  (define-record-type* <boot-parameters>
>>    boot-parameters make-boot-parameters boot-parameters?
>>    (label            boot-parameters-label)
>> +  ;; Because we will use the 'store-device' to create the GRUB search command,
>> +  ;; the 'store-device' has slightly different semantics than 'root-device'.
>> +  ;; The 'store-device' can be a file system uuid, a file system label, or #f,
>> +  ;; but it cannot be a device path such as "/dev/sda3", since GRUB would not
>> +  ;; understand that.  The 'root-device', on the other hand, corresponds
>> +  ;; exactly to the device field of the <file-system> object representing the
>> +  ;; OS's root file system, so it might be a device path like "/dev/sda3".
>>    (root-device      boot-parameters-root-device)
>> +  (store-device     boot-parameters-store-device)
>> +  (store-fs-mount-point boot-parameters-store-fs-mount-point)
>
> In your patch series ‘store-fs-mount-point’ ends up being unused.

Is this true?  I think we're using it.  For example, in commit
18813be0053b4797071e20f373a8a9e8d669e32a (Implement switch-generation
and roll-back), in procedure 'reinstall-grub', don't we use it to get
the mount point for each <boot-parameters>?

> However, after rereading the whole series, I think what we need is a
> ‘device-mount-point’ in <menu-entry>, in a symmetrical fashion.

That would work, and I'm open to it.  However, before we decide to do
that, what do you think about the alternative in which we simply require
that all the paths in the <boot-parameters> and <menu-entry> objects
must be relative paths?  By relative, I mean relative to the GRUB root.
If all the paths were relative, then we would not need a
'device-mount-point' field in the first place.

However, I suspect it might be tricky to get all the information we need
if we do it that way.  For example, during switch-generation, how would
we know the correct paths to use for the files in the GRUB
configuration's 'eye-candy' section?  In light of that, I suspect it
would be better to do what you've suggested: include the mount point in
both <boot-parameters> and <menu-entry>.

> Attached is an updated patch.  I have grouped together the patches of
> your series that touch this topic, including the documentation part
> (this is all one logical change so it’s best to commit it as such), and
> made the above change.

Thank you for doing this.  I've looked it over.  It looks good to me.
Unless you think strongly that we should use relative paths in the
<boot-parameters> and <menu-entry> objects, I think it's good as-is!

> I’ve also followed my suggestion at
> <https://lists.gnu.org/archive/html/guix-devel/2016-10/msg00568.html> to
> not change the version of the <boot-parameters> sexp.

OK.  FYI, I bumped the version number in my patch series because I
thought it would simplify the matching logic (it was still backwards
compatible).  However, looking at your version, I see now that it wasn't
too hard to keep the version number the same.  Your version looks good
to me!

> I’ve tested that the generated grub.cfg is what we expect in several
> different scenario.

Thank you for taking the extra time to test it.

> If that’s fine with you, I’d like to commit this version.  With that
> done, the rest of the patch series will be rather easy.
>
> WDYT?
>

Sounds good to me.  Unless you think strongly that we should use
relative paths in the <boot-parameters> and <menu-entry> objects, I will
re-submit my patches once you have committed this change to the official
repo.

> Thank you!

Thank you for all of your help and tips!  I really appreciate it, and
it's all been very helpful.

-- 
Chris

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