DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [dpdk-dev] [PATCH v8 1/2] mk:Simplify the ifdefs in rte.app.mk
@ 2015-05-13 14:43 Wiles, Keith
  2015-05-14 12:21 ` Olivier MATZ
  0 siblings, 1 reply; 21+ messages in thread
From: Wiles, Keith @ 2015-05-13 14:43 UTC (permalink / raw)
  To: Olivier MATZ, dev



On 5/13/15, 9:28 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:

>
>On 05/13/2015 04:04 PM, Wiles, Keith wrote:
>>
>>
>> On 5/13/15, 8:56 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
>>
>>> Hi Keith,
>>>
>>> On 05/13/2015 03:17 PM, Wiles, Keith wrote:
>>>>>>
>>>>>>     endif # ifeq ($(NO_AUTOLIBS),)
>>>>>>
>>>>>> -LDLIBS += $(CPU_LDLIBS)
>>>>>> +LDLIBS += $(_LDLIBS-y) $(EXTRA_LDLIBS)
>>>>>>
>>>>>
>>>>> As discussed in the previous mail, all things that are about
>>>>> EXTRA_LDLIBS should be moved in the second patch. Therefore,
>>>>> the title of the second patch should not be "update doc...", but
>>>>> something like "mk: introduce EXTRA_LDLIBS...".
>>>>>
>>>>> By the way, I missed that before, but it seems that your
>>>>> patch removes CPU_LDLIBS, I don't think it's correct.
>>>>
>>>> I found no reference to CPU_LDLIBS in the docs or code other then then
>>>> one
>>>> line. We now have EXTRA_LDLIBS for the command line, right?
>>>
>>> Yes, but your patch says "simplify the ifdef". Removing
>>> a variable (even if it is not used) in this patch is not
>>> a good idea.
>>>
>>> Now, the CPU_CFLAGS, CPU_LDFLAGS, CPU_LDLIBS can be defined internally
>>> by the rte.vars.mk in mk/arch/ or mk/machine/ directories.
>>
>> No docs for CPU_LDLIBS or reference to that variable, which means it
>>does
>> not exist, right?
>> If it was used or documented then I would agree. Having magic variables
>>is
>> not a good idea. I will add the CPU_LDLIBS in to the line, but someone
>> will have to document that variable.
>
>First, this variable is internal to DPDK framework. It is not

Not referenced in the code any place except the one line and not
referenced in the docs, which means no one used that variable. By
definition this is some magic variable that someone could use only because
he knew about that variable.

As I stated before, I will add that variable back, but it must be
documented, correct?

Look at the code a number of CPU_XXXX flags are commented about in the
code.

machine/power8/rte.vars.mk:39:#   - can define CPU_CFLAGS variable
(overridden by cmdline value) that
machine/power8/rte.vars.mk:41:#   - can define CPU_LDFLAGS variable
(overridden by cmdline value) that
machine/power8/rte.vars.mk:43:#   - can define CPU_ASFLAGS variable
(overridden by cmdline value) that
machine/power8/rte.vars.mk:53:# CPU_CFLAGS =
machine/power8/rte.vars.mk:54:# CPU_LDFLAGS =
machine/power8/rte.vars.mk:55:# CPU_ASFLAGS =
machine/wsm/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
by cmdline value) that
machine/wsm/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
machine/wsm/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
machine/wsm/rte.vars.mk:54:# CPU_CFLAGS =
machine/wsm/rte.vars.mk:55:# CPU_LDFLAGS =
machine/wsm/rte.vars.mk:56:# CPU_ASFLAGS =
machine/native/rte.vars.mk:40:#   - can define CPU_CFLAGS variable
(overriden by cmdline value) that
machine/native/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
machine/native/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
machine/native/rte.vars.mk:54:# CPU_CFLAGS =
machine/native/rte.vars.mk:55:# CPU_LDFLAGS =
machine/native/rte.vars.mk:56:# CPU_ASFLAGS =
machine/native/rte.vars.mk:66:  CPU_SSE42_SUPPORT = $(shell grep SSE4\.2
/var/run/dmesg.boot 2>/dev/null)
machine/native/rte.vars.mk:67:  ifneq ($(CPU_SSE42_SUPPORT),)
machine/atm/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
by cmdline value) that
machine/atm/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
machine/atm/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
machine/atm/rte.vars.mk:54:# CPU_CFLAGS =
machine/atm/rte.vars.mk:55:# CPU_LDFLAGS =
machine/atm/rte.vars.mk:56:# CPU_ASFLAGS =
machine/hsw/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
by cmdline value) that
machine/hsw/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
machine/hsw/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
machine/hsw/rte.vars.mk:54:# CPU_CFLAGS =
machine/hsw/rte.vars.mk:55:# CPU_LDFLAGS =
machine/hsw/rte.vars.mk:56:# CPU_ASFLAGS =
machine/ivb/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
by cmdline value) that
machine/ivb/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
machine/ivb/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
machine/ivb/rte.vars.mk:54:# CPU_CFLAGS =
machine/ivb/rte.vars.mk:55:# CPU_LDFLAGS =
machine/ivb/rte.vars.mk:56:# CPU_ASFLAGS =
machine/snb/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
by cmdline value) that
machine/snb/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
machine/snb/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
machine/snb/rte.vars.mk:54:# CPU_CFLAGS =
machine/snb/rte.vars.mk:55:# CPU_LDFLAGS =
machine/snb/rte.vars.mk:56:# CPU_ASFLAGS =
machine/nhm/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
by cmdline value) that
machine/nhm/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
machine/nhm/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
machine/nhm/rte.vars.mk:54:# CPU_CFLAGS =
machine/nhm/rte.vars.mk:55:# CPU_LDFLAGS =
machine/nhm/rte.vars.mk:56:# CPU_ASFLAGS =
machine/default/rte.vars.mk:40:#   - can define CPU_CFLAGS variable
(overriden by cmdline value) that
machine/default/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
machine/default/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
machine/default/rte.vars.mk:54:# CPU_CFLAGS =
machine/default/rte.vars.mk:55:# CPU_LDFLAGS =
machine/default/rte.vars.mk:56:# CPU_ASFLAGS =
rte.lib.mk:43:CPU_LDFLAGS += --version-script=$(SRCDIR)/$(EXPORT_MAP)
rte.lib.mk:67:LD := $(CC) $(CPU_CFLAGS)
rte.lib.mk:68:_CPU_LDFLAGS := $(call linkerprefix,$(CPU_LDFLAGS))
rte.lib.mk:70:_CPU_LDFLAGS := $(CPU_LDFLAGS)
rte.lib.mk:82:O_TO_S = $(LD) $(_CPU_LDFLAGS) -shared $(OBJS-y)
-Wl,-soname,$(LIB) -o $(LIB)
rte.app.mk:144:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
arch/x86_64/rte.vars.mk:39:#   - define CPU_CFLAGS variable (overriden by
cmdline or previous
arch/x86_64/rte.vars.mk:41:#   - define CPU_LDFLAGS variable (overriden by
cmdline or previous
arch/x86_64/rte.vars.mk:43:#   - define CPU_ASFLAGS variable (overriden by
cmdline or previous
arch/x86_64/rte.vars.mk:55:CPU_CFLAGS  ?= -m64
arch/x86_64/rte.vars.mk:56:CPU_LDFLAGS ?=
arch/x86_64/rte.vars.mk:57:CPU_ASFLAGS ?= -felf64
arch/x86_64/rte.vars.mk:59:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
CPU_ASFLAGS
arch/i686/rte.vars.mk:39:#   - define CPU_CFLAGS variable (overriden by
cmdline or previous
arch/i686/rte.vars.mk:41:#   - define CPU_LDFLAGS variable (overriden by
cmdline or previous
arch/i686/rte.vars.mk:43:#   - define CPU_ASFLAGS variable (overriden by
cmdline or previous
arch/i686/rte.vars.mk:55:CPU_CFLAGS  ?= -m32
arch/i686/rte.vars.mk:56:CPU_LDFLAGS ?= -melf_i386
arch/i686/rte.vars.mk:57:CPU_ASFLAGS ?= -felf
arch/i686/rte.vars.mk:59:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
CPU_ASFLAGS
arch/x86_x32/rte.vars.mk:39:#   - define CPU_CFLAGS variable (overridden
by cmdline or previous
arch/x86_x32/rte.vars.mk:41:#   - define CPU_LDFLAGS variable (overridden
by cmdline or previous
arch/x86_x32/rte.vars.mk:43:#   - define CPU_ASFLAGS variable (overridden
by cmdline or previous
arch/x86_x32/rte.vars.mk:54:CPU_CFLAGS  ?= -mx32
arch/x86_x32/rte.vars.mk:55:CPU_LDFLAGS ?= -melf32_x86_64
arch/x86_x32/rte.vars.mk:56:#CPU_ASFLAGS ?= -felf64
arch/x86_x32/rte.vars.mk:59:ifneq ($(shell echo | $(CC) $(CPU_CFLAGS) -E -
2>/dev/null 1>/dev/null && echo 0), 0)
arch/x86_x32/rte.vars.mk:63:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
CPU_ASFLAGS
arch/ppc_64/rte.vars.mk:35:CPU_CFLAGS  ?= -m64 -DRTE_CACHE_LINE_SIZE=128
arch/ppc_64/rte.vars.mk:36:CPU_LDFLAGS ?=
arch/ppc_64/rte.vars.mk:37:CPU_ASFLAGS ?= -felf64
arch/ppc_64/rte.vars.mk:39:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
CPU_ASFLAGS
target/generic/rte.vars.mk:46:#   - can define CPU_CFLAGS variable
(overriden by cmdline value) that
target/generic/rte.vars.mk:48:#   - can define CPU_LDFLAGS variable
(overriden by cmdline value) that
target/generic/rte.vars.mk:50:#   - can define CPU_ASFLAGS variable
(overriden by cmdline value) that
target/generic/rte.vars.mk:64:#   - define CPU_CFLAGS variable (overriden
by cmdline or previous
target/generic/rte.vars.mk:66:#   - define CPU_LDFLAGS variable (overriden
by cmdline or previous
target/generic/rte.vars.mk:68:#   - define CPU_ASFLAGS variable (overriden
by cmdline or previous
target/generic/rte.vars.mk:110:CFLAGS := $(CPU_CFLAGS) $(EXECENV_CFLAGS)
$(TOOLCHAIN_CFLAGS) $(MACHINE_CFLAGS)
target/generic/rte.vars.mk:114:LDFLAGS := $(CPU_LDFLAGS)
$(EXECENV_LDFLAGS) $(TOOLCHAIN_LDFLAGS) $(MACHINE_LDFLAGS)
target/generic/rte.vars.mk:118:ASFLAGS := $(CPU_ASFLAGS)
$(EXECENV_ASFLAGS) $(TOOLCHAIN_ASFLAGS) $(MACHINE_ASFLAGS)
rte.sharelib.mk:52:LD := $(CC) $(CPU_CFLAGS)
rte.sharelib.mk:53:O_TO_S = $(LD) $(call linkerprefix,$(CPU_LDFLAGS)) \
rte.sharelib.mk:56:O_TO_S = $(LD) $(CPU_LDFLAGS) \



>documented, because the goal of the documentation is not to
>document all internal variables. In one word, it's not a magic
>variable at all, it is simply a variable.
>
>Now, the heart of the matter is that your patch silently remove
>this line. This is not acceptable and that's why I'm commenting
>on it. The goal of patch splitting and proper title is to separate
>features, and win time when searching in the git log for the cause
>of a problem.
>
>This is the same problem with EXTRA_LDLIBS. How can you justify
>having 2 patches:
>- "simplify the ifdef" that also adds the EXTRA_LDLIBS
>  then
>- "update the documentation for EXTRA_LDLIBS"
>
>Regards,
>Olivier
>
>
>
>>>
>>> Regards,
>>> Olivier
>>>
>>
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-05-14 21:39 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <017274>
2015-05-11 23:14 ` [dpdk-dev] [PATCH v7 1/2] Simplify the ifdefs in rte.app.mk Keith Wiles
2015-05-11 23:14   ` [dpdk-dev] [PATCH v7 2/2] Update Docs for new EXTRA_LDLIBS variable Keith Wiles
2015-05-12 15:44   ` [dpdk-dev] [PATCH v7 1/2] Simplify the ifdefs in rte.app.mk Olivier MATZ
2015-05-12 19:11   ` [dpdk-dev] [PATCH v8 1/2] mk:Simplify " Keith Wiles
2015-05-12 19:11     ` [dpdk-dev] [PATCH v8 2/2] mk:Update Docs for new EXTRA_LDLIBS variable Keith Wiles
2015-05-13  7:41     ` [dpdk-dev] [PATCH v8 1/2] mk:Simplify the ifdefs in rte.app.mk Olivier MATZ
2015-05-13 13:17       ` Wiles, Keith
2015-05-13 13:56         ` Olivier MATZ
2015-05-13 14:04           ` Wiles, Keith
2015-05-13 14:28             ` Olivier MATZ
2015-05-13 14:34               ` Wiles, Keith
2015-05-13 16:35   ` [dpdk-dev] [PATCH v9 1/2] mk:Simplify the ifdefs in the makefile Keith Wiles
2015-05-13 16:35     ` [dpdk-dev] [PATCH v9 2/2] mk:Introduce the EXTRA_LDLIBS variable Keith Wiles
2015-05-14 12:30     ` [dpdk-dev] [PATCH v9 1/2] mk:Simplify the ifdefs in the makefile Olivier MATZ
2015-05-14 15:14       ` Wiles, Keith
2015-05-14 14:21   ` [dpdk-dev] [PATCH v10 " Keith Wiles
2015-05-14 14:21     ` [dpdk-dev] [PATCH v10 2/2] mk:Introduce the EXTRA_LDLIBS variable Keith Wiles
2015-05-14 15:42     ` [dpdk-dev] [PATCH v10 1/2] mk:Simplify the ifdefs in the makefile Olivier MATZ
2015-05-14 21:38       ` Thomas Monjalon
2015-05-13 14:43 [dpdk-dev] [PATCH v8 1/2] mk:Simplify the ifdefs in rte.app.mk Wiles, Keith
2015-05-14 12:21 ` Olivier MATZ

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