From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: Panu Matilainen <pmatilai@redhat.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"christian.ehrhardt@canonical.com"
<christian.ehrhardt@canonical.com>,
"thomas.monjalon@6wind.com" <thomas.monjalon@6wind.com>
Subject: Re: [dpdk-dev] [PATCH 1/3] mk: fix librte_pipeline dependency list truncation
Date: Tue, 21 Jun 2016 10:58:47 +0000 [thread overview]
Message-ID: <3EB4FA525960D640B5BDFFD6A3D8912647A0A75A@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <fdc869f8-d003-bb84-0284-2ba627bd0fb7@redhat.com>
> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Tuesday, June 21, 2016 11:45 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; 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 <pmatilai@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
next prev parent reply other threads:[~2016-06-21 10:58 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-21 8:11 Panu Matilainen
2016-06-21 8:11 ` [dpdk-dev] [PATCH 2/3] pdump: fix missing dependency on libpthread Panu Matilainen
2016-06-21 8:11 ` [dpdk-dev] [PATCH 3/3] mk: fail build on incomplete shared library dependencies Panu Matilainen
2016-06-21 10:01 ` [dpdk-dev] [PATCH 1/3] mk: fix librte_pipeline dependency list truncation Dumitrescu, Cristian
2016-06-21 10:25 ` Panu Matilainen
2016-06-21 10:31 ` Bruce Richardson
2016-06-21 10:44 ` Panu Matilainen
2016-06-21 10:58 ` Dumitrescu, Cristian [this message]
2016-06-21 11:53 ` Panu Matilainen
2016-06-26 16:41 ` [dpdk-dev] [PATCH v2 0/7] fix dependencies Thomas Monjalon
2016-06-26 16:41 ` [dpdk-dev] [PATCH v2 1/7] mk: remove traces of combined library Thomas Monjalon
2016-06-26 16:42 ` [dpdk-dev] [PATCH v2 2/7] mk: fix external library link Thomas Monjalon
2016-06-26 16:42 ` [dpdk-dev] [PATCH v2 3/7] pipeline: fix truncated dependency list Thomas Monjalon
2016-06-27 9:20 ` Dumitrescu, Cristian
2016-06-27 9:27 ` Thomas Monjalon
2016-06-26 16:42 ` [dpdk-dev] [PATCH v2 4/7] mk: fix internal dependencies Thomas Monjalon
2016-06-26 16:42 ` [dpdk-dev] [PATCH v2 5/7] mk: fix external dependencies of crypto drivers Thomas Monjalon
2016-06-27 8:48 ` Panu Matilainen
2016-06-26 16:42 ` [dpdk-dev] [PATCH v2 6/7] pdump: fix missing dependency on libpthread Thomas Monjalon
2016-06-26 16:42 ` [dpdk-dev] [PATCH v2 7/7] mk: check shared library dependencies Thomas Monjalon
2016-06-27 15:15 ` [dpdk-dev] [PATCH v2 0/7] fix dependencies Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3EB4FA525960D640B5BDFFD6A3D8912647A0A75A@IRSMSX108.ger.corp.intel.com \
--to=cristian.dumitrescu@intel.com \
--cc=bruce.richardson@intel.com \
--cc=christian.ehrhardt@canonical.com \
--cc=dev@dpdk.org \
--cc=pmatilai@redhat.com \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).