From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f45.google.com (mail-wg0-f45.google.com [74.125.82.45]) by dpdk.org (Postfix) with ESMTP id 9F73911C5 for ; Wed, 4 Mar 2015 13:55:27 +0100 (CET) Received: by wggz12 with SMTP id z12so9287280wgg.9 for ; Wed, 04 Mar 2015 04:55:27 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=mfvDyAHsRsXG7fWMOFP4Xvs2dVp7k5vFSx5drWa4Ids=; b=PAtuCEMsKhUC62W7VDQmgc8x7AbaRNUBExvaOYtqc69ljSx1Q/GNStwD8HnNcgt/FT 3EGR4uzjXZKPFXusO7wh7VlK3BmJMEVsMRZlVGNeZMx7iJYc9qH5avHZhSD92gwIcGOI LTlqP6nYEmLy9kVxJeJUT9LmIGnrQTkwitv83CD+B2F873O2AduMZokZ7ZsqLqlptepP trqYrcixWVVRME2QenpO+17doOq4CyM/FXrihVGVcHUI03cIrY61QBBDW+Z3mX+B1/ud ndzNc7izqghaejVAo90LrHRpWs7Xhly+6ydbRSiJx76Ltx0gy6CiTRvK3NfY5mKYAjtr FWFg== X-Gm-Message-State: ALoCoQlwY54RoQFJjUo9Ou79wSPcsbRtMIuMNF2YQtVoIa23jq9V/fv9CplvKNqfZNyOIdC+sNbx X-Received: by 10.180.106.70 with SMTP id gs6mr55606118wib.39.1425473725823; Wed, 04 Mar 2015 04:55:25 -0800 (PST) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by mx.google.com with ESMTPSA id fa3sm3287778wib.17.2015.03.04.04.55.24 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Mar 2015 04:55:25 -0800 (PST) From: Thomas Monjalon To: Neil Horman Date: Wed, 04 Mar 2015 13:54:49 +0100 Message-ID: <3858426.kLEMBMBQqi@xps13> Organization: 6WIND User-Agent: KMail/4.14.4 (Linux/3.18.4-1-ARCH; KDE/4.14.4; x86_64; ; ) In-Reply-To: <20150304114951.GB5808@hmsreliant.think-freely.org> References: <1422652596-12777-1-git-send-email-nhorman@tuxdriver.com> <5425830.nAiNf2ERY3@xps13> <20150304114951.GB5808@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2] ABI: Add abi checking utility 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, 04 Mar 2015 12:55:27 -0000 Hi Neil, I remove parts that I agree and reply to those which deserve more discussion. 2015-03-04 06:49, Neil Horman: > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote: > > 2015-02-02 13:18, Neil Horman: > > > +# Validate that we have all the arguments we need > > > +res=$(validate_args) > > > +if [ -n "$res" ] > > > +then > > > + echo $res > > > > Should be redirected to stderr >&2 > > > Why? this is eactly what I intended. All the other messages from log are > directed to stdout, so should this be. I'm wondering if there's some normal output which could be redirected for further processing, and some error output. My comment was not only for this log but also for every error message. > > I guess this is the tool: > > http://ispras.linuxbase.org/index.php/ABI_compliance_checker > > Correct. So maybe you should add this URL in the commit log. > > > +log "INFO" "We're going to check and make sure that applications built" > > > +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed" > > > +log "INFO" "against DPDK DSOs built from tag $TAG2." > > > +log "INFO" "" > > > +if [ ! -d ./.git ] > > > +then > > > + log "WARN" "You must be in the root of the dpdk git tree" > > > + log "WARN" "You are in $PWD" > > > + cleanup_and_exit 1 > > > +fi > > > > Why not cd $(dirname $0)/.. instead of returning an error? > > Why would that help in finding the base of the git tree. Theres no guarantee > that you are in a subdirectory of a git tree. I suppose we can try it > recursively until we hit /, but it seems just as easy and clear to tell the user > whats needed. No I'm saying that you could avoid this check by going into the right directory from the beginning. We know that the root dir is $(dirname $0)/.. because this script is in scripts/ directory. > > > +# Make sure we configure SHARED libraries > > > +# Also turn off IGB and KNI as those require kernel headers to build > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET > > > > Why not tuning configuration after make config in .config file? > > > Because this way we save a reconfig (from a developer viewpoint), you should run > make config again after changing configs, and so this way you save doing that. No, you run make config once and update .config file. That's the recommended way to configure DPDK. defconfig files are default configurations and should stay read-only. > > > +for i in `ls *.so` > > > > I think ls is useless. > > > Um, I don't? Not sure what you're getting at here. I think "for i in *.so" should work. > > > + #compare the abi dumps > > > + $ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME > > > > Do we need to do a visual check? I didn't try yet. > > > Yes, it generates an html report of all the symbols exported in a build and > compares them with the alternate version. That needs manual review. OK I think it's important to explain in the commit log. > > So you compare the ABI dumps. > > Do we also need to run an app from TAG2 with libs from TAG1? > > I started down that path, but its not really that helpful, as all it will do is > refuse to run if there is a symbol missing from a later version. While that > might be helpful, its no where near as through as the full report from the > compliance checker. > > The bottom line is that real ABI compliance requires a developer to be aware of > the changes going into the code and how they affect binary layout. A simple > "does it still work" test isn't sufficient. I hope we'll be able to integrate this kind of tool in an automated sanity check in order to find obvious errors. Thanks