From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f172.google.com (mail-wi0-f172.google.com [209.85.212.172]) by dpdk.org (Postfix) with ESMTP id 0F0B45697 for ; Wed, 13 May 2015 16:28:36 +0200 (CEST) Received: by widdi4 with SMTP id di4so58201878wid.0 for ; Wed, 13 May 2015 07:28:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=AObzXS1jhy6oebgs/4xzVI2h7DcnkbeFQnky1f6/W1U=; b=Ubu0f1DYXAbwfpcFUAXW/WnPuJoctWTvEn5dDBPpICQQbkhVAJDJ4ADVi93WpOxYNG RdGhPZ1kZUUUYldMsgTAOecaYTdc6s5Vr/SR2b1AEUlUC2H3YeMH46UZvyeUTZ2403y4 MV2xFsoQQOcWtDysTwlP0YojY5c63iNQo2BzoUPPisfwLUn1DOcfnvhfYgGglcOf14HF 6kMZmd9KR8kvUO1sKCyt1LT67b39oIOFfRYaPgEpAmRwy7N81Qa8MEvUA8z8DvRzpRbH tRSlsc4X4t1z4eyFrPq3PccmvJzzny2oCy1jKhHGXLOUJ8/T8CMUq3wZdJN7mJZTFytf iIOg== X-Gm-Message-State: ALoCoQnm4uQ2/oVeLgTT1U1Z66SWhw13/hI0ouBSvenUpQS3I7xmweDqskNN0+wGaSRABaZ8DbRk X-Received: by 10.180.79.227 with SMTP id m3mr40022812wix.71.1431527315933; Wed, 13 May 2015 07:28:35 -0700 (PDT) Received: from [10.16.0.195] (6wind.net2.nerim.net. [213.41.180.237]) by mx.google.com with ESMTPSA id mc20sm8353595wic.15.2015.05.13.07.28.34 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 13 May 2015 07:28:35 -0700 (PDT) Message-ID: <55535F91.5000405@6wind.com> Date: Wed, 13 May 2015 16:28:33 +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: <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> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed 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: Wed, 13 May 2015 14:28:36 -0000 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 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 >> >