[bug#72457] [PATCH v5 00/15] Rewrite bootloader subsystem.

[Herman Rimm via Guix-patches via <guix-patches@gnu.org>]

On Tue, Sep 17, 2024 at 05:20:05PM -0500, Lilah Tascheter wrote:
> > I would like to submit a rewritten patch series.  Basically, it would
> > consist of patches #4, #6, #12, #13, and #14.
> since efi's getting split out, would it make sense to split #6 out too?
> though, that could pose issues if it gets forgotten and the others get
> merged. your choice!
I would rather not adapt the existing Raspberry Pi bootloader to the new
system.

> > I want to submit #1, #2, #3, #5 and #15 to issue #73202
> so, #73202'd end up being a general cleanup of the current bootloader
> system, right? I feel #2 wouldn't quite fit there, seeing as it just
> adds the infastructure needed for #4.
#73202 is also preparation for this issue.  I think #2 is big enough for
it to reviewed on its own outside of this issue, and that #73202 is
small enough to fit #2.

> > #7, #8, #9, #10 (excl. efibootmgr) and #11 to #68524.
> I'll send an unmerge to #68524 then!
Thanks. 

> > Parts of patch #4 which fit better with #73202 or function standalone
> > would be submitted to #73202.  Finally, #4 will be split into seven
> > to ten patches, hopefully making referring to changes easier and
> > review less demanding.
> this sounds great! but,
> good fucking luck splitting up #4. a ton of the changes are
> interconnected, and it'll be a pain to do so if you don't want some
> commits to just not compile. if you can pull it off, that'd be amazing!
I don't intend for the commits to compile.  It's to adequately describe
the changes while fitting each commit message on a monitor and to give
reviewers the choice of squashing them together into one working commit.

> > [1]: https://codeberg.org/herman_rimm/guix
> typo in gnu/build/bootloader.scm "thtat", also
> gnu/system/install.scm(embedded-installation-os) operating-system-
> bootloader's default is '(), not #f. otherwise, this looks great!!!
Fixed, thanks.

> I'm also thinking now, since you mentioned the operating-system-
> bootloader sanitizer in a previous email, it'd probably be a good idea
> to expand the sanitizer to detect for 'part type targets too. a simple
> /dev/.*[0-9] regex should work well? I can write a quick patch up for
> you, or you can just include that when making the new patch series if
> you'd prefer?
I had stashed the changes I made to warn-update-targets.  I do try to
create branches and fixup commits instead to better track changes.
Anyway, maybe you can send a diff based on/relative to:

;; Based on report-duplicate-field-specifier from (guix records).
(define (report-duplicate-type-field targets)
  "Report the first target with duplicate type among TARGETS."
  (let loop ((targets targets)
             (seen    '()))
    (match targets
      ((target rest ...)
       (let (type (bootloader-target-type target)))
         (when (memq type seen)
           (error loc (G_ "target with duplicate type~%") duplicate))
         (loop rest (cons type seen)))
      (() #t))))

(define-with-syntax-properties (warn-update-targets (targets properties))
  (let ((targets (if (list? targets) targets (list targets)))
        (loc (source-properties->location properties)))
    (define string->target
      (match-lambda
        ((? bootloader-target? target) target)
        ((? string? s) (if (string-prefix? "/dev" s)
                           (bootloader-target
                             (type 'disk)
                             (device s))
                           (bootloader-target
                             (type 'esp)
                             (offset 'root)
                             (path s))))
        (x (error loc (G_ "invalid target '~a'~%") x))))

    ;; XXX: Should this be an error?
    (when (any string? targets)
      (warning loc (G_ "the 'targets' field should now contain \
<bootloader-target> records, inferring a best guess, this might break!~%")))
    (let* ((targets (map string->target targets)))
      (report-duplicate-type-field targets)
      targets)))

> > [2]: ...
> looks like the failure here was caused by cpan failing to build, which
> shouldn't be (hopefully isn't) a result of this patchset. what does the
> build log mentioned say?
I think because the build log was on a VFS I couldn't access it
directly.  But why do these packages need to be built at all?

> thanks so much for all the help, by the way :)
No problem.

Cheers,
Herman