From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 93CEF5A49 for ; Thu, 14 May 2015 14:21:17 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1YssCs-0004wu-0r; Thu, 14 May 2015 14:25:46 +0200 Message-ID: <55549337.2040106@6wind.com> Date: Thu, 14 May 2015 14:21:11 +0200 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: "Wiles, Keith" , "dev@dpdk.org" References: In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v8 1/2] mk:Simplify the ifdefs in rte.app.mk 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: Thu, 14 May 2015 12:21:17 -0000 Hi Keith, On 05/13/2015 04:43 PM, Wiles, Keith wrote: > > > On 5/13/15, 9:28 AM, "Olivier MATZ" wrote: > >> >> On 05/13/2015 04:04 PM, Wiles, Keith wrote: >>> >>> >>> On 5/13/15, 8:56 AM, "Olivier MATZ" 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. It could be added at the same place than other CPU_* variables, in another patch. Olivier > > 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 >>>> >>> >> > >