Kelly O'Hair wrote: > > Seems ok to me, just a few observations: > > Do we need this: > > ifeq ($(DEBUG_CLASSFILES), true) > ANT_OPTIONS += -Djavac.debug=true > ANT_OPTIONS += -Djavac.debuglevel=source,lines,vars > endif > > to be > > ifeq ($(DEBUG_CLASSFILES), true) > ANT_OPTIONS += -Djavac.debug=true > ANT_OPTIONS += -Djavac.debuglevel=source,lines,vars > else > ANT_OPTIONS += -Djavac.debug=false > ANT_OPTIONS += -Djavac.debuglevel= > endif
I think we probably do, yes. I was trying to follow the principle of touching as little as possible with my first patch, but it seems to me that $(DEBUG_CLASSFILES) is the appropriate make variable to control this. > Not sure what happens in ant if a property is not set but used???. > > On this: > > # DEBUG_BINARIES overrides everything > ifeq ($(DEBUG_BINARIES), true) > CFLAGS_REQUIRED += -g > DEBUG_FLAG = -g > endif > > I think the idea with isolating the -g to a variable was to allow for > needing to globally set it to -gstabs or -g1 or some custom debug form for > everyone. e.g. being able to do a top level 'make DEBUG_BINARIES=true > DEBUG_FLAG=-g1'. Thanks, I hadn't considered this possibility. > So how about: > > # DEBUG_BINARIES overrides everything, use full -g debug information > ifeq ($(DEBUG_BINARIES), true) > DEBUG_FLAG = -g > CFLAGS_REQUIRED += $(DEBUG_FLAG) > endif OK. > Otherwise it seems fine. > > If you have patch files for each repository (hg diff > patch) I can > run them through our test build system JPRT for you, and send you > the results. Right, will do. Thanks, Andrew.
