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 -
