Re: [dpdk-dev] [PATCHv6 1/7] pmdinfogen: Add buildtools and pmdinfogen utility

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Jun 07, 2016 at 11:57:42AM +0200, Thomas Monjalon wrote:
> 2016-05-31 09:57, Neil Horman:
> > +++ b/buildtools/Makefile
> > @@ -0,0 +1,36 @@
> > +#   BSD LICENSE
> > +#
> > +#   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> > +#   All rights reserved.
> 
> I really think it is a strange copyright for a new empty file.
> 
Its not empty, It lists the subdirectories to build.  And given that the DPDK is
licensed under multiple licenses (BSD/GPL/LGPL), it introduces confusion to not
call out the license in a specific file, file size is really irrelevant to that.

> > +#if __x86_64__ || __aarch64__
> 
> Better to use CONFIG_RTE_ARCH_64.
> 
I'm not sure why, given that every supported compiler defines the arches I use,
but sure, fine.

> > +#define TO_NATIVE(x) (x)
> 
> We already have some functions for endianness in
> lib/librte_eal/common/include/generic/rte_byteorder.h
> 
Very well, I'll use the rte byteorder macros.

> > +struct elf_info {
> > +	unsigned long size;
> > +	Elf_Ehdr     *hdr;
> > +	Elf_Shdr     *sechdrs;
> > +	Elf_Sym      *symtab_start;
> > +	Elf_Sym      *symtab_stop;
> > +	Elf_Section  export_sec;
> > +	Elf_Section  export_unused_sec;
> > +	Elf_Section  export_gpl_sec;
> > +	Elf_Section  export_unused_gpl_sec;
> > +	Elf_Section  export_gpl_future_sec;
> > +	char         *strtab;
> 
> The export_* fields are not used.
> 
Fine, I'll remove them.

> > --- /dev/null
> > +++ b/mk/rte.buildtools.mk
> 
> I'm sorry I really do not agree it is a good practice to create a new
> makefile type just for a new directory.
> My opinion is that you should use and improve rte.hostapp.mk to make
> it usable for possible other host apps.
> 
I am so exhausted by this argument.

They are the same file Thomas.  I'm not sure how you don't see that.  I've
explained to you that they are, with the exception of whitespace noise,
identical.  buildtools is a better nomenclature because it more closely
describes what is being built at the moment.  The only reason we still have
hostapp is because you didn't remove it when you removed the applications that,
in your own words from the commit log, are "useless".  The argument that we
should keep the build file, and its naming convention on the off chance that
someone might use it in the future really doesn't hold water with me, at least
not to the point that, when we have something that duplicates its function we
should do anything other than take the path of least resistance to make it work.
I'm not sure how you expected anyone to know there is a makefile in place in the
DPDK to build local application, when there are currently no applications in
place, but asking people to use it after the fact is really just the height of
busywork.  If it was already building other utilities, I'd feel differently, but
given that its just sitting there, a vestigual file, makes this all just silly.

But clearly, this isn't going to be done until I do what you want, regardless of
what either of us think of it, So I'll make the change.

Neil