From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id B02C85A31 for ; Wed, 13 May 2015 16:35:15 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 13 May 2015 07:34:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,421,1427785200"; d="scan'208";a="728571956" Received: from orsmsx101.amr.corp.intel.com ([10.22.225.128]) by orsmga002.jf.intel.com with ESMTP; 13 May 2015 07:34:46 -0700 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by ORSMSX101.amr.corp.intel.com (10.22.225.128) with Microsoft SMTP Server (TLS) id 14.3.224.2; Wed, 13 May 2015 07:34:46 -0700 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.51]) by FMSMSX109.amr.corp.intel.com ([169.254.15.53]) with mapi id 14.03.0224.002; Wed, 13 May 2015 07:34:46 -0700 From: "Wiles, Keith" To: Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v8 1/2] mk:Simplify the ifdefs in rte.app.mk Thread-Index: AQHQjOeCjmIzPd/5n0GFkZB2ucTpFZ15+9qAgAAKIwCAAF6+gP//rnoAgABagoD//63pAA== Date: Wed, 13 May 2015 14:34:45 +0000 Message-ID: References: <1431386066-6147-1-git-send-email-keith.wiles@intel.com> <1431457872-10345-1-git-send-email-keith.wiles@intel.com> <5553000D.3030004@6wind.com> <55535807.5070900@6wind.com> <55535F91.5000405@6wind.com> In-Reply-To: <55535F91.5000405@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.90.134] Content-Type: text/plain; charset="us-ascii" Content-ID: <4CE6ABF3A0225C49B011C0265142F6B1@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Wed, 13 May 2015 14:35:16 -0000 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 +=3D $(CPU_LDLIBS) >>>>>> +LDLIBS +=3D $(_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? >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 >>> >> >