libpciaccess: add dependency on hwdata

Message ID 20180128235233.26989-1-casantos@datacom.ind.br
State Rejected
Headers show
Series
  • libpciaccess: add dependency on hwdata
Related show

Commit Message

Carlos Santos Jan. 28, 2018, 11:52 p.m.
libpciaccess requires /usr/share/hwdata/pci.ids (or pci.ids.gz, if
compiled with zlib support). That file is provided by the hwdata
package, so add it as a run-time dependency.

Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
---
 package/libpciaccess/Config.in | 2 ++
 1 file changed, 2 insertions(+)

Comments

Peter Korsgaard March 29, 2018, 2:05 p.m. | #1
>>>>> "Carlos" == Carlos Santos <casantos@datacom.ind.br> writes:

 > libpciaccess requires /usr/share/hwdata/pci.ids (or pci.ids.gz, if
 > compiled with zlib support). That file is provided by the hwdata
 > package, so add it as a run-time dependency.

 > Signed-off-by: Carlos Santos <casantos@datacom.ind.br>

Is this really a hard dependency? Looking at src/common_device_name.c,
this seems to only be used in populate_vendor(), which just returns
without error if the file cannot be opened.
Carlos Santos March 29, 2018, 4:41 p.m. | #2
> From: "Peter Korsgaard" <peter@korsgaard.com>
> To: "Carlos Santos" <casantos@datacom.ind.br>
> Cc: "buildroot" <buildroot@buildroot.org>, "Bernd Kuhls" <bernd.kuhls@t-online.de>
> Sent: Thursday, March 29, 2018 11:05:17 AM
> Subject: Re: [PATCH] libpciaccess: add dependency on hwdata

>>>>>> "Carlos" == Carlos Santos <casantos@datacom.ind.br> writes:
> 
> > libpciaccess requires /usr/share/hwdata/pci.ids (or pci.ids.gz, if
> > compiled with zlib support). That file is provided by the hwdata
> > package, so add it as a run-time dependency.
> 
> > Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
> 
> Is this really a hard dependency? Looking at src/common_device_name.c,
> this seems to only be used in populate_vendor(), which just returns
> without error if the file cannot be opened.
> 
> --
> Bye, Peter Korsgaard

It is required by libvirt[1] to show device names when we use virtual
machine manager to add hardware via PCI passthrough, otherwise only
the PCI bus information (slot, port, function) is shown.

1. https://patchwork.ozlabs.org/patch/841613/
Yann E. MORIN March 29, 2018, 5:52 p.m. | #3
Carlos, All,

On 2018-03-29 13:41 -0300, Carlos Santos spake thusly:
> > From: "Peter Korsgaard" <peter@korsgaard.com>
> > To: "Carlos Santos" <casantos@datacom.ind.br>
> > Cc: "buildroot" <buildroot@buildroot.org>, "Bernd Kuhls" <bernd.kuhls@t-online.de>
> > Sent: Thursday, March 29, 2018 11:05:17 AM
> > Subject: Re: [PATCH] libpciaccess: add dependency on hwdata
> 
> >>>>>> "Carlos" == Carlos Santos <casantos@datacom.ind.br> writes:
> > 
> > > libpciaccess requires /usr/share/hwdata/pci.ids (or pci.ids.gz, if
> > > compiled with zlib support). That file is provided by the hwdata
> > > package, so add it as a run-time dependency.
> > 
> > > Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
> > 
> > Is this really a hard dependency? Looking at src/common_device_name.c,
> > this seems to only be used in populate_vendor(), which just returns
> > without error if the file cannot be opened.
> > 
> > --
> > Bye, Peter Korsgaard
> 
> It is required by libvirt[1] to show device names when we use virtual
> machine manager to add hardware via PCI passthrough, otherwise only
> the PCI bus information (slot, port, function) is shown.

Then I would say that it is not _required_. It may be _needed_, but it
is not mandatory; it just makes it a little bit more user-friendly.

Thus, I would say that we do not want to enforce this dependency,
because it still works without it.

Regards,
Yann E. MORIN.
Peter Korsgaard March 29, 2018, 7:05 p.m. | #4
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 >> It is required by libvirt[1] to show device names when we use virtual
 >> machine manager to add hardware via PCI passthrough, otherwise only
 >> the PCI bus information (slot, port, function) is shown.

 > Then I would say that it is not _required_. It may be _needed_, but it
 > is not mandatory; it just makes it a little bit more user-friendly.

 > Thus, I would say that we do not want to enforce this dependency,
 > because it still works without it.

And if it IS required when using libvirt with pci passthrough, but not
necessarily for other users of libpciaccess, then we can make libvirt
select it.

In the general case for nice-to-have-but-not-absolutely-required
dependencies we don't have a solution in Buildroot today except for a
note in the help text. Upstream kconfig has recently added the imply
keyword which could be used to implement this, but we would need to sync
our kconfig with upstream for that.
Carlos Santos April 3, 2018, 1:38 a.m. | #5
> From: "Peter Korsgaard" <peter@korsgaard.com>
> To: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: "Carlos Santos" <casantos@datacom.ind.br>, "Bernd Kuhls" <bernd.kuhls@t-online.de>, "buildroot"
> <buildroot@buildroot.org>
> Sent: Thursday, March 29, 2018 4:05:28 PM
> Subject: Re: [PATCH] libpciaccess: add dependency on hwdata

>>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> 
> Hi,
> 
> >> It is required by libvirt[1] to show device names when we use virtual
> >> machine manager to add hardware via PCI passthrough, otherwise only
> >> the PCI bus information (slot, port, function) is shown.
> 
> > Then I would say that it is not _required_. It may be _needed_, but it
> > is not mandatory; it just makes it a little bit more user-friendly.
> 
> > Thus, I would say that we do not want to enforce this dependency,
> > because it still works without it.
> 
> And if it IS required when using libvirt with pci passthrough, but not
> necessarily for other users of libpciaccess, then we can make libvirt
> select it.
> 
> In the general case for nice-to-have-but-not-absolutely-required
> dependencies we don't have a solution in Buildroot today except for a
> note in the help text. Upstream kconfig has recently added the imply
> keyword which could be used to implement this, but we would need to sync
> our kconfig with upstream for that.

OK, I will move the selection o hwdata to libvirt in the next version
of the path series.
Thomas Petazzoni April 28, 2018, 9:39 p.m. | #6
Hello,

On Mon, 2 Apr 2018 22:38:39 -0300 (BRT), Carlos Santos wrote:

> > In the general case for nice-to-have-but-not-absolutely-required
> > dependencies we don't have a solution in Buildroot today except for a
> > note in the help text. Upstream kconfig has recently added the imply
> > keyword which could be used to implement this, but we would need to sync
> > our kconfig with upstream for that.  
> 
> OK, I will move the selection o hwdata to libvirt in the next version
> of the path series.

I don't know where we stand with the libvirt packaging, but following
this feedback, I marked this patch as Rejected in patchwork.

Best regards,

Thomas

Patch

diff --git a/package/libpciaccess/Config.in b/package/libpciaccess/Config.in
index 6cc983f577..381383e1f9 100644
--- a/package/libpciaccess/Config.in
+++ b/package/libpciaccess/Config.in
@@ -1,4 +1,6 @@ 
 config BR2_PACKAGE_LIBPCIACCESS
 	bool "libpciaccess"
+	select BR2_PACKAGE_HWDATA
+	select BR2_PACKAGE_HWDATA_PCI_IDS
 	help
 	  X.Org libpciaccess