Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Towards autoconf #1 #2044

Merged
merged 4 commits into from Sep 17, 2018

Conversation

@shindere
Copy link
Contributor

commented Sep 13, 2018

Initially, I intended to submit the switch to autoconf as only one GPR.
However, it turns out that the resulting PR would be quite big and
not very easy to review. Moreover, since this development changes a lot
of files, I find myself spending a lot of time just rebasing my current
work as other GPRs get merged, which makes me quite slow andis, in addition,
very error-prone.

So in an attempt to simplify everybody's life, here is a first bunch of
pre-autoconf changes. This GPR is probably easier to review in a
commit-by-commit way. It contains the 4 following commits:

  1. Minor config.h cleanup

  2. Introduction of a new C preprocessor macro, HAS_ARCH_CODE32. This is
    to prepare m.h and s.h to be generated from .in files.
    Indeed, autoconf prescribes that template header files must not
    contain #ifdef directives, so this commits avoids this by moving the
    conditional directive to config.h.

  3. Move config/Makefile to Makefile.config, so that the config
    directory can ultimately be deleted. This has already been discussed on
    caml-devel so I hope it will be okay.

  4. Use ROOTDIR more consistently accross Makefiles. This has led to a few
    minor enhancements, in particular in the debugger's Makefile.

Cc @dra27 @xavierleroy @damiendoligez

@damiendoligez
Copy link
Member

left a comment

A few small remarks but otherwise good to go.

command_line.cmo \
main.cmo
DIRECTORIES=$(UNIXDIR) $(addprefix $(ROOTDIR)/,\
utils parsing typing bytecomp toplevel driver)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Sep 17, 2018

Member

Note: you have changed the order: $(UNIXDIR) was at the end of the list, now it's at the front. I don't think it matters, but you might want to restore the original order just to be sure.

@@ -224,7 +226,7 @@ distclean: clean

# Generated non-object files

ld.conf: ../config/Makefile
ld.conf: $(ROOTDIR)//Makefile.config

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Sep 17, 2018

Member

There is a spurious double / here.

@@ -277,8 +279,8 @@ caml/jumptbl.h : caml/instruct.h
sed -n -e '/^ /s/ \([A-Z]\)/ \&\&lbl_\1/gp' \
-e '/^}/q' > $@

caml/version.h : ../VERSION ../tools/make-version-header.sh
../tools/make-version-header.sh ../VERSION > caml/version.h
caml/version.h : $(ROOTDIR)//tools/make-version-header.sh $(ROOTDIR)/VERSION

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Sep 17, 2018

Member

double / here too

@@ -17,7 +17,7 @@

# FIXME reduce redundancy by including ../Makefile

include ../../config/Makefile
include ../../Makefile.config

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Sep 17, 2018

Member

Why not do the ROOTDIR transformation in this file too?

shindere added some commits Jun 11, 2018

config.h cleanup
Get rid of a few lines that are now useless
Change the way ARCH_CODE32 is defined
Before this commit, this C preprocessor macro was defined in
byterun/caml/m.h by the configure script, but just on some architectures
and only in non-PIC mode.

This commit introduces the HAS_ARCH_CODE32 predicate which is inserted
in the m.h file when this is relevant, the ifdef block on PIC
being moved to config.h.

This is to prepare the switch to autoconf, since header files processed
by config.status are not allowed to contain ifdef blocks.
Move config/Makefile to Makefile.config
In order to prepare the transition to autoconf, this commit moves the
configuration Makefile out of the config directory which will disappear
and gives it the name it will have once intstalled, namely Makefile.config.
@dra27

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2018

Can I strongly urge that the renaming of config/Makefile to Makefile.config goes in after 4.08 has been branched? Or, alternatively, that we merge it now but revert that commit on 4.08 immediately before beta1 is cut?

@shindere shindere force-pushed the shindere:towards-autoconf-1 branch from 8161d1d to 01b65ac Sep 17, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

@shindere shindere merged commit 01b65ac into ocaml:trunk Sep 17, 2018

0 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@shindere shindere deleted the shindere:towards-autoconf-1 branch Sep 17, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2018

@damiendoligez looks to have gone through the good tidy-ups in detail, so I haven't repeated that - this is all looks great to me, just with the caveat already mentioned that I'd prefer not to have yet another set of Windows instructions for 4.08 - unless the plan is for autoconf in 4.08?!

@dra27

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2018

@shindere - crossed messages! If the 4.08 branching is holding off until all the autoconf stuff is in, then great!

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.