From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 1D5539A92 for ; Tue, 21 Jun 2016 13:53:31 +0200 (CEST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5B9B8C0467E7; Tue, 21 Jun 2016 11:53:30 +0000 (UTC) Received: from sopuli.koti.laiskiainen.org (vpn1-6-229.ams2.redhat.com [10.36.6.229]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5LBrStG002476; Tue, 21 Jun 2016 07:53:29 -0400 To: "Dumitrescu, Cristian" , "Richardson, Bruce" References: <3EB4FA525960D640B5BDFFD6A3D8912647A0A645@IRSMSX108.ger.corp.intel.com> <573cdca2-ee36-8de3-2bcd-e40ba7c9dd2f@redhat.com> <20160621103115.GB21016@bricha3-MOBL3> <3EB4FA525960D640B5BDFFD6A3D8912647A0A75A@IRSMSX108.ger.corp.intel.com> Cc: "dev@dpdk.org" , "christian.ehrhardt@canonical.com" , "thomas.monjalon@6wind.com" From: Panu Matilainen Message-ID: <7f09b15d-9880-937f-1bd0-8c8eb10144db@redhat.com> Date: Tue, 21 Jun 2016 14:53:28 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D8912647A0A75A@IRSMSX108.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 21 Jun 2016 11:53:30 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 1/3] mk: fix librte_pipeline dependency list truncation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Jun 2016 11:53:31 -0000 On 06/21/2016 01:58 PM, Dumitrescu, Cristian wrote: > > >> -----Original Message----- >> From: Panu Matilainen [mailto:pmatilai@redhat.com] >> Sent: Tuesday, June 21, 2016 11:45 AM >> To: Richardson, Bruce >> Cc: Dumitrescu, Cristian ; dev@dpdk.org; >> christian.ehrhardt@canonical.com; thomas.monjalon@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@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 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 >>>>>> --- >>>>>> 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 -