On 06/21/2016 01:58 PM, Dumitrescu, Cristian wrote:
>
>
>> -----Original Message-----
>> From: Panu Matilainen [mailto:pmatilai at redhat.com]
>> Sent: Tuesday, June 21, 2016 11:45 AM
>> To: Richardson, Bruce <bruce.richardson at intel.com>
>> Cc: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>; dev at dpdk.org;
>> christian.ehrhardt at canonical.com; thomas.monjalon at 6wind.com
>> Subject: Re: [dpdk-dev] [PATCH 1/3] mk: fix librte_pipeline dependency list
>> truncation
>>
>> On 06/21/2016 01:31 PM, Bruce Richardson wrote:
>>> On Tue, Jun 21, 2016 at 01:25:52PM +0300, Panu Matilainen wrote:
>>>> On 06/21/2016 01:01 PM, Dumitrescu, Cristian wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Panu
>> Matilainen
>>>>>> Sent: Tuesday, June 21, 2016 9:12 AM
>>>>>> To: dev at dpdk.org
>>>>>> Cc: christian.ehrhardt at canonical.com; thomas.monjalon at 6wind.com
>>>>>> Subject: [dpdk-dev] [PATCH 1/3] mk: fix librte_pipeline dependency list
>>>>>> truncation
>>>>>>
>>>>>> In other libraries, dependency list is always appended to, but
>>>>>> in commit 6cbf4f75e059 it with an assignment. This causes the
>>>>>> librte_eal dependency added in commit 6cbf4f75e059 to get discarded,
>>>>>> resulting in missing dependency on librte_eal.
>>>>>>
>>>>>> Fixes: b3688bee81a8 ("pipeline: new packet framework logic")
>>>>>> Fixes: 6cbf4f75e059 ("mk: fix missing internal dependencies")
>>>>>>
>>>>>> Signed-off-by: Panu Matilainen <pmatilai at redhat.com>
>>>>>> ---
>>>>>> lib/librte_pipeline/Makefile | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/librte_pipeline/Makefile b/lib/librte_pipeline/Makefile
>>>>>> index 95387aa..a8f3128 100644
>>>>>> --- a/lib/librte_pipeline/Makefile
>>>>>> +++ b/lib/librte_pipeline/Makefile
>>>>>> @@ -53,7 +53,7 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_PIPELINE)-
>> include +=
>>>>>> rte_pipeline.h
>>>>>>
>>>>>> # this lib depends upon:
>>>>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += lib/librte_eal
>>>>>> -DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) := lib/librte_table
>>>>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += lib/librte_table
>>>>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += lib/librte_port
>>>>>>
>>>>>> include $(RTE_SDK)/mk/rte.lib.mk
>>>>>> --
>>>>>> 2.5.5
>>>>>
>>>>>
>>>>> In release 16.4, EAL was missing from the dependency list, now it is
>> added. The librte_pipeline uses rte_malloc, therefore it depends on
>> librte_eal being present.
>>>>>
>>>>> In the Makefile of the other Packet Framework libraries (librte_port,
>> librte_table), it looks like the first dependency in the list is EAL, which
>> is listed
>> with the assignment operator, followed by others that are listed with the
>> append operator:
>>>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_XYZ) := lib/librte_eal
>>>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_XYZ) += lib/librte_some other lib
>>>>>
>>>>> Therefore, at least for cosmetic reasons, we should probably do the
>> same in librte_pipeline, which requires changing both the librte_eal and the
>> librte_table lines as below:
>>>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) := lib/librte_eal
>>>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += lib/librte_table
>>>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += lib/librte_port
>>>>
>>>> Ah, didn't notice those because the assignment is first of the
>> dependencies.
>>>>
>>>>>
>>>>> However, some other libraries e.g. librte_lpm simply add the EAL
>> dependency using the append operator:
>>>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_LPM) += lib/librte_eal
>>>>>
>>>>> To be honest, I need to refresh my knowledge on make, I don't
>> remember right now when we should use the assignment and when the
>> append. Do we need to use the assign for first dependency (EAL) and
>> append for others or should we use append everywhere?
>>>>
>>>> At least in automake, you need to assign before you can append. But in
>> gmake
>>>> this apparently is not the case, quoting from
>>>>
>> https://www.gnu.org/software/make/manual/html_node/Appending.html:
>>>>
>>>> "When the variable in question has not been defined before, ?+=? acts
>> just
>>>> like normal ?=?: it defines a recursively-expanded variable. However,
>> when
>>>> there is a previous definition, exactly what ?+=? does depends on what
>>>> flavor of variable you defined originally."
>>>>
>>>> So there's no need to use := anywhere for the dependencies, in fact its
>>>> probably best avoided to avoid issues like this. Of course after the third
>>>> patch in this "series" is applied, mistakes like these can no longer go
>>>> unnoticed.
>>>>
>>> Will the build be any slower with everything defaulting to recursively
>> expanded
>>> variables rather than the simply-expanded variables defined by the initial
>> ":="?
>>
>> Bruce, everything already *is* defaulting to recursively expanded
>> variables, except for the three libraries here which have used := for
>> who knows what (historical or other) reason. And out of those three
>> exceptions, one is buggy. Which is what I'm addressing here.
>>
>> - Panu -
>
> Yes, you're right, looks like the assign operator is only used in these 3
> places. Therefore, it would probably make sense to replace it with the append
> operator for all of them?
>
> dpdk/lib> grep DEPDIRS `find . -name Makefile` | grep '+=' | wc -l
> 60
> dpdk/lib> grep DEPDIRS `find . -name Makefile` | grep ':=' | wc -l
> 3
> dpdk/lib> grep DEPDIRS `find . -name Makefile` | grep ':='
> ./librte_pipeline/Makefile:DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) :=
> lib/librte_table
> ./librte_port/Makefile:DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) := lib/librte_eal
> ./librte_table/Makefile:DEPDIRS-$(CONFIG_RTE_LIBRTE_TABLE) := lib/librte_eal
>
Yeah, it wouldn't hurt to be consistent. OTOH only the one in
librte_pipeline is an actual problem.
- Panu -
