Re: [dpdk-dev] [PATCH v4 1/2] Simplify the ifdefs in rte.app.mk.

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Apr 30, 2015 at 05:33:36PM +0100, Wiles, Keith wrote:
> 
> 
> On 4/30/15, 11:22 AM, "Richardson, Bruce" <bruce.richardson@intel.com>
> wrote:
> 
> >On Thu, Apr 30, 2015 at 02:31:13PM +0000, Wiles, Keith wrote:
> >> 
> >> 
> >> On 4/30/15, 8:38 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
> >> 
> >> >Hi Keith,
> >> >
> >> >On 04/30/2015 03:24 PM, Wiles, Keith wrote:
> >> >>
> >> >>
> >> >> On 4/30/15, 4:45 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
> >> >>
> >> >>> Hi Keith,
> >> >>>
> >> >>> Thank you for submitting a clean-up. Please see some comments below.
> >> >>>
> >> >>> On 04/29/2015 05:25 PM, Keith Wiles wrote:
> >> >>>> Trying to simplify the ifdefs in rte.app.mk to make the code
> >> >>>> more readable and maintainable by moving LDLIBS variable to use
> >> >>>> the same style as LDLIBS-y being used in the rest of the code.
> >> >>>>
> >> >>>> Added a new variable called EXTRA_LDLIBS to be used by example apps
> >> >>>> instead of using LDLIBS directly.
> >> >>>
> >> >>> If I understand well, the goal of this patch is only a cleanup in
> >> >>> rte.app.mk, but at the end, it changes the makefile user "API",
> >> >>> which could probably be a problem for applications using the
> >> >>> dpdk makefile framework.
> >> >>>
> >> >>> Why not just having an temporary internal variable (let's say
> >> >>> _LDLIBS-y) that would allow to do the clean-up without modifying
> >> >>> the user interface?
> >> >>>
> >> >>> Also, with your patch, the approach for EXTRA_LDLIBS would be
> >> >>> different than CFLAGS or LDFLAGS:
> >> >>> - CFLAGS/LDFLAGS are in Makefiles only
> >> >>> - EXTRA_CFLAGS/EXTRA_LDFLAGS are prefered in command line
> >> >>>    to add flags to the default ones
> >> >>>
> >> >>> I'm not opposed to add EXTRA_LDLIBS in addition to LDLIBS,
> >> >>> keeping a compatibility with existing application Makefiles.
> >> >>
> >> >> The docs for DPDK 28.3.6 states they can be used for both command
> >>line
> >> >>and
> >> >> Makefile, so I think I like the current solution unless everyone
> >>wants
> >> >>it
> >> >> as you suggested.
> >> >>
> >> >> 
> >> 
> >>>>http://dpdk.readthedocs.org/en/v2.0.0/prog_guide/dev_kit_build_system.h
> >>>>tm
> >> >>l
> >> >
> >> > From the link you have sent:
> >> >
> >> >- About CFLAGS:
> >> >
> >> >"28.3.4. Variables that Can be Set/Overridden in a Makefile Only
> >> >[...]
> >> >CFLAGS: Flags to use for C compilation. The user should use += to
> >>append
> >> >data in this variable."
> >> >
> >> >nothing in 28.3.6
> >> >
> >> >
> >> >- About EXTRA_CFLAGS:
> >> >
> >> >nothing in 28.3.4
> >> >
> >> >"28.3.6. Variables that Can be Set/Overridden by the User in a Makefile
> >> >or Command Line
> >> >[...]
> >> >EXTRA_CFLAGS: The content of this variable is appended after CFLAGS
> >>when
> >> >compiling."
> >> 
> >> The point was that EXTRA_XXX can be used for command line and Makefile
> >>as
> >> it was pointed out in a previous email the assumption was EXTRA_XXX was
> >> only for the command line. (Just to make sure we understood EXTRA_XXX
> >>was
> >> not just for command line options.) This was the reason I sent the link
> >>an
> >> to point out using EXTRA_XXX is a much cleaner method then allowing
> >> someone to modify what I believe is an internal variable.
> >
> >Just beware that setting EXTRA_* flags on the commandline can override
> >their
> >values in the makefiles, and cause unexpected compilation problems.
> >Therefore,
> >it tends to be best to avoid using the EXTRA_* variables for variables
> >essential
> >to compile. For example: putting "-g -O3" in EXTRA_CFLAGS is ok, as the
> >if the
> >useroverrides those with something else things should still work, but
> >putting
> >"-I/path/to/include" would not be.
> 
> On the command line and makefile you should be using += and not just = or
> you run into this problem.

Using "+=" on the commandline is not normal. It's also rather tricky to do
at times, if a value is already defined, as bash shell does not add in whitespace
to the existing variable appropriately. For example, I have EXTRA_CFLAGS set to
'-g -Wfatal-errors' in my .bashrc so I always get debug builds that stop on first
error. Building the dpdk.org as below works fine:

 EXTRA_CFLAGS=-g gmake
 == Build lib
 == Build lib/librte_compat
 ...

However, using += causes very strange behaviour:

$ EXTRA_CFLAGS+=-g gmake
== Build lib
== Build lib/librte_compat
== Build lib/librte_eal
== Build lib/librte_net
== Build lib/librte_eal/common
== Build lib/librte_eal/bsdapp
== Build lib/librte_eal/bsdapp/eal
== Build lib/librte_eal/bsdapp/contigmem
== Build lib/librte_eal/bsdapp/nic_uio
Warning: Object directory not changed from original /usr/home/bruce/dpdk.org/x86_64-native-bsdapp-clang/build/lib/librte_eal/bsdapp/nic_uio
Warning: Object directory not changed from original /usr/home/bruce/dpdk.org/x86_64-native-bsdapp-clang/build/lib/librte_eal/bsdapp/contigmem
  CC eal.o
  CC eal_memory.o
  CC eal_hugepage_info.o
  CC eal_thread.o
error: unknown warning option '-Wfatal-errors=g' [-Werror,-Wunknown-warning-option]

So, in short, you must assume that EXTRA_CFLAGS will be specified using "=" on
the commandline, and that things build appropriately when the user does so.
[Given you use "+=" internally in the makefiles, I assume this is the case, but
flagging this as something worth double-checking]

/Bruce