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! 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
