> -----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
