diff mbox series

[bug#40579,RFC] add iPXE.

Message ID e7f84fcc-a555-4ef8-2f6b-1bf31a9496a7@gmail.com
State Superseded
Headers show
Series [bug#40579,RFC] add iPXE. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job

Commit Message

Vincent Legoll April 12, 2020, 5:59 p.m. UTC
Here is a patch for review that adds iPXE.


The licensing is "interesting", see:

https://ipxe.org/licensing

Is that a problem ?


There was no real install target in makefile, so I

added one, any hints on how to write that in a

cleaner way ?


I've put all firmware & rom images in $(out)/bin

Is there a better subdirectory to put such files ?


The double with-directory-excursion could be

handled with a single phase cd'ing into src, but I

can't find where I've seen that...


WDYT ?

Comments

ashish.is--- via Guix-patches" via April 12, 2020, 6:47 p.m. UTC | #1
Vincent,

Thank you!  Brief review, will build & maybe notice more later:

Vincent Legoll 写道:
> The licensing is "interesting", see:
>
> https://ipxe.org/licensing
>
> Is that a problem ?

Could you elaborate?  What's "interesting" about it?  That all 
looks very boring and straightforward to me (which is good! :-) — 
the result is GPL2-only, no?

+              (file-name (string-append name "-" version 
"-checkout"))

You can use the GIT-FILE-NAME helper here.

+     `(#:phases (modify-phases %standard-phases

Aside: I'd indent arguments' #:keywords as

+     `(#:phases
+       (modify-phases %standard-phases

to give you more breathing room at deeper indentation levels. 
It's
not needed now, but if someone were to add a new phase they might
                                                            have
                                                            to
                                                            do
                                                            annoying
                                                            things,
or re-indent the entire thing later, causing noise.  Maybe that's 
just me though.

+                  (add-after 'unpack 'add-real-make-install
+                    (lambda* (#:key outputs #:allow-other-keys)
+                      (substitute* "src/Makefile"
+                        (("^install :")
+                         (string-append "install :"
+                                        "\n\t@$(MKDIR) -p "
+                                        (assoc-ref outputs "out") 
"/bin"
+                                        "\n\t@$(CP) $(ALL) "
+                                        (assoc-ref outputs "out") 
"/bin"
+                                        "\n\n__old_install :")))

Interesting approach!  I'm OK with it; looking at ALL it wouldn't 
be more readable or future-proff to use FIND-FILES & Scheme.

/bin is not the right place for these files.  /lib/ipxe looks to 
be the standard; let's use that.

+                  (replace 'build
+                     (lambda _ (with-directory-excursion "src"
+                                 (invoke "make" "-j" 
(number->string
+ 
(parallel-job-count))))))

Let's, instead:

  (add-after 'unpack 'enter-source-directory
    (lambda _ (chdir "src") #t))

Don't worry, the state can't hurt you now.  Now we can keep the 
standard build & install phases.

It might be necessary to add a ‘leave-source-directory’ after 
'install to make sure the licence files are still installed to 
share/doc/.

+       #:tests? #f))
→       #:tests? #f))                    ; no test suite

+    (native-inputs

Nitpick: sort?  :-)

+    (synopsis "PXE-compliant network boot firmware")

I personally like the ‘these are just boot loaders’ angle, but 
would users expect to find this in (gnu packages firmware) 
instead?  Shrug.

+    (license license:gpl2+)))

‘gpl2’ as mentioned above.

If you feel like it (there aren't that many files) you could list 
the licences for each output binary, but that's optional.  The 
combined work appears to be GPL2.

Kind regards,

T G-R
ashish.is--- via Guix-patches" via April 12, 2020, 6:58 p.m. UTC | #2
Tobias Geerinckx-Rice 写道:
> Brief review, will build & maybe notice more later:

Such as: we don't ship static (.a) libraries in Guix unless 
something else (in Guix) needs them.  Can we delete the 5.6-MiB 
blib.a?

Kind regards,

T G-R
Danny Milosavljevic April 12, 2020, 7:46 p.m. UTC | #3
Hi,

On Sun, 12 Apr 2020 20:47:23 +0200
Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org> wrote:

> Could you elaborate?  What's "interesting" about it?  That all 
> looks very boring and straightforward to me (which is good! :-) — 
> the result is GPL2-only, no?

No, there are different makefile targets (rom output filenames) which will
result in different parts of ipxe to be built and included in the output.

There's special makefile targets for each of these targets to determine
the license that applies to each of the latter targets, respectively.

Unfortunately, there's no "all.license" target.

> /bin is not the right place for these files.  /lib/ipxe looks to 
> be the standard; let's use that.

I agree.

> Let's, instead:
> 
>   (add-after 'unpack 'enter-source-directory
>     (lambda _ (chdir "src") #t))
>
> Don't worry, the state can't hurt you now.  Now we can keep the 
> standard build & install phases.

I agree.

> If you feel like it (there aren't that many files) you could list 
> the licences for each output binary, but that's optional.  The 
> combined work appears to be GPL2.

Does it?  Did you try to make all of the targets' license targets?

P.S. The following ROMs are not reproducible:

10222000.rom GPL2
10500940.rom GPL2 
10ec8139.rom GPL2
15ad07b0.rom GPL2+
1af41000.rom GPL2+
8086100e.mrom GPL2+
8086100f.mrom GPL2+
808610d3.mrom GPL2+
80861209.rom GPL2+
ipxe.iso license checker doesn't work
rtl8139.rom GPL2

which are ALL of them except blib.a, ipxe.dsk, ipxe.lkrn, ipxe.pxe, ipxe.usb
and undionly.kpxe (this one has GPL2+) (which are not roms).

Note: Maybe also pass make-flags to overwrite those:

./Makefile.housekeeping:ECHO_E_BIN_ECHO         := /bin/echo
./Makefile.housekeeping:ECHO_E_BIN_ECHO_E       := /bin/echo -e
ashish.is--- via Guix-patches" via April 12, 2020, 9:18 p.m. UTC | #4
Hullo Danny,

Danny Milosavljevic 写道:
> Tobias Geerinckx-Rice via Guix-patches via 
> <guix-patches@gnu.org> wrote:
>
>> Could you elaborate?  What's "interesting" about it?  That all 
>> looks very boring and straightforward to me (which is good! :-) 
>> — 
>> the result is GPL2-only, no?
>
> No, there are different makefile targets (rom output filenames) 
> which will
> result in different parts of ipxe to be built and included in 
> the output.
>
> There's special makefile targets for each of these targets to 
> determine
> the license that applies to each of the latter targets, 
> respectively.

OK.  This was already clear.

I'm now convinced that this whole make target thing is a 
misunderstanding and a distraction: iPXE licencing is not, in any 
way, ‘generated on the fly by make rules’ or ‘unclear’ or 
‘interesting’.  It's boring.  It's good.  It's GPL2.

  λ grep -hr '^FILE_LICENCE ( .* );' * | sort -u
  FILE_LICENCE ( BSD2 );
  FILE_LICENCE ( BSD3 );
  FILE_LICENCE ( GPL2_ONLY );
  FILE_LICENCE ( GPL2_OR_LATER );
  FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
  FILE_LICENCE ( GPL_ANY );
  FILE_LICENCE ( MIT );
  FILE_LICENCE ( PUBLIC_DOMAIN );

Just a normal Free software package.  Happens to bundle a Perl 
script similar to <https://linux.die.net/man/1/licensecheck> (but 
less useful).  Nothing to see here.

> Did you try to make all of the targets' license targets?

No.  I hope I've shown they're irrelevant now.

What is relevant is that some files are missing licence headers 
(e.g. drivers/net/tg3/tg3_phy.c).  However, this is so common as 
to be standard (Guix wouldn't exist if we demanded headers in 
every file) and COPYING says that these are licenced under ‘the 
GPL’.  That's… acceptable.

> P.S. The following ROMs are not reproducible:
>
> 10222000.rom GPL2
> 10500940.rom GPL2 
> 10ec8139.rom GPL2
> 15ad07b0.rom GPL2+
> 1af41000.rom GPL2+
> 8086100e.mrom GPL2+
> 8086100f.mrom GPL2+
> 808610d3.mrom GPL2+
> 80861209.rom GPL2+
> ipxe.iso license checker doesn't work
> rtl8139.rom GPL2

Ooh, thanks for catching that.

I noticed that Debian installs a very different set of file( 
name)s[0] than we do.  I don't know what that means though.

Kind regards,

T G-R

[0]: https://packages.debian.org/sid/all/ipxe/filelist
     https://packages.debian.org/sid/all/ipxe-qemu/filelist
Vincent Legoll Jan. 14, 2021, 8:33 a.m. UTC | #5
Thanks a lot Danny !
diff mbox series

Patch

From d0fcf7a69a7503e8fb5b61a4c1109ebfed208a40 Mon Sep 17 00:00:00 2001
From: Vincent Legoll <vincent.legoll@gmail.com>
Date: Sun, 12 Apr 2020 19:54:47 +0200
Subject: [PATCH] gnu: Add iPXE.

* gnu/packages/bootloaders.scm (ipxe): New variable.
---
 gnu/packages/bootloaders.scm | 54 ++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/gnu/packages/bootloaders.scm b/gnu/packages/bootloaders.scm
index cadcc937e1..4b1f209540 100644
--- a/gnu/packages/bootloaders.scm
+++ b/gnu/packages/bootloaders.scm
@@ -11,6 +11,7 @@ 
 ;;; Copyright © 2019 nee <nee@cock.li>
 ;;; Copyright © 2019 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2020 Björn Höfling <bjoern.hoefling@bjoernhoefling.de>
+;;; Copyright © 2020 Vincent Legoll <vincent.legoll@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -834,6 +835,59 @@  to Novena upstream, does not load u-boot.img from the first partition.")
        `(("firmware" ,arm-trusted-firmware-rk3399)
          ,@(package-native-inputs base))))))
 
+(define-public ipxe
+  (package
+    (name "ipxe")
+    (version "1.20.1")
+    (source (origin
+              (method git-fetch)
+              (uri (git-reference
+                    (url "https://github.com/ipxe/ipxe")
+                    (commit (string-append "v" version))))
+              (file-name (string-append name "-" version "-checkout"))
+              (sha256
+               (base32
+                "0w7h7y97gj9nqvbmsg1zp6zj5mpbbpckqbbx7bpp6k3ahy5fk8zp"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:phases (modify-phases %standard-phases
+                  (add-after 'unpack 'add-real-make-install
+                    (lambda* (#:key outputs #:allow-other-keys)
+                      (substitute* "src/Makefile"
+                        (("^install :")
+                         (string-append "install :"
+                                        "\n\t@$(MKDIR) -p "
+                                        (assoc-ref outputs "out") "/bin"
+                                        "\n\t@$(CP) $(ALL) "
+                                        (assoc-ref outputs "out") "/bin"
+                                        "\n\n__old_install :")))
+                      #t))
+                  (delete 'configure)
+                  (replace 'build
+                     (lambda _ (with-directory-excursion "src"
+                                 (invoke "make" "-j" (number->string
+                                         (parallel-job-count))))))
+                  (replace 'install
+                     (lambda _ (with-directory-excursion "src"
+                                 (invoke "make" "install")))))
+       #:tests? #f))
+    (native-inputs
+     `(("perl" ,perl)
+       ("syslinux" ,syslinux)
+       ("cdrtools" ,cdrtools)
+       ("mtools" ,mtools)
+       ("xz" ,xz)))
+    (home-page "https://ipxe.org")
+    (synopsis "PXE-compliant network boot firmware")
+    (description "iPXE is a network boot firmware.  It provides a full PXE
+implementation enhanced with additional features such as booting from: a web
+server via HTTP, an iSCSI SAN, a Fibre Channel SAN via FCoE, an AoE SAN, a
+wireless network, a wide-area network, an Infiniband network.  It allows to
+control the boot process with a script.  You can use iPXE to replace the
+existing PXE ROM on your network card, or you can chainload into iPXE to obtain
+the features of iPXE without the hassle of reflashing.")
+    (license license:gpl2+)))
+
 (define-public vboot-utils
   (package
     (name "vboot-utils")
-- 
2.26.0