On 2016-02-16 07:24, Ricardo Wurmus wrote:
> rennes@openmailbox.org writes:
> 
>> i attached libosinfo patch required for GNOME Boxes.
> 
> Thank you very much!  Below I’ll add a couple of comments.  I’m not 
> sure
> about a couple of things and I hope you can shed some light on these
> issues.
> 
>> Considerations:
>> 
>>   a) In the source i used
>> "https://fedorahosted.org/releases/l/i/libosinfo" instead
>> "mirror://gnome/sources/".
> 
> The project home page points to fedorahosted.org for downloading
> releases.  I don’t even find the libosinfo sources on ftp.gnome.org, so
> I think your choice is completely fine.
> 
>>   b) In native inputs, i used "vala" instead "glib:bin"; i follow the
>> README.
> 
> I don’t understand what you mean.  Vala and glib:bin are not the same
> thing.  Following the README is a good idea.  If we don’t need
> “glib:bin” then it’s the right thing not to add it.
> 
>> From 073a183499bd764b0b0efc246748638c6e4d3aeb Mon Sep 17 00:00:00 2001
>> From: Rene Saavedra <rennes@openmailbox.org>
>> Date: Sat, 13 Feb 2016 16:23:10 -0600
>> Subject: [PATCH] gnu: Add libosinfo.
> 
>> * gnu/packages/gnome.scm (libosinfo): New variable.
> 
> OK.
> 
>> +
>> +(define-public libosinfo
>> +  (package
>> +    (name "libosinfo")
>> +    (version "0.3.0")
>> +    (source
>> +     (origin
>> +       (method url-fetch)
>> +       (uri (string-append 
>> "https://fedorahosted.org/releases/l/i/libosinfo/"
>> +                            name "-" version ".tar.gz"))
>> +       (sha256
>> +       (base32
>> +        "1g7g5hc4lhi4y0j3mbcj19hawlqkflni1zk4aggrx49fg5l392jk"))))
> 
> The indentation of the “(base32” expression is wrong; the opening
> parenthesis should be aligned with the ‘s’ of “(sha256”.  (Don’t worry
> about this too much — I can fix this before pushing if you don’t get it
> right.)
> 
>> +    (build-system glib-or-gtk-build-system)
> 
> Is this necessary or can we use the simpler “gnu-build-system” instead?
> 
>> +    (native-inputs
>> +     `(("check" ,check)
>> +       ("intltool" ,intltool)
>> +       ("libsoup" ,libsoup)
>> +       ("pkg-config" ,pkg-config)
>> +       ("vala" ,vala)
>> +       ("wget" ,wget)))
> 
> Why are “wget” and “libsoup” native inputs?  In the build environment
> there is no network access, so wget cannot be used to download 
> anything.
> Is libosinfo linked with libsoup?  If so, it should be a regular input,
> not among the native-inputs.
> 
>> +    (inputs
>> +     `(("libxslt" ,libxslt)))
> 
> Does it link with libxslt?  Or is it used at build time only (e.g. for
> building manuals from XML sources)?
> 
>> +    (home-page "https://libosinfo.org")
>> +    (synopsis "Library for managing information about operating 
>> systems")
>> +    (description
>> +     "libosinfo is a GObject based library API for managing 
>> information about
>> +operating systems, hypervisors and the (virtual) hardware devices 
>> they can
>> +support.  It includes a database containing device metadata and 
>> provides APIs
>> +to match/identify optimal devices for deploying an operating system 
>> on a
>> +hypervisor.  Via the magic of GObject Introspection, the API is 
>> available in all
>> +common programming languages with demos for javascript (GJS/Seed) and 
>> python
>> +(PyGObject).  Also provided are Vala bindings.")
> 
> I would cut “the magic of”.  Please also replace “javascript” with
> “JavaScript” and “python” with “Python”.  Are the examples and the Vala
> bindings actually installed?  I see that gobject-introspection is not
> among the inputs, so I wonder if
> 
>> +    (license license:lgpl2.1+)))
> 
> At least “tools/osinfo-query.c” has a license header that says it’s
> released under GPLv2+.  It is better to make the license field hold a
> list of “(license:gpl2+ license:lgpl2.1+)” with a comment above it that
> explains what files are under GPL (the tools) and what files are LGPL
> (the library).
> 
> Could you please send an updated patch (after clarifying the points
> above if they are confusing/contentious)?
> 
> Thanks again for the patch!
> 
> ~~ Ricardo


Hi Ricardo,

I made changes to the patch. I generated the patch for the file 
"gnu/packages/gnome.scm".

Explanations/ questions:

  a) I replace "build-system glib-or-gtk-build-system", in this case is a 
library(not GNOME applications); then i think is convenient use 
"build-system gnu-build-system".
  b) I don't know exactly the function in the package "libxslt", but is 
required(README). I can see the package "libxslt" in the output of the 
command "./pre-inst-env guix size libosinfo". I will look for more 
documentation.
  c) The examples are not installed, sorry for my mistake in the 
description. I removed that part.


Thanks