[dpdk-dev] port: fix build when KNI support is not enabled

  Commit 9fc37d1c071c is missing a conditional in the dependencies,
causing builds to fail when KNI is not enabled:
    == Build lib/librte_port
      LD librte_port.so.3
    /usr/bin/ld: cannot find -lrte_kni
    collect2: error: ld returned 1 exit status

Fixes: 9fc37d1c071c ("port: support KNI")

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_port/Makefile | 2 ++
 1 file changed, 2 insertions(+)
  

2016-06-22 14:34, Panu Matilainen:
> --- a/lib/librte_port/Makefile
> +++ b/lib/librte_port/Makefile
> @@ -82,6 +82,8 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_mempool
>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ether
>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ip_frag
>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_sched
> +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_kni
> +endif

I do not remember why $(CONFIG_RTE_LIBRTE_PORT) is needed in its Makefile.
I think we can do
	DEPDIRS-$(CONFIG_RTE_LIBRTE_KNI) += lib/librte_kni
and set DEPDIRS-y everywhere else.
  

Hi Thomas,

On 06/22/2016 01:49 PM, Thomas Monjalon wrote:
> 2016-06-22 14:34, Panu Matilainen:
>> --- a/lib/librte_port/Makefile
>> +++ b/lib/librte_port/Makefile
>> @@ -82,6 +82,8 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_mempool
>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ether
>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ip_frag
>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_sched
>> +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_kni
>> +endif
> 
> I do not remember why $(CONFIG_RTE_LIBRTE_PORT) is needed in its Makefile.
> I think we can do
> 	DEPDIRS-$(CONFIG_RTE_LIBRTE_KNI) += lib/librte_kni
> and set DEPDIRS-y everywhere else.
> 

It's probably not much used, but the build framework allows to do
the following to build only one directory:

  make lib/librte_port_sub

This directly jumps to the librte_port Makefile, bypassing parent
directories. I think that's why the config check is duplicated in the
Makefile.


Olivier
  

2016-06-22 13:57, Olivier Matz:
> Hi Thomas,
> 
> On 06/22/2016 01:49 PM, Thomas Monjalon wrote:
> > 2016-06-22 14:34, Panu Matilainen:
> >> --- a/lib/librte_port/Makefile
> >> +++ b/lib/librte_port/Makefile
> >> @@ -82,6 +82,8 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_mempool
> >>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ether
> >>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ip_frag
> >>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_sched
> >> +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
> >>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_kni
> >> +endif
> > 
> > I do not remember why $(CONFIG_RTE_LIBRTE_PORT) is needed in its Makefile.
> > I think we can do
> > 	DEPDIRS-$(CONFIG_RTE_LIBRTE_KNI) += lib/librte_kni
> > and set DEPDIRS-y everywhere else.
> > 
> 
> It's probably not much used, but the build framework allows to do
> the following to build only one directory:
> 
>   make lib/librte_port_sub
> 
> This directly jumps to the librte_port Makefile, bypassing parent
> directories. I think that's why the config check is duplicated in the
> Makefile.

If we want to specifically build this directory, why preventing us to do
so with CONFIG_RTE_LIBRTE_PORT?
  

On 06/22/2016 02:20 PM, Thomas Monjalon wrote:
> 2016-06-22 13:57, Olivier Matz:
>> Hi Thomas,
>>
>> On 06/22/2016 01:49 PM, Thomas Monjalon wrote:
>>> 2016-06-22 14:34, Panu Matilainen:
>>>> --- a/lib/librte_port/Makefile
>>>> +++ b/lib/librte_port/Makefile
>>>> @@ -82,6 +82,8 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_mempool
>>>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ether
>>>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ip_frag
>>>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_sched
>>>> +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
>>>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_kni
>>>> +endif
>>>
>>> I do not remember why $(CONFIG_RTE_LIBRTE_PORT) is needed in its Makefile.
>>> I think we can do
>>> 	DEPDIRS-$(CONFIG_RTE_LIBRTE_KNI) += lib/librte_kni
>>> and set DEPDIRS-y everywhere else.
>>>
>>
>> It's probably not much used, but the build framework allows to do
>> the following to build only one directory:
>>
>>   make lib/librte_port_sub
>>
>> This directly jumps to the librte_port Makefile, bypassing parent
>> directories. I think that's why the config check is duplicated in the
>> Makefile.
> 
> If we want to specifically build this directory, why preventing us to do
> so with CONFIG_RTE_LIBRTE_PORT?

If we call foo_sub with CONFIG_FOO=n, it will generate a library and
install headers in the build directory, however the config is unset.
Some propositions if we want to replace
DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) by DEPDIRS-y:

1/ say that "make foo_sub" should be used with care, only if CONFIG_FOO
   is set (else it is not supported) -> nothing to do
2/ fix the make %_sub feature to browse parent directories, checking
   the SUBDIRS-${CONFIG_FOO}
3/ remove the make %_sub feature, maybe nobody cares...

I think 1/ is acceptable.
  

On Wed, Jun 22, 2016 at 05:32:37PM +0200, Olivier Matz wrote:
> On 06/22/2016 02:20 PM, Thomas Monjalon wrote:
> > 2016-06-22 13:57, Olivier Matz:
> >> Hi Thomas,
> >>
> >> On 06/22/2016 01:49 PM, Thomas Monjalon wrote:
> >>> 2016-06-22 14:34, Panu Matilainen:
> >>>> --- a/lib/librte_port/Makefile
> >>>> +++ b/lib/librte_port/Makefile
> >>>> @@ -82,6 +82,8 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_mempool
> >>>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ether
> >>>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ip_frag
> >>>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_sched
> >>>> +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
> >>>>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_kni
> >>>> +endif
> >>>
> >>> I do not remember why $(CONFIG_RTE_LIBRTE_PORT) is needed in its Makefile.
> >>> I think we can do
> >>> 	DEPDIRS-$(CONFIG_RTE_LIBRTE_KNI) += lib/librte_kni
> >>> and set DEPDIRS-y everywhere else.
> >>>
> >>
> >> It's probably not much used, but the build framework allows to do
> >> the following to build only one directory:
> >>
> >>   make lib/librte_port_sub
> >>
> >> This directly jumps to the librte_port Makefile, bypassing parent
> >> directories. I think that's why the config check is duplicated in the
> >> Makefile.
> > 
> > If we want to specifically build this directory, why preventing us to do
> > so with CONFIG_RTE_LIBRTE_PORT?
> 
> If we call foo_sub with CONFIG_FOO=n, it will generate a library and
> install headers in the build directory, however the config is unset.
> Some propositions if we want to replace
> DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) by DEPDIRS-y:
> 
> 1/ say that "make foo_sub" should be used with care, only if CONFIG_FOO
>    is set (else it is not supported) -> nothing to do
> 2/ fix the make %_sub feature to browse parent directories, checking
>    the SUBDIRS-${CONFIG_FOO}
> 3/ remove the make %_sub feature, maybe nobody cares...
> 
> I think 1/ is acceptable.

+1

Especially given the fact I wasn't even aware that you could do that (building
just one subdir). Do we have a usecase where people might want to build just one
DPDK subdirectory?

/Bruce
  

Hi Bruce,

On 06/23/2016 10:53 AM, Bruce Richardson wrote:
>>> If we want to specifically build this directory, why preventing us to do
>>> so with CONFIG_RTE_LIBRTE_PORT?
>>
>> If we call foo_sub with CONFIG_FOO=n, it will generate a library and
>> install headers in the build directory, however the config is unset.
>> Some propositions if we want to replace
>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) by DEPDIRS-y:
>>
>> 1/ say that "make foo_sub" should be used with care, only if CONFIG_FOO
>>    is set (else it is not supported) -> nothing to do
>> 2/ fix the make %_sub feature to browse parent directories, checking
>>    the SUBDIRS-${CONFIG_FOO}
>> 3/ remove the make %_sub feature, maybe nobody cares...
>>
>> I think 1/ is acceptable.
> 
> +1
> 
> Especially given the fact I wasn't even aware that you could do that (building
> just one subdir). Do we have a usecase where people might want to build just one
> DPDK subdirectory?

The only use-case I see is to gain few seconds when recompiling: it can
avoid to check deps in all directories if you know that your
modifications only take place in a specific directory.

Well, nothing really essential ;)
  

> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Wednesday, June 22, 2016 12:34 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> zhuangwj@gmail.com
> Subject: [PATCH] port: fix build when KNI support is not enabled
> 
> Commit 9fc37d1c071c is missing a conditional in the dependencies,
> causing builds to fail when KNI is not enabled:
>     == Build lib/librte_port
>       LD librte_port.so.3
>     /usr/bin/ld: cannot find -lrte_kni
>     collect2: error: ld returned 1 exit status
> 
> Fixes: 9fc37d1c071c ("port: support KNI")
> 
> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> ---

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
  

> > Commit 9fc37d1c071c is missing a conditional in the dependencies,
> > causing builds to fail when KNI is not enabled:
> >     == Build lib/librte_port
> >       LD librte_port.so.3
> >     /usr/bin/ld: cannot find -lrte_kni
> >     collect2: error: ld returned 1 exit status
> > 
> > Fixes: 9fc37d1c071c ("port: support KNI")
> > 
> > Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> 
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Applied, thanks

A cleanup of the makefiles is welcome, as discussed in this thread,
to remove useless reference to their own config variable.