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.

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

Thanks,
Jose

> 
> > +
> > +HS_SRCS = $(HS_LIBTESTBUILT_SRCS)
> >
> > -$(HS_ALL_PROGS): %: %.hs $(HS_LIBTESTBUILT_SRCS) Makefile
> > +.NOTPARALLEL: $(HS_ALL_PROGS)
> > +.PHONY: $(HS_ALL_PROGS)
> > +$(HS_ALL_PROGS): %: %.hs $(HS_SRCS) Makefile
> >         @if [ "$(notdir $@)" = "test" ] && [ "$(HS_NODEV)" ]; then \
> >           echo "Error: cannot run unittests without the development" \
> >                " libraries (see devnotes.rst)" 1>&2; \
> >           exit 1; \
> >         fi
> >         @rm -f $(notdir $@).tix
> > -       $(GHC) --make \
> > -         $(HFLAGS) \
> > -         $(HS_PARALLEL3) $(HS_REGEX_PCRE) \
> > -         -osuf $(notdir $@).o -hisuf $(notdir $@).hi \
> > -         $(HEXTRA) $(HEXTRA_INT) $@
> > +       $(GHC) --make $(HFLAGS) \
> > +               -osuf $(HSUFFIX) \
> > +               -hisuf $(patsubst %.o,%.hi,$(HSUFFIX)) \
> > +               $(HS_PARALLEL3) $(HS_REGEX_PCRE) $@
> >         @touch "$@"
> 
> HEXTRA is missing here too.
> 
> >  # for the test/hs/htest binary, we need to enable profiling/coverage
> > -test/hs/htest: HEXTRA_INT=-fhpc -itest/hs
> > +test/hs/htest: HCOVERAGE = true
> > +test/hs/htest: HFLAGS += -itest/hs
> >
> >  # we compile the hpc-htools binary with the program coverage
> > -test/hs/hpc-htools: HEXTRA_INT=-fhpc
> > +test/hs/hpc-htools: HCOVERAGE = true
> >
> >  # we compile the hpc-mon-collector binary with the program coverage
> > -test/hs/hpc-mon-collector: HEXTRA_INT=-fhpc
> > +test/hs/hpc-mon-collector: HCOVERAGE = true
> >
> >  # test dependency
> >  test/hs/offline-test.sh: test/hs/hpc-htools test/hs/hpc-mon-collector
> > --
> > 1.8.4
> >
> 
> Also, I don't see the HTEST and HPROFILE variables ever used. Are they
> meant to be manually set?
> 
> 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

Reply via email to