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 >
