On Wed, Nov 6, 2013 at 11:16 AM, Jose A. Lopes <[email protected]> wrote: > Interdiff > > diff --git a/Makefile.am b/Makefile.am > index def2fdb..c52e7fb 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -1010,6 +1010,7 @@ src/hs2py-constants: $(HS2PY_CONSTANTS_SRCS) \ > $(GHC) --make $(HFLAGS) \ > -osuf $(HSUFFIX) \ > -hisuf $(patsubst %.o,%.hi,$(HSUFFIX)) \ > + $(HEXTRA) \ > src/hs2py-constants.hs > > HS_SRCS = $(HS_LIBTESTBUILT_SRCS) > @@ -1026,7 +1027,7 @@ $(HS_ALL_PROGS): %: %.hs $(HS_SRCS) Makefile > $(GHC) --make $(HFLAGS) \ > -osuf $(HSUFFIX) \ > -hisuf $(patsubst %.o,%.hi,$(HSUFFIX)) \ > - $(HS_PARALLEL3) $(HS_REGEX_PCRE) $@ > + $(HS_PARALLEL3) $(HS_REGEX_PCRE) $(HEXTRA) $@ > @touch "$@" > > # for the test/hs/htest binary, we need to enable profiling/coverage > @@ -1042,27 +1043,14 @@ test/hs/hpc-mon-collector: HCOVERAGE = true > # test dependency > test/hs/offline-test.sh: test/hs/hpc-htools test/hs/hpc-mon-collector > > -# rules for building profiling-enabled versions of the haskell > -# programs: hs-prof does the full two-step build, whereas > -# hs-prof-quick does only the final rebuild (hs-prof must have been > -# run before) > -.PHONY: hs-prof hs-prof-quick > +# rule for building profiling-enabled versions of the haskell programs > +.PHONY: hs-prof > hs-prof: > @if [ -z "$(TARGET)" ]; then \ > echo "You need to define TARGET when running this rule" 1>&2; \ > exit 1; \ > fi > - $(MAKE) $(AM_MAKEFLAGS) clean > - $(MAKE) $(AM_MAKEFLAGS) $(TARGET) HEXTRA="-osuf o" > - rm -f $(HS_ALL_PROGS) > - $(MAKE) $(AM_MAKEFLAGS) hs-prof-quick > - > -hs-prof-quick: > - @if [ -z "$(TARGET)" ]; then \ > - echo "You need to define TARGET when running this rule" 1>&2; \ > - exit 1; \ > - fi > - $(MAKE) $(AM_MAKEFLAGS) $(TARGET) HEXTRA="-osuf prof_o -prof > -auto-all" > + $(MAKE) $(AM_MAKEFLAGS) HPROFILE=y $(TARGET) > > On Tue, Nov 05, 2013 at 02:33:06PM +0100, Michele Tartara wrote: >> On Mon, Nov 4, 2013 at 7:58 PM, Jose A. Lopes <[email protected]> wrote: >> > On Mon, Nov 04, 2013 at 05:00:26PM +0100, Michele Tartara wrote: >> >> On Mon, Nov 4, 2013 at 4:53 PM, Jose A. Lopes <[email protected]> >> >> wrote: >> >> > On Mon, Nov 04, 2013 at 10:58:01AM +0100, Michele Tartara wrote: >> >> >> On Mon, Oct 7, 2013 at 2:53 PM, Jose A. Lopes <[email protected]> >> >> >> wrote: >> >> >> > The idea is to compile (on demand, that is, when necessary) each >> >> >> > source file to a normal object file, a coverage object file, a >> >> >> > profiling object file, and a test object file. Also, a given Haskell >> >> >> > binary is linked with the proper object files. This is achieved with >> >> >> > the following Makefile variables: >> >> >> > >> >> >> > Compilation modes (profiles): >> >> >> > 1. HPROFILE enables/disables profiling >> >> >> > 2. HCOVERAGE enables/disables coverage >> >> >> > 3. HTEST enables/disables 'TEST' preprocessor definition >> >> >> > >> >> >> > A few words on testing: testing means the problem described in issue >> >> >> > 535: https://code.google.com/p/ganeti/issues/detail?id=535. With >> >> >> > HTEST enable, ghc will be instructed to define the preprocessor >> >> >> > definition TEST, for modules that use '#ifdef TEST'. >> >> >> > >> >> >> > Haskell binary targets fetch the proper dependencies. They are also >> >> >> > '.PHONY' targets so that 'make' does not check for the file timestamp >> >> >> > and instead it will always call 'ghc --make ...'. This is not a >> >> >> > problem because 'ghc' with the '--make' flag only compiles the >> >> >> > necessary object files. >> >> >> > >> >> >> > Signed-off-by: Jose A. Lopes <[email protected]> >> >> >> > --- >> >> >> > Makefile.am | 66 >> >> >> > ++++++++++++++++++++++++++++++++++++++++++------------------- >> >> >> > 1 file changed, 46 insertions(+), 20 deletions(-) >> >> >> > >> >> >> > diff --git a/Makefile.am b/Makefile.am >> >> >> > index bd6872e..279448e 100644 >> >> >> > --- a/Makefile.am >> >> >> > +++ b/Makefile.am >> >> >> > @@ -965,45 +965,71 @@ install-exec-hook: >> >> >> > done >> >> >> > endif >> >> >> > >> >> >> > +HNORMAL_SUFFIX = .o >> >> >> > +HPROFILE_SUFFIX = .prof.o >> >> >> > +HCOVERAGE_SUFFIX = .hpc.o >> >> >> > +HTEST_SUFFIX = .test.o >> >> >> > + >> >> >> > +HSUFFIX = $(if $(HPROFILE),$(HPROFILE_SUFFIX), \ >> >> >> > + $(if $(HCOVERAGE),$(HCOVERAGE_SUFFIX), \ >> >> >> > + $(if $(HTEST),$(HTEST_SUFFIX), \ >> >> >> > + $(HNORMAL_SUFFIX)))) >> >> >> > + >> >> >> > +HFLAGS += $(if $(HPROFILE),-prof -auto-all,) >> >> >> > +HFLAGS += $(if $(HCOVERAGE),-fhpc,) >> >> >> > +HFLAGS += $(if $(HTEST),-DTEST,) >> >> >> >> >> >> Are HPROFILE, HCOVERAGE and HTEST always alternative to each other? Or >> >> >> might they be defined together? >> >> > >> >> > Always exclusive. >> >> >> >> Ok. >> >> >> >> > >> >> >> >> >> >> > + >> >> >> > +HS2PY_CONSTANTS_SRCS = src/hs2py-constants.hs \ >> >> >> > + src/AutoConf.hs \ >> >> >> > + src/Ganeti/BasicTypes.hs \ >> >> >> > + src/Ganeti/ConstantUtils.hs \ >> >> >> > + src/Ganeti/JSON.hs \ >> >> >> > + src/Ganeti/THH.hs \ >> >> >> > + src/Ganeti/Hs2Py/GenConstants.hs \ >> >> >> > + src/Ganeti/Hs2Py/ListConstants.hs \ >> >> >> > + src/Ganeti/HsConstants.hs \ >> >> >> > + src/Ganeti/PyValueInstances.hs >> >> >> > + >> >> >> > # This target cannot be merged with the '$(HS_ALL_PROGS)' target >> >> >> > # because 'hs2py-constants' cannot depend on 'Ganeti.Constants'. >> >> >> > And >> >> >> > # the reason for this is because 'hs2py-constants' needs to generate >> >> >> > # Python code, and 'Ganeti.Constants' is generated by Python. >> >> >> > -src/hs2py-constants: src/hs2py-constants.hs src/AutoConf.hs \ >> >> >> > - src/Ganeti/BasicTypes.hs >> >> >> > src/Ganeti/ConstantUtils.hs \ >> >> >> > - src/Ganeti/JSON.hs src/Ganeti/THH.hs \ >> >> >> > - src/Ganeti/Hs2Py/GenConstants.hs \ >> >> >> > - src/Ganeti/Hs2Py/ListConstants.hs \ >> >> >> > - src/Ganeti/HsConstants.hs \ >> >> >> > - src/Ganeti/PyValueInstances.hs \ >> >> >> > +.NOTPARALLEL: src/hs2py-constants >> >> >> > +.PHONY: src/hs2py-constants >> >> >> > +src/hs2py-constants: $(HS2PY_CONSTANTS_SRCS) \ >> >> >> > | stamp-srclinks >> >> >> > - $(GHC) --make \ >> >> >> > - $(HFLAGS) \ >> >> >> > - -osuf $(notdir $@).o -hisuf $(notdir $@).hi \ >> >> >> > - $(HEXTRA) $(HEXTRA_INT) src/hs2py-constants.hs >> >> >> > + @rm -f [email protected] >> >> >> > + $(GHC) --make $(HFLAGS) \ >> >> >> > + -osuf $(HSUFFIX) \ >> >> >> > + -hisuf $(patsubst %.o,%.hi,$(HSUFFIX)) \ >> >> >> > + src/hs2py-constants.hs >> >> >> >> >> >> I think that while rewriting the rule $(HEXTRA) and $(HEXTRA_INT) were >> >> >> unintentionally left out. >> >> > >> >> > HEXTRA_INT was being used for coverage, for example, to pass '-fhpc'. >> >> > Now, we have a HPROFILE mode, therefore, HEXTRA_INT is no longer >> >> > necessary. >> >> > >> >> > HEXTRA is only used by 'hs-prof' and 'hs-prof-quick' which were needed >> >> > for profiling. I guess these can be eliminated because of 'HPROFILE'. >> >> > >> >> > What do you think ? >> >> >> >> Ok, for HEXTRA_INT (which, I guess, means "internal") but NACK for >> >> HEXTRA. It should be anywhere ghc is used. It is meant to be a way to >> >> inject extra compiler parameters from the environment, e.g. as done >> >> here: >> >> http://docs.ganeti.org/ganeti/master/html/devnotes.html#haskell-development-notes >> >> >> >> It's a bit like the CFLAGS variable for Makefiles running gcc. >> >> Please, add it back. >> >> >> >> The rest looks really good! >> > >> > What about removing 'hs-prof' and 'hs-prof-quick' ? >> > Remove ? >> >> This would be a user-facing change, so I'm not convince we should >> remove them (even if only facing the users compiling Ganeti). >> >> I'd replace their current version with one that properly sets the >> HPROFILE variable and then launches the compilation. >> >> Thanks, >> Michele >> -- >> Google Germany GmbH >> Dienerstr. 12 >> 80331 München >> >> Registergericht und -nummer: Hamburg, HRB 86891 >> Sitz der Gesellschaft: Hamburg >> Geschäftsführer: Graham Law, Christine Elizabeth Flores > > -- > Jose Antonio Lopes > Ganeti Engineering > Google Germany GmbH > Dienerstr. 12, 80331, München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Steuernummer: 48/725/00206 > Umsatzsteueridentifikationsnummer: DE813741370
With the interdiff, LGTM, thanks. Michele -- Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
