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