From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 6FD815683 for ; Wed, 4 Mar 2015 15:39:53 +0100 (CET) Received: from nat-pool-rdu-t.redhat.com ([66.187.233.202] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1YTASg-0003PG-C3; Wed, 04 Mar 2015 09:39:52 -0500 Date: Wed, 4 Mar 2015 09:39:40 -0500 From: Neil Horman To: Thomas Monjalon Message-ID: <20150304143940.GA6187@neilslaptop.think-freely.org> References: <1422652596-12777-1-git-send-email-nhorman@tuxdriver.com> <5425830.nAiNf2ERY3@xps13> <20150304114951.GB5808@hmsreliant.think-freely.org> <3858426.kLEMBMBQqi@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3858426.kLEMBMBQqi@xps13> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No 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 14:39:53 -0000 On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote: > 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. > No, the report output is in html format and always to a file, so stdout isn't used for any inline informational reporting. > > > 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. > sure, fine. > > > > +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. > That only helps if you start from the right directory. If you run this command from some other location, your solution just breaks. > > > > +# 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. They get overwritten when we do the git resets. Its silly to modify your config file after you run make config, in the event the make target has to re-read any modified options and adjust dependent config files accordingly. I understand that doesn't happen now, but its common practice for every open source project in existance. > > > > > +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. > Then its irrelevant in my mind. They both work equally well. > > > > + #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. Ok. > > > > 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 >