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

Use autoconf to generate the compiler's configuration script #2139

Merged
merged 2 commits into from Dec 21, 2018

Conversation

@shindere
Copy link
Contributor

commented Nov 7, 2018

Cc @pgraverdy

Introduction

This GPR continues the work started in GPRs #2044, #2059, #2113 and #2115. At a higher level: after having merged the Unix and Windows build systems, this PR merges their configuration systems.

To do so, the handwritten configuration script is replaced by an autoconf-generated one, which is also expected to make the future work on producing cross-compilers easier. It becomes possible to configure the Windows builds similarly to what can be done under Unix and without having to tweak the makefiles. (This GPR does start to update README files, but the update should be completed in a future GPR.)

Command-line options to the configure script

They change completely because autoconf imposes restrictions on their form. The new options are however documented in ./configure --help, which was not the case previously.

Still, for people packaging the OCaml compiler, it may be useful to provide a mapping between the old and new option syntax. I would be willing to do that if it is deemed necessary, but I am not sure where it would best fit. It could be done in a distinct GPR that could even be merged after the feature freeze.

libtool

Basically, autoconf by itself does not provide any means of figuring out anything related to libraries (where the linker is, which options to use to build shared libraries, etc.). To do that, one is expected to rely on libtool, which provides its own set of macros that should be invoked in the configuration script. These macros generate a libtool script that one is supposed to use during the build to compile and link files, rather than calling the compiler and linker directly.

The approach implemented in this GPR does use the libtool configuration-time machinery to figure things out but at the same time does not call libtool to compile and link files. This approach has been chosen first because introducing libtool in the build system seemed a too big change, second because on Windows some libtool guesses were wrong and it was also not clear how well libtool would interact with flexlink.

(Now that we have libtool, nothing prevents us from relying on it more in the future and, should we want to do so, it will be possible to do the transition in an incremental way.)

An important consequence of all this is that some of the guesses shown by configure (such as the linker location) will actually not be used. This is because, once the libtool-related detection macros have been invoked, some of its results are ignored and replaced by the ones that were previously hardcoded.

This may look awkward and, to some extent, it certainly is, but at the same time it seemed that this strategy makes the transition to autoconf smooth by avoiding to modify the rest of the build system as much as possible.

Regressions

This GPR introduces two regressions, to be fixed before the 4.08 final release:

  1. The former -target-bind-dir option has not been implemented yet;
  2. The unwind library is not yet detected.

Due to time constraints, it seemed better to submit the code in its current state to give people more time to test and discuss. Adding the support for these two features can be done in parallel with the discussion.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

Bravo @shindere! Review and tests at my end to follow

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

Note to reviewers: all the files in the build-aux directory come from either autoconf or libtool -- it is a standard practice to bundle them. Of course, configure is generated. So all these files do normally not need to be reviewed.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

@shindere shindere force-pushed the shindere:autoconf branch from 6a69623 to 16dc931 Nov 7, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

@gasche: one Travis-related question, so I am assuming that's for you.

Why is flambda enabled in .travis.yml, whereas flambda-invariants is
enabled in the shell script? Do you know / remember?

@damiendoligez
Copy link
Member

left a comment

A few remarks from having a quick look.

aclocal.m4 Outdated Show resolved Hide resolved
README.win32.adoc Show resolved Hide resolved
config/Makefile.mingw Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
[arch=amd64; system=openbsd],
[x86_64-*-darwin*],
[arch=amd64; system=macosx],
[x86_64-*-mingw32],

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Nov 7, 2018

Member

Same question again with mingw32/mingw*
It seems more important here, since you're turning mingw32 into mingw64.

[power,gcc*,elf,ppc64], [PARTIALLD="ld -r -m elf64ppc"],
[power,gcc*,elf,ppc64le], [PARTIALLD="ld -r -m elf64lppc"],
[*,cl,*,*], [PARTIALLD="link -lib -nologo"],
[PARTIALLD="ld -r"])])

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Nov 7, 2018

Member

You removed the command-line option to override partialld?

tools/ci/travis/travis-ci.sh Show resolved Hide resolved
tools/ci/travis/travis-ci.sh Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@xclerc xclerc referenced this pull request Nov 8, 2018

@shindere shindere force-pushed the shindere:autoconf branch 2 times, most recently from c78abb1 to 9eae477 Nov 13, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

@shindere shindere force-pushed the shindere:autoconf branch from 9eae477 to 712f021 Nov 23, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

@shindere

Well actually, it's the whole config/ subdirectory that would become
obsolete but I didn't dare to remove it right now.

Sorry, my question was not clear. Why do you patch config/Makefile.mingw if it's now obsolete?

@shindere shindere force-pushed the shindere:autoconf branch from 712f021 to 710c507 Nov 23, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

yes that makes sense

@shindere shindere force-pushed the shindere:autoconf branch from 710c507 to 963413a Nov 30, 2018

@dra27 dra27 added this to the 4.08 milestone Nov 30, 2018

@shindere shindere force-pushed the shindere:autoconf branch 2 times, most recently from e7f90a1 to 7f5df20 Dec 4, 2018

@dra27
Copy link
Contributor

left a comment

I've gone through the mapping of options to the old configure script and where they appear in the new one. Reading the various guides for autoconf, it seems to me that AC_ARG_WITH is intended to be used for external dependency related things only and so should only relate to the libunwind and afl options. That seems a more important rule to regard than the nonsense in the autoconf manual about AC_ARG_ENABLE not altering any features (since they don't provide any other kind of parameter for things like flambda, which is a feature which alters other features).

The old configure made no use of environment variables, so there are a few arguments which really should change to AC_ARG_VAR now, especially when they relate to passing directories in. Users have been used to saying ./configure -cc cc so hopefully will switch to the correct ./configure CC=cc and not incorrect CC=cc ./configure.

  • The old -lib* parameter has no equivalent in the new one - should $LIBS be being put into $cclibs?
  • This is more of a todo for me, but a reminder - a patch of mine suggested changing PARTIALLD to PACKLD in configure. This is a mistake which I'll change - configure should accept PARTIALLD but Makefile.config should only contain PACKLD. (this is fixed)
  • The -dldefs parameter has no equivalent - as with LIBS, shouldn't CFLAGS be read at some point?
  • --with-cplugins should be added, solely to maintain the error message.

Every single argument has changed, apart from --prefix, --bindir, --libdir, --mandir, --host, --target and --verbose. In doing this review, I now have a document which maps the old options to what should be their new equivalents. I think we should either insert a one-time script in front of configure.ac which translates the parameters (and which would never be updated with any new ones, obviously) or update the documentation with this mapping. My preference is for the script, which I'm happy to write, since it maintains as smooth a path as possible.

I don't think --disable-shared is working properly, but it's awkward testing both this and #2160 simultaneously, so I'll come back to that after this GPR is merged!


AC_ARG_ENABLE([pthread],
[AS_HELP_STRING([--disable-pthread],
[disable the POSIX thread library])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

I think this should be named systhreads, since the Windows implementation has nothing to do with POSIX:

AC_ARG_ENABLE([systhreads-lib], 
  [AS_HELP_STRING([--disable-systhreads-lib], 
    [disable the native threads (POSIX/Win32) library])])

AC_ARG_WITH([libunwind-include-dir],
[AS_HELP_STRING([--with-libunwind-include-dir],
[location of header files for libunwind])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

The unwind stuff seems to have several problems - there are large parts of the old configure (especially with macOS options) which don't appear to have been translated - is that intentional? Most importantly, the default seems to have changed - libunwind is meant to be required unless it's explicitly disabled? I know there was a discussion on caml-devel, but I wonder if the proper interface for this should be --with-libunwind with LIBUNWIND_PREFIX (default ""), LIBUNWIND_INCLUDE (default is either "" or $LIBUNWIND_PREFIX/include if LIBUNWIND_PREFIX is defined) and LIBUNWIND_LIB (same system for the default)


AC_ARG_ENABLE([call-counts],
[AS_HELP_STRING([--disable-call-counts],
[disable the call counts in spacetime])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

enable-call-counts only makes sense with --enable-spacetime. I therefore wonder if it would be better to have --spacetime=mode where mode is with-call-counts (the default) or without-call-counts

[install *.cmt* and *.mli files])])
AC_ARG_ENABLE([installing-bytecode-programs],
[AS_HELP_STRING([--enable-installing-bytecode-programs],
[also install the bytecode versions of programs])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

As a matter of personal taste, I find these options unusually named. Can I propose --enable-source-install and --enable-bytecode-install - it's the installing which seems very strange.


AC_ARG_ENABLE([native-compiler],
[AS_HELP_STRING([--disable-native-compiler],
[do not build the native compiler])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

Given again that it's changing name, can we consider --enable-ocamlopt?


AC_ARG_ENABLE([flambda-invariants],
[AS_HELP_STRING([--enable-flambda-invariants],
[enable invariants checks in flambda])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

As with --enable-spacetime, this is related to its parent option, so would --flambda=with-invariants be more logical?


AC_ARG_WITH([target-bindir],
[AS_HELP_STRING([--with-target-bindir],
[location of binary programs on target system])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

This parameter isn't propagated anywhere - it should be in Makefile.config.in. However, given how it's used, it should really be AC_ARG_VAR([TARGET_BINDIR], ...)


AC_ARG_WITH([reserved-header-bits],
[AS_HELP_STRING([--with-reserved-header-bits=BITS],
[reserve BITS bits in block headers for profiling info])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

This option hasn't quite preserved the semantics of the old one, although I'm not sure how relevant it is. Specifying --with-reserved-header-bits is also supposed to enable profinfo (even without spacetime). I wonder if it would be better to have AC_ARG_ENABLE([profinfo], ...) and have this as its parameter?

AC_ARG_WITH([default-string],
[AS_HELP_STRING([--with-default-string=mode],
[whether strings should be safe or unsafe by default
@<:@default=safe@:>@])])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

This was a problem in the previous configure script, so perhaps now is the time to fix this properly. --enable-force-safe-string --with-default-string=unsafe is a conflicting configuration (I think it builds a compiler which displays an error message by default, but I haven't checked). I think this should one option --enable-safe-string=mode where mode is no (equivalent to --disable-force-safe-string --with-default-string=unsafe), yes/default (equivalent to --disable-force-safe-string --with-default-string=safe) or always (equivalent to --enable-force-safe-string --with-default-string=safe)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Dec 18, 2018

Member

In trunk, -force-safe-string takes precedence. (see lines 167-170 of utils/clflags.ml)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Dec 18, 2018

Member

I'd use unsafe / safe / force instead of no / yes / always.

This comment has been minimized.

Copy link
@gasche

gasche Apr 15, 2019

Member

There is a conceptual difference between:

  • --force-safe-string, which must be a configure-time option that changes the runtime definitions and has a global impact on all modules compiled and linked by the resulting compiler
  • --default-string={safe,unsafe} which is "merely" a convenience feature to set a default for compile-time options, rather than a configure-time option by nature (afl is the same, etc.). It does nothing more than change the default for a choice that the user makes on a per-invocation basis.

This comment has been minimized.

Copy link
@gasche

gasche Apr 15, 2019

Member

In my opinion this conceptual difference justifies not showing the three options as a single knob to fix at configure-time, but two separate kind of options. There is a dependency between them (--force-safe-string makes it incorrect to change the per-invocation value, so it makes --default-string useless and meaningless), but that is common across the configuration space, for example --disable-native-compiler will make many other settings meaningless.

[AS_HELP_STRING([--with-dllibs=LIBS],
[use LIBS (in addition to dl) to load dynamic libs @<:@default=""@:>@])],
[otherdllibs=$withval],
[otherdllibs=""])

This comment has been minimized.

Copy link
@dra27

dra27 Dec 9, 2018

Contributor

This should be AC_ARG_VAR([DLLIBS], ...)

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

Meanwhile: did you read the text written to introduce this PR? I think
it may respond to a few of your comments, e.g. regarding libunwind

Ah - my bad! My suggestion for -target-bindir as TARGET_BINDIR should deal with that one completely - possibly with a minor change needed to stdlib/Makefile which I don't think should define it. I think with my suggestion for --with-libunwind and three environment variables that the code can be pretty much transplanted from the old configure script - happy to help.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

@shindere - I have. The question from caml-devel is whether macOS should ignore those settings when passed, but it still looks as though it should be possible to pass the include and lib directories separately - as it stands, there aren't enough options corresponding to the previously available (and used) ones.

I'm completely happy for the logic to be improved after autoconf is in, but the interface needs to be correct before it goes in - i.e. this PR must determine the final names of parameters and environment variables. That's what my review's attempted to address, hopefully without meandering to other things!

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2018

@shindere shindere force-pushed the shindere:autoconf branch from 1d6f270 to 4048ec2 Dec 13, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

@shindere - please pull your GitHub branch before next force pushing... your last rebase blew away 1d6f270 (a .gitattributes update)... I've pushed it back!

@dra27

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

@shindere - I just pushed e742127. Although the OCaml Travis job hasn't started yet, your local fork appears to be working with that shim. The default for ASPP with autoconf is picking up $toolpref which isn't strictly correct - I think we should either be reusing CC or redoing the test with AC_CHECK_TOOL (this would look for i386-pc-linux-gnu-gcc and on not finding it fall back to gcc). However, I think that can wait...

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

@ksromanov

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

Well that line comes from config/auto-aux/cckind.c, see line 28. You may be right but (1) it's hard to say without being able to test with the xlc compiler

I checked on AIX/xlc - both versions #elif defined(__xlc__) && (__xlC__) and #elif defined(__xlc__) && defined(__xlC__) work. Of course, the second looks better, so please correct in both places if you want.

@shindere shindere force-pushed the shindere:autoconf branch from 46dcca6 to dc42e77 Dec 21, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Just rebased on latest trunk and squashed all the commits into the main one.
Let me thank @dra27 again for his careful review and all his help.

This is currently going through precheck and hopefully through our other CI
solutions.

@shindere shindere force-pushed the shindere:autoconf branch 2 times, most recently from 10e654e to 8a78d20 Dec 21, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@shindere shindere force-pushed the shindere:autoconf branch from 8a78d20 to e72e09c Dec 21, 2018

@dra27

dra27 approved these changes Dec 21, 2018

Copy link
Contributor

left a comment

Inria CI is a qualified (the failures which happened are either unrelated or now tested separately).

I propose merging once Travis/AppVeyor re-pass this!

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Spectrum Is Green! Here goes...

@dra27 dra27 merged commit cc3f70b into ocaml:trunk Dec 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@shindere shindere deleted the shindere:autoconf branch Dec 21, 2018

@ksromanov

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

The latest version of the GPR includes a fix, see aclocal.m4:52. What do you mean by both places?

In cckind.c - https://github.com/ocaml/ocaml/blob/trunk/config/auto-aux/cckind.c#L28
But it is a cosmetic change.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@dra27, sorry for having such a bad memory: was it you who added

AC_CHECK_TOOL([DIRECT_CPP],[cpp])
AS_CASE([$host],
  [*-pc-windows], [CPP="$CC -nologo -EP"],
  [CPP=$DIRECT_CPP])

to configure.ac?

I am asking because this breaks the OCAML_CC_VENDOR macro, e.g.
even if configure is called with CC=clang, say, then the macro will call
gcc's preprocessor and then the C compiler vendor will be detected in a
wrong way. And same goes for xlc, for instance.

Any idea how this should be fixed?

@ksromanov

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

The breaking part is default choice of AC_CASE. Perhaps you can remove this snippet at all and add

      CPP="$CC -nologo -EP";

to the snippet couple lines above - https://github.com/ocaml/ocaml/blob/trunk/configure.ac#L395
Of course, comment about fcc -E is very important, though I could not find, which tool is fcc (it is impossible to google it out). My guess - something from Visual Studio.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@shindere - no, the correction I was made was to remove the hardcoding of CPP=cpp to CPP=$DIRECT_CPP, since that was breaking the mingw ports if Cygwin's gcc wasn't also installed (they need to use i686-w64-mingw32-cpp or x86_64-w64-mingw32-cpp, which is why I added the AC_CHECK_TOOL call)

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

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