From: Maynard J. <may...@us...> - 2012-07-09 14:03:43
|
On 07/06/2012 02:26 PM, Gergely Kis wrote: > Maynard, > > On Fri, Jul 6, 2012 at 4:12 PM, Maynard Johnson <may...@us... <mailto:may...@us...>> wrote: > > Gergely, > This patch was made against the master branch. It will apply with fuzz > against the perf-events branch, but then the pe_profiling/Makefile.am > file would need to be changed to use the @OP_CPPFLAGS@ and @OP_LDFLAGS@ > similar to the other Makefile.am files altered by this patch (for example, > see daemon/Makefile.am). > > Can you please give it a test? Thanks. > > Yes, we tested it, and it works fine, after the necessary additions. > > There is one question, whether we should introduce a separate OP_KERNEL_CPPFLAGS so the kernel headers are only included where it is actually used. I am not sure if it is necessary. > > We found 2 typos in the patch, see below: That's generous of you to refer to them as "typos". ;-) Thanks for the review! Since this patch had such a ripple effect on many Makefile.am files, I'm hoping to get at least one more review (from Will or anyone else in the community) before committing it. -Maynard > > +# Since we should not permanently alter user environment variables, we'll > > +# save the contents of the original flags in case the user has set them > +# prior to running this configue script. > +CPPFLAGS_SAVE="$CPP_FLAGS" > > Here I suppose you meant to write $CPPFLAGS. > > > +LDFLAGS_SAVE="$LDFLAGS" > AC_ARG_WITH(binutils, > [ --with-binutils=dir Path to binutils installation to use], BINUTILSDIR=$withval) > > if test "$BINUTILSDIR" != ""; then > LD="$BINUTILSDIR/ld" > - if test "$CFLAGS" = ""; then > - CFLAGS="-g -O2" > - fi > - if test "$CXXFLAGS" = ""; then > - CXXFLAGS="-g -O2" > - fi > - CFLAGS="$CFLAGS -I$BINUTILSDIR/include" > - CXXFLAGS="$CXXFLAGS -I$BINUTILSDIR/include" > + CPPFLAGS="$CFLAGS -I$BINUTILSDIR/include" > > Here also, I suppose you wanted to write $CPPFLAGS. > > Best Regards, > Gergely |