From mboxrd@z Thu Jan 1 00:00:00 1970 From: Panu Matilainen Subject: Re: [PATCH 1/3] mk: fix librte_pipeline dependency list truncation Date: Tue, 21 Jun 2016 13:44:45 +0300 Message-ID: References: <3EB4FA525960D640B5BDFFD6A3D8912647A0A645@IRSMSX108.ger.corp.intel.com> <573cdca2-ee36-8de3-2bcd-e40ba7c9dd2f@redhat.com> <20160621103115.GB21016@bricha3-MOBL3> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Cc: "Dumitrescu, Cristian" , "dev@dpdk.org" , "christian.ehrhardt@canonical.com" , "thomas.monjalon@6wind.com" To: Bruce Richardson Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 1ECC65A56 for ; Tue, 21 Jun 2016 12:44:48 +0200 (CEST) In-Reply-To: <20160621103115.GB21016@bricha3-MOBL3> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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@dpdk.org] On Behalf Of Panu Matilainen >>>> Sent: Tuesday, June 21, 2016 9:12 AM >>>> To: dev@dpdk.org >>>> Cc: christian.ehrhardt@canonical.com; thomas.monjalon@6wind.com >>>> Subject: [dpdk-dev] [PATCH 1/3] mk: fix librte_pipeline dependency l= ist >>>> 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 >>>> --- >>>> lib/librte_pipeline/Makefile | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/lib/librte_pipeline/Makefile b/lib/librte_pipeline/Make= file >>>> index 95387aa..a8f3128 100644 >>>> --- a/lib/librte_pipeline/Makefile >>>> +++ b/lib/librte_pipeline/Makefile >>>> @@ -53,7 +53,7 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_PIPELINE)-include +=3D >>>> rte_pipeline.h >>>> >>>> # this lib depends upon: >>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D lib/librte_eal >>>> -DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) :=3D lib/librte_table >>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D lib/librte_table >>>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D 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 librt= e_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, wh= ich is listed with the assignment operator, followed by others that are l= isted with the append operator: >>> DEPDIRS-$(CONFIG_RTE_LIBRTE_XYZ) :=3D lib/librte_eal >>> DEPDIRS-$(CONFIG_RTE_LIBRTE_XYZ) +=3D lib/librte_some other lib >>> >>> Therefore, at least for cosmetic reasons, we should probably do the s= ame in librte_pipeline, which requires changing both the librte_eal and t= he librte_table lines as below: >>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) :=3D lib/librte_eal >>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D lib/librte_table >>> DEPDIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D lib/librte_port >> >> Ah, didn't notice those because the assignment is first of the depende= ncies. >> >>> >>> However, some other libraries e.g. librte_lpm simply add the EAL depe= ndency using the append operator: >>> DEPDIRS-$(CONFIG_RTE_LIBRTE_LPM) +=3D lib/librte_eal >>> >>> To be honest, I need to refresh my knowledge on make, I don't remembe= r 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 o= r 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, =E2=80=98+= =3D=E2=80=99 acts just >> like normal =E2=80=98=3D=E2=80=99: it defines a recursively-expanded v= ariable. However, when >> there is a previous definition, exactly what =E2=80=98+=3D=E2=80=99 do= es depends on what >> flavor of variable you defined originally." >> >> So there's no need to use :=3D anywhere for the dependencies, in fact = its >> probably best avoided to avoid issues like this. Of course after the t= hird >> patch in this "series" is applied, mistakes like these can no longer g= o >> unnoticed. >> > Will the build be any slower with everything defaulting to recursively = expanded > variables rather than the simply-expanded variables defined by the init= ial ":=3D"? Bruce, everything already *is* defaulting to recursively expanded=20 variables, except for the three libraries here which have used :=3D for=20 who knows what (historical or other) reason. And out of those three=20 exceptions, one is buggy. Which is what I'm addressing here. - Panu -