Re: [PATCH] gnu: Add u-boot.

[Ludovic Courtès]

David Craven <address@hidden> skribis:

> From: Danny Milosavljevic <address@hidden>
>
> * gnu/packages/u-boot.scm (u-boot, make-u-boot-package,
>   u-boot-vexpress_ca9x4, u-boot-malta armhf-linux-uboot,
>   mips64el-linux-uboot): New variables.
>
> Co-authored-by: David Craven <address@hidden>

Cool, thanks for picking it up!

Overall this LGTM.  I guess Danny’s comments should be taken into
account.  Other than that, I only have cosmetic suggestions:

> +    (synopsis "ARM Universal Bootloader")

“ARM bootloader” (or “ARM universal bootloader” if you insist).

> +    (description "U-Boot is an universal bootloader mostly used for ARM 
> boards.
> +It also initializes the boards (RAM etc).")

I also think “universal” can be removed.

> +(define (make-u-boot-package board triplet xgcc)

A docstring would be nice here.

I think you could remove the ‘xgcc’ parameter and simply do:

  (let ((xgcc (cross-gcc triplet)))
    (package
       …))

> +           (lambda* (#:key outputs make-flags #:allow-other-keys)
> +             (let ((configname (string-append ,board "_defconfig")))

Should be ‘config-name’ per our conventions, but ‘config’ is probably
enough.

> +               (if (file-exists? (string-append "configs/" configname))
> +                   (zero? (apply system* "make" `(,@make-flags ,configname)))
> +                   (begin
> +                     (display "Invalid boardname. Valid boardnames would 
> have been:")
> +                     (newline)
> +                     (system* "ls" "-1" "configs")
> +                     #f)))))

“board name” (two words).

> +         (replace 'install
> +           (lambda* (#:key outputs make-flags #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out"))
> +                    (libexec (string-append out "/libexec"))
> +                    (uboot-files (find-files "." ".*\\.(bin|efi)$")))
> +               (mkdir-p libexec)
> +               (for-each
> +                (lambda (file-path)
> +                  (let ((target-file-path (string-append libexec "/" 
> file-path)))
> +                    (mkdir-p (dirname target-file-path))
> +                    (copy-file file-path target-file-path)))
> +                uboot-files)))))))))

s/-path//                

> +
> +(define-public u-boot-vexpress_ca9x4
> +  (make-u-boot-package "vexpress_ca9x4" "arm-linux-gnueabihf" xgcc-armhf))
> +
> +(define-public u-boot-malta
> +  (make-u-boot-package "malta" "mips64el-linux-gnuabi64" xgcc-mips64el))
> +
> +(define-public armhf-linux-uboot
> +  u-boot-vexpress_ca9x4)
> +
> +(define-public mips64el-linux-uboot
> +  u-boot-malta)

I think these two aliases can be removed, no?

Thanks!

Ludo’.