From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <bruce.richardson@intel.com> Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 5DEB4CD0C for <dev@dpdk.org>; Fri, 1 May 2015 11:09:54 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP; 01 May 2015 02:09:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,350,1427785200"; d="scan'208";a="718949108" Received: from unknown ([10.237.220.106]) by fmsmga002.fm.intel.com with SMTP; 01 May 2015 02:09:50 -0700 Received: by (sSMTP sendmail emulation); Fri, 01 May 2015 10:09:50 +0025 Date: Fri, 1 May 2015 10:09:50 +0100 From: Bruce Richardson <bruce.richardson@intel.com> To: "Wiles, Keith" <keith.wiles@intel.com> Message-ID: <20150501090949.GA7992@bricha3-MOBL3> References: <1430321158-17805-1-git-send-email-keith.wiles@intel.com> <5541F9C1.9050701@6wind.com> <D16796D9.1E5CF%keith.wiles@intel.com> <5542305D.9070505@6wind.com> <D167A479.1E5E6%keith.wiles@intel.com> <20150430162255.GA5708@bricha3-MOBL3> <D167C366.1E6DE%keith.wiles@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <D167C366.1E6DE%keith.wiles@intel.com> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "dev@dpdk.org" <dev@dpdk.org> Subject: Re: [dpdk-dev] [PATCH v4 1/2] 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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Fri, 01 May 2015 09:09:55 -0000 On Thu, Apr 30, 2015 at 05:33:36PM +0100, Wiles, Keith wrote: > > > On 4/30/15, 11:22 AM, "Richardson, Bruce" <bruce.richardson@intel.com> > wrote: > > >On Thu, Apr 30, 2015 at 02:31:13PM +0000, Wiles, Keith wrote: > >> > >> > >> On 4/30/15, 8:38 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote: > >> > >> >Hi Keith, > >> > > >> >On 04/30/2015 03:24 PM, Wiles, Keith wrote: > >> >> > >> >> > >> >> On 4/30/15, 4:45 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote: > >> >> > >> >>> Hi Keith, > >> >>> > >> >>> Thank you for submitting a clean-up. Please see some comments below. > >> >>> > >> >>> On 04/29/2015 05:25 PM, Keith Wiles wrote: > >> >>>> Trying to simplify the ifdefs in rte.app.mk to make the code > >> >>>> more readable and maintainable by moving LDLIBS variable to use > >> >>>> the same style as LDLIBS-y being used in the rest of the code. > >> >>>> > >> >>>> Added a new variable called EXTRA_LDLIBS to be used by example apps > >> >>>> instead of using LDLIBS directly. > >> >>> > >> >>> If I understand well, the goal of this patch is only a cleanup in > >> >>> rte.app.mk, but at the end, it changes the makefile user "API", > >> >>> which could probably be a problem for applications using the > >> >>> dpdk makefile framework. > >> >>> > >> >>> Why not just having an temporary internal variable (let's say > >> >>> _LDLIBS-y) that would allow to do the clean-up without modifying > >> >>> the user interface? > >> >>> > >> >>> Also, with your patch, the approach for EXTRA_LDLIBS would be > >> >>> different than CFLAGS or LDFLAGS: > >> >>> - CFLAGS/LDFLAGS are in Makefiles only > >> >>> - EXTRA_CFLAGS/EXTRA_LDFLAGS are prefered in command line > >> >>> to add flags to the default ones > >> >>> > >> >>> I'm not opposed to add EXTRA_LDLIBS in addition to LDLIBS, > >> >>> keeping a compatibility with existing application Makefiles. > >> >> > >> >> The docs for DPDK 28.3.6 states they can be used for both command > >>line > >> >>and > >> >> Makefile, so I think I like the current solution unless everyone > >>wants > >> >>it > >> >> as you suggested. > >> >> > >> >> > >> > >>>>http://dpdk.readthedocs.org/en/v2.0.0/prog_guide/dev_kit_build_system.h > >>>>tm > >> >>l > >> > > >> > From the link you have sent: > >> > > >> >- About CFLAGS: > >> > > >> >"28.3.4. Variables that Can be Set/Overridden in a Makefile Only > >> >[...] > >> >CFLAGS: Flags to use for C compilation. The user should use += to > >>append > >> >data in this variable." > >> > > >> >nothing in 28.3.6 > >> > > >> > > >> >- About EXTRA_CFLAGS: > >> > > >> >nothing in 28.3.4 > >> > > >> >"28.3.6. Variables that Can be Set/Overridden by the User in a Makefile > >> >or Command Line > >> >[...] > >> >EXTRA_CFLAGS: The content of this variable is appended after CFLAGS > >>when > >> >compiling." > >> > >> The point was that EXTRA_XXX can be used for command line and Makefile > >>as > >> it was pointed out in a previous email the assumption was EXTRA_XXX was > >> only for the command line. (Just to make sure we understood EXTRA_XXX > >>was > >> not just for command line options.) This was the reason I sent the link > >>an > >> to point out using EXTRA_XXX is a much cleaner method then allowing > >> someone to modify what I believe is an internal variable. > > > >Just beware that setting EXTRA_* flags on the commandline can override > >their > >values in the makefiles, and cause unexpected compilation problems. > >Therefore, > >it tends to be best to avoid using the EXTRA_* variables for variables > >essential > >to compile. For example: putting "-g -O3" in EXTRA_CFLAGS is ok, as the > >if the > >useroverrides those with something else things should still work, but > >putting > >"-I/path/to/include" would not be. > > On the command line and makefile you should be using += and not just = or > you run into this problem. Using "+=" on the commandline is not normal. It's also rather tricky to do at times, if a value is already defined, as bash shell does not add in whitespace to the existing variable appropriately. For example, I have EXTRA_CFLAGS set to '-g -Wfatal-errors' in my .bashrc so I always get debug builds that stop on first error. Building the dpdk.org as below works fine: EXTRA_CFLAGS=-g gmake == Build lib == Build lib/librte_compat ... However, using += causes very strange behaviour: $ EXTRA_CFLAGS+=-g gmake == Build lib == Build lib/librte_compat == Build lib/librte_eal == Build lib/librte_net == Build lib/librte_eal/common == Build lib/librte_eal/bsdapp == Build lib/librte_eal/bsdapp/eal == Build lib/librte_eal/bsdapp/contigmem == Build lib/librte_eal/bsdapp/nic_uio Warning: Object directory not changed from original /usr/home/bruce/dpdk.org/x86_64-native-bsdapp-clang/build/lib/librte_eal/bsdapp/nic_uio Warning: Object directory not changed from original /usr/home/bruce/dpdk.org/x86_64-native-bsdapp-clang/build/lib/librte_eal/bsdapp/contigmem CC eal.o CC eal_memory.o CC eal_hugepage_info.o CC eal_thread.o error: unknown warning option '-Wfatal-errors=g' [-Werror,-Wunknown-warning-option] So, in short, you must assume that EXTRA_CFLAGS will be specified using "=" on the commandline, and that things build appropriately when the user does so. [Given you use "+=" internally in the makefiles, I assume this is the case, but flagging this as something worth double-checking] /Bruce