diff mbox series

[bug#54069,v2,3/4] gnu: libosinfo: Unbundle usb.ids, pci.ids.

Message ID 20220226043238.2657-3-mail@brendan.scot
State Accepted
Headers show
Series [bug#54069,v2,1/4] gnu: Add hwdata. | expand

Checks

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

Commit Message

Brendan Tildesley Feb. 26, 2022, 4:32 a.m. UTC
* gnu/packages/virtualization.scm (libosinfo):
[arguments]: Use Gexp style. Use hwdata package for latest usb.ids and
pci.ids.
[inputs]: Add hwdata:pci, hwdata:usb. remove pci.ids, usb.ids origins.
Use new input style.
---
 gnu/packages/virtualization.scm | 64 ++++++++++++++-------------------
 1 file changed, 26 insertions(+), 38 deletions(-)

Comments

M Feb. 26, 2022, 11:21 a.m. UTC | #1
Brendan Tildesley schreef op za 26-02-2022 om 15:32 [+1100]:
> +         (string-append "-Dwith-usb-ids-path=" #$hwdata:usb "/share/hwdata/usb.ids")
> +         (string-append "-Dwith-pci-ids-path=" #$hwdata:pci "/share/hwdata/pci.ids"))

To allow for package transformations, I recommend using
this-package-input or the like instead of hardcoding a particular
package:

#~(list
    (string-append
      "-Dwith-usb-ids-path="
      #$(this-package-input "hwdata:usb") "/share/hwdata/usb.ids"))
    [...])

it might also be possible to use 'search-input-file' instead, avoiding
input labels (untested):

#~(let* ((inputs #$(input-tuples->gexp (package-inputs this-package)))
         (usb.ids (search-input-file inputs "/share/hwdata/usb.ids"))
         (pci.ids (search-input-file inputs "/share/hwdata/pci.ids")))
    (list (string-append "-Dwith-usb-ids-path=" usb.ids) [...]))

Greetings,
Maxime.
Brendan Tildesley Feb. 27, 2022, 1:37 a.m. UTC | #2
> On 02/26/2022 12:21 PM Maxime Devos <maximedevos@telenet.be> wrote:
> 
>  
> Brendan Tildesley schreef op za 26-02-2022 om 15:32 [+1100]:
> > +         (string-append "-Dwith-usb-ids-path=" #$hwdata:usb "/share/hwdata/usb.ids")
> > +         (string-append "-Dwith-pci-ids-path=" #$hwdata:pci "/share/hwdata/pci.ids"))
> 
> To allow for package transformations, I recommend using
> this-package-input or the like instead of hardcoding a particular
> package:
> 
> #~(list
>     (string-append
>       "-Dwith-usb-ids-path="
>       #$(this-package-input "hwdata:usb") "/share/hwdata/usb.ids"))
>     [...])
> 
> it might also be possible to use 'search-input-file' instead, avoiding
> input labels (untested):
> 
> #~(let* ((inputs #$(input-tuples->gexp (package-inputs this-package)))
>          (usb.ids (search-input-file inputs "/share/hwdata/usb.ids"))
>          (pci.ids (search-input-file inputs "/share/hwdata/pci.ids")))
>     (list (string-append "-Dwith-usb-ids-path=" usb.ids) [...]))
> 
I got strange errors trying that and couldn't figure out how to make it work.
There are no examples of input-tuples->gexp being used in a package definition.

This works:
 #~(list
         (string-append "-Dwith-usb-ids-path="
                        (search-input-file %build-inputs "/share/hwdata/usb.ids"))
         (string-append "-Dwith-pci-ids-path="
                        (search-input-file %build-inputs "/share/hwdata/pci.ids")))

I thought %build-inputs was not a recommended thing to use though?
> Greetings,
> Maxime.




Backtrace:
In guix/store.scm:
   1385:9 19 (map/accumulate-builds #<store-connection 256.99 7f3a3…> …)
   1320:8 18 (call-with-build-handler #<procedure 7f3a22f4b360 at g…> …)
In guix/scripts/build.scm:
   582:16 17 (_ #<package libosinfo@1.9.0 /home/b/code/guix/gnu/pack…>)
   571:24 16 (_ #<package libosinfo@1.9.0 /home/b/code/guix/gnu/pac…> …)
In guix/packages.scm:
  1260:17 15 (supported-package? #<package libosinfo@1.9.0 /home/b/…> …)
In guix/memoization.scm:
    101:0 14 (_ #<hash-table 7f3a2538e7a0 0/31> #<package libosinfo…> …)
In guix/packages.scm:
  1238:37 13 (_)
  1498:16 12 (package->bag _ _ _ #:graft? _)
  1603:43 11 (thunk)
In ice-9/eval.scm:
   191:35 10 (_ #(#(#<directory (gnu packages virtualization) 7f…>) …))
   173:47  9 (_ #(#(#<directory (gnu packages virtualization) 7f…>) …))
   213:29  8 (_ #(#(#<directory (gnu packages virtualization) 7f…>) …))
    155:9  7 (_ #(#(#<directory (gnu packages virtualization) 7f…>) …))
   202:35  6 (_ #(#(#<directory (gnu packages virtualization) 7f…>) …))
In guix/gexp.scm:
   1886:4  5 (input-tuples->gexp (("glib" #<package glib@2.70…> …) …) …)
In srfi/srfi-1.scm:
   586:17  4 (map1 (("glib" #<package glib@2.70.2 gnu/packages…> …) …))
In guix/gexp.scm:
   1886:9  3 (_ _)
In ice-9/boot-9.scm:
  1685:16  2 (raise-exception _ #:continuable? _)
  1685:16  1 (raise-exception _ #:continuable? _)
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
Throw to key `match-error' with args `("match" "no matching pattern" ("glib" #<package glib@2.70.2 gnu/packages/glib.scm:180 7f3a24fdbdc0> "bin"))'.
M Feb. 27, 2022, 10:46 a.m. UTC | #3
Brendan Tildesley schreef op zo 27-02-2022 om 02:37 [+0100]:
> I got strange errors trying that and couldn't figure out how to make it work.
> There are no examples of input-tuples->gexp being used in a package definition.
> 
> This works:
>  #~(list
>          (string-append "-Dwith-usb-ids-path="
>                         (search-input-file %build-inputs "/share/hwdata/usb.ids"))
>          (string-append "-Dwith-pci-ids-path="
>                         (search-input-file %build-inputs "/share/hwdata/pci.ids")))
> 
> I thought %build-inputs was not a recommended thing to use though?

It is.  Anyway, I found a solution, see attachement.

Maybe the 'gexp-input-compiler' could be moved into (guix gexp)
and 'this-package-input-list'/'this-package-native-input-list' could
be moved into (guix packages) (+ documented in the manual)?

CC'ing ludo for the proposed new gexpology.

Greetings,
Maxime.
M Feb. 27, 2022, 10:56 a.m. UTC | #4
Maxime Devos schreef op zo 27-02-2022 om 11:46 [+0100]:
> 
> It is.  Anyway, I found a solution, see attachement.

Oops forgot the attachement


> Maybe the 'gexp-input-compiler' could be moved into (guix gexp)
> and 'this-package-input-list'/'this-package-native-input-list' could
> be moved into (guix packages) (+ documented in the manual)?
> 
> CC'ing ludo for the proposed new gexpology.
> 
> Greetings,
> Maxime.
(use-modules (guix packages) (gnu packages) (gnu packages gtk) (gnu packages glib) (guix gexp)
             (gnu packages gnome) (gnu packages xml) (gnu packages virtualization)
             (gnu packages pkg-config) (guix git-download) (guix build-system gnu)
             (guix build-system meson) (gnu packages compression) (ice-9 match)
             (guix monads) (guix store) (guix derivations)
             ((guix licenses) #:prefix license:))


(define <gexp-input> (@@ (guix gexp) <gexp-input>))
(define-gexp-compiler gexp-input-compiler <gexp-input>
  compiler => (lambda (input system target)
                (lower-object (gexp-input-thing input) system
                              #:target
                              (and (not (gexp-input-native? input)) target)))
  expander => (lambda (object lowered output)
                (derivation->output-path lowered (gexp-input-output object))))

 
(define-public hwdata
  (package
    (name "hwdata")
    (version "0.356")
    (source (origin
              (method git-fetch)
              (uri (git-reference
                    (url "https://github.com/vcrhonek/hwdata")
                    (commit (string-append "v" version))))
              (file-name (git-file-name name version))
              (sha256
               (base32
                "0m04d93dwiplwp9v74nhnc0hyi2n007mylkg8f0frb46z5qjrpl3"))))
    (build-system gnu-build-system)
    (outputs '("out" "iab" "oui" "pci" "pnp" "usb"))
    (native-inputs (list gzip))
    (arguments
     ;; Tests require pciutils, python, podman. Disable to avoid recursive dep.
     (list
      #:tests? #f
      #:configure-flags #~(list (string-append "--datadir=" #$output "/share"))
      #:phases
      #~(modify-phases %standard-phases
          (replace 'install
            (lambda _
              (install-file "iab.txt" (string-append #$output:iab "/share/hwdata"))
              (install-file "oui.txt" (string-append #$output:oui "/share/hwdata"))
              (install-file "pci.ids" (string-append #$output:pci "/share/hwdata"))
              (install-file "pnp.ids" (string-append #$output:pnp "/share/hwdata"))
              (install-file "usb.ids" (string-append #$output:usb "/share/hwdata")))))))
    (home-page "https://github.com/vcrhonek/hwdata")
    (synopsis "Hardware identification and configuration data")
    (description "@code{hwdata} contains various hardware identification and
 configuration data, such as the @file{pci.ids} and @file{usb.ids} databases.")
    (license (list license:gpl2+
                   license:expat)))) ;XFree86 1.0


(define* (package-input-list->build-input-list inputs native?)
  "Expand INPUTS, a list of input tuples, into a list of name/<gexp-input>
tulples."
  #~'#$(map (match-lambda
              ((name object output)
               #~(#$name . #$(gexp-input object output #:native? native?)))
              ((name object) #~(#$name . #$object)))
            inputs))

(define-syntax this-package-input-list
  (identifier-syntax
    (package-input-list->build-input-list
      (append (package-inputs this-package)
              (package-propagated-inputs this-package))
      #false)))

(define-syntax this-package-native-input-list
  (identifier-syntax
    (package-input-list->build-input-list
      (package-native-inputs this-package) #true)))

(define libosinfo-new
  (package
    (inherit (specification->package "libosinfo"))
    (arguments
      (list #:configure-flags
            #~(let* ((inputs #$this-package-native-input-list)
                     (usb.ids (search-input-file inputs "/share/hwdata/usb.ids"))
                     (pci.ids (search-input-file inputs "/share/hwdata/pci.ids")))
                (list (string-append "-Dwith-usb-ids-path=" usb.ids)
                      (string-append "-Dwith-pci-ids-path=" pci.ids)))
            #:phases
            #~(modify-phases %standard-phases
                (add-after 'unpack 'patch-osinfo-path
                  (lambda* (#:key inputs #:allow-other-keys)
                    (substitute* "osinfo/osinfo_loader.c"
                      (("path = DATA_DIR.*")
                       (string-append "path = \"" (search-input-directory inputs "/share/osinfo") "\";"))))))))
    (inputs
      (list libsoup-minimal-2 libxml2 libxslt osinfo-db))
    (native-inputs
      (list `(,glib "bin")
            gobject-introspection
            gtk-doc/stable
            vala
            intltool
            pkg-config
            `(,hwdata "pci")
            `(,hwdata "usb")))))
libosinfo-new
M Feb. 27, 2022, 12:12 p.m. UTC | #5
Brendan Tildesley schreef op za 26-02-2022 om 15:32 [+1100]:
> +           `(,hwdata "pci")
> +           `(,hwdata "usb")))

I would put these in 'inputs', since they are kept in the closure,
and I'd like to eventually automatically implicitely set #:allowed-
references to make sure only (non-native) inputs make their way into
the references, to catch some cross-compilation bugs early.

It doesn't truly matter here though, since hwdata is architecture-
independent.

Greetings,
Maxime.
Ludovic Courtès Feb. 27, 2022, 9:23 p.m. UTC | #6
Hello!

Maxime Devos <maximedevos@telenet.be> skribis:

> (define libosinfo-new
>   (package
>     (inherit (specification->package "libosinfo"))
>     (arguments
>       (list #:configure-flags
>             #~(let* ((inputs #$this-package-native-input-list)
>                      (usb.ids (search-input-file inputs "/share/hwdata/usb.ids"))
>                      (pci.ids (search-input-file inputs "/share/hwdata/pci.ids")))
>                 (list (string-append "-Dwith-usb-ids-path=" usb.ids)
>                       (string-append "-Dwith-pci-ids-path=" pci.ids)))

I think this is equivalent to:

  #~(let* ((inputs #$(input-tuples->gexp (package-inputs this-package)))
           …)
      …)

Am I right?

However, that looks a bit far-fetched to me.

Can’t we instead do:

  #~(let* ((usb.ids (string-append #$hwdata:usb "/share/hwdata/usb.ids"))
           (pci.ids (string-append #$hwdata:pci "/share/hwdata/pci.ids")))
      …)

?

Ludo’.
M Feb. 27, 2022, 10:17 p.m. UTC | #7
Ludovic Courtès schreef op zo 27-02-2022 om 22:23 [+0100]:
> Can’t we instead do:
> 
>   #~(let* ((usb.ids (string-append #$hwdata:usb "/share/hwdata/usb.ids"))
>            (pci.ids (string-append #$hwdata:pci "/share/hwdata/pci.ids")))
>       …)
> 
> ?

We could, and that's what the original v2 patch did.
However, this prevents package transformations:

  (package
    (inherit libosinfo)
    (native-inputs
      ;; not sure about the syntax 
      (modify-inputs (package-inputs libosinfo)
        (replace "hwdata:usb"
          hwdata-with-some-new-fancy-hardware "usb"))))

Greetings,
Maxime.
M Feb. 27, 2022, 10:18 p.m. UTC | #8
Ludovic Courtès schreef op zo 27-02-2022 om 22:23 [+0100]:
> I think this is equivalent to:
> 
>   #~(let* ((inputs #$(input-tuples->gexp (package-inputs this-package)))
>            …)
>       …)
> 
> Am I right?

That's what I suggested initially:

> > #~(let* ((inputs #$(input-tuples->gexp (package-inputs this-
> package)))
> >          (usb.ids (search-input-file inputs
> "/share/hwdata/usb.ids"))
> >          (pci.ids (search-input-file inputs
> "/share/hwdata/pci.ids")))
> >     (list (string-append "-Dwith-usb-ids-path=" usb.ids) [...]))
> > 
> 

However, that lead to errors -- strings like #<package foo [...]>
appeared in the builder.

Greetings,
Maxime.
Ludovic Courtès Feb. 28, 2022, 11:48 a.m. UTC | #9
Hi!

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op zo 27-02-2022 om 22:23 [+0100]:
>> Can’t we instead do:
>> 
>>   #~(let* ((usb.ids (string-append #$hwdata:usb "/share/hwdata/usb.ids"))
>>            (pci.ids (string-append #$hwdata:pci "/share/hwdata/pci.ids")))
>>       …)
>> 
>> ?
>
> We could, and that's what the original v2 patch did.
> However, this prevents package transformations:

Right, but maybe that’s good enough?…

Otherwise, we can do:

  #~(let* ((usb.ids (string-append
                     (ungexp (this-package-inputs "hwdata") "usb")
                     …)))
      …)


[...]

> Ludovic Courtès schreef op zo 27-02-2022 om 22:23 [+0100]:
>> I think this is equivalent to:
>> 
>>   #~(let* ((inputs #$(input-tuples->gexp (package-inputs this-package)))
>>            …)
>>       …)
>> 
>> Am I right?
>
> That's what I suggested initially:
>
>> > #~(let* ((inputs #$(input-tuples->gexp (package-inputs this-
>> package)))
>> >          (usb.ids (search-input-file inputs
>> "/share/hwdata/usb.ids"))
>> >          (pci.ids (search-input-file inputs
>> "/share/hwdata/pci.ids")))
>> >     (list (string-append "-Dwith-usb-ids-path=" usb.ids) [...]))
>> > 
>> 
>
> However, that lead to errors -- strings like #<package foo [...]>
> appeared in the builder.

Hmm weird.

Thanks,
Ludo’.
diff mbox series

Patch

diff --git a/gnu/packages/virtualization.scm b/gnu/packages/virtualization.scm
index 506ebe4bc2..c890b43a3e 100644
--- a/gnu/packages/virtualization.scm
+++ b/gnu/packages/virtualization.scm
@@ -25,6 +25,7 @@ 
 ;;; Copyright © 2021 Petr Hodina <phodina@protonmail.com>
 ;;; Copyright © 2021 Raghav Gururajan <rg@raghavgururajan.name>
 ;;; Copyright © 2022 Oleg Pykhalov <go.wigust@gmail.com>
+;;; Copyright © 2022 Brendan Tildesley <mail@brendan.scot>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -98,6 +99,7 @@  (define-module (gnu packages virtualization)
   #:use-module (gnu packages onc-rpc)
   #:use-module (gnu packages package-management)
   #:use-module (gnu packages perl)
+  #:use-module (gnu packages pciutils)
   #:use-module (gnu packages pcre)
   #:use-module (gnu packages pkg-config)
   #:use-module (gnu packages polkit)
@@ -1027,46 +1029,32 @@  (define-public libosinfo
          "0nd360c9ampw8hb6xh5g45q858df2r4jj9q88bcl6gzgaj0l3wxl"))))
     (build-system meson-build-system)
     (arguments
-     `(#:configure-flags
-       (list (string-append "-Dwith-usb-ids-path="
-                            (assoc-ref %build-inputs "usb.ids"))
-             (string-append "-Dwith-pci-ids-path="
-                            (assoc-ref %build-inputs "pci.ids")))
-       #:phases
-       (modify-phases %standard-phases
-         (add-after 'unpack 'patch-osinfo-path
-           (lambda* (#:key inputs #:allow-other-keys)
-             (substitute* "osinfo/osinfo_loader.c"
-               (("path = DATA_DIR.*")
-                (string-append "path = \"" (assoc-ref inputs "osinfo-db")
-                               "/share/osinfo\";"))))))))
+     (list
+      #:configure-flags
+      #~(list
+         (string-append "-Dwith-usb-ids-path=" #$hwdata:usb "/share/hwdata/usb.ids")
+         (string-append "-Dwith-pci-ids-path=" #$hwdata:pci "/share/hwdata/pci.ids"))
+      #:phases
+      #~(modify-phases %standard-phases
+          (add-after 'unpack 'patch-osinfo-path
+            (lambda* (#:key inputs #:allow-other-keys)
+              (substitute* "osinfo/osinfo_loader.c"
+                (("path = DATA_DIR.*")
+                 (string-append "path = \"" #$osinfo-db "/share/osinfo\";"))))))))
     (inputs
-     `(("libsoup" ,libsoup-minimal-2)
-       ("libxml2" ,libxml2)
-       ("libxslt" ,libxslt)
-       ("osinfo-db" ,osinfo-db)))
+     (list libsoup-minimal-2
+           libxml2
+           libxslt
+           osinfo-db))
     (native-inputs
-     `(("glib" ,glib "bin")  ; glib-mkenums, etc.
-       ("gobject-introspection" ,gobject-introspection)
-       ("gtk-doc" ,gtk-doc/stable)
-       ("vala" ,vala)
-       ("intltool" ,intltool)
-       ("pkg-config" ,pkg-config)
-       ("pci.ids"
-        ,(origin
-           (method url-fetch)
-           (uri "https://github.com/pciutils/pciids/raw/ad02084f0bc143e3c15e31a6152a3dfb1d7a3156/pci.ids")
-           (sha256
-            (base32
-             "0kfhpj5rnh24hz2714qhfmxk281vwc2w50sm73ggw5d15af7zfsw"))))
-       ("usb.ids"
-        ,(origin
-           (method url-fetch)
-           (uri "https://svn.code.sf.net/p/linux-usb/repo/trunk/htdocs/usb.ids?r=2681")
-           (file-name "usb.ids")
-           (sha256
-            (base32
-             "1m6yhvz5k8aqzxgk7xj3jkk8frl1hbv0h3vgj4wbnvnx79qnvz3r"))))))
+     (list `(,glib "bin")  ; glib-mkenums, etc.
+           gobject-introspection
+           gtk-doc/stable
+           vala
+           intltool
+           pkg-config
+           `(,hwdata "pci")
+           `(,hwdata "usb")))
     (home-page "https://libosinfo.org/")
     (synopsis "Operating system information database")
     (description "libosinfo is a GObject based library API for managing