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 #2: OCAML_STDLIB_DIR #2059

Merged
merged 1 commit into from Sep 25, 2018

Conversation

Projects
None yet
3 participants
@shindere
Copy link
Contributor

commented Sep 24, 2018

(Cc @dra27, @damiendoligez and @xavierleroy)

So far, OCAML_STDLIB_DIR has been computed at configure time and written
to s.h. However, in the autoconf world, it is recommended that
directories can be changed by running just make without having to
reconfigure. So defining a directory at configure time to a value
that can then not be overwritten by running make is not suitable.
And, in the autoconf world, it's not only not suitable, it is also hard
(if not impossible) to achieve.

So this PR takes OCAML_STDLIB_DIR out of s.h and defines it in
runtime/Makefile. This seems also reasonable in the sense that s.h
is for OS-related settings, to which OCAML_STDLIB_DIR does not really
belong.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

For the record this PR belongs to a series started with #2044.

@dra27

dra27 approved these changes Sep 24, 2018

Copy link
Contributor

left a comment

This nicely fixes a headers inconsistency between Windows and Unix.

However, it’s fixed by “breaking” any Unix build which expected OCAML_STDLIB_DIR to be available by including config.h

Alternatively, could the -D be moved to OCAMLC_CPPFLAGS and OCAMLOPT_CPPFLAGS (or which ever ones get embedded in config.ml)? Then at least it only “breaks” builds which don’t use ocamlopt to compile C stubs (and don’t query ocamlopt -config to get the preprocessor flags)

@dra27

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

I think it should have a CHANGES entry, though - it’s a visible change, however obscure.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Why does config.ml suddenly have to be generated at configure-time just because we switch to autoconf? There's no specific need to do that - I find it slightly strange that switching to autoconf is preventing us from doing things in the build system?

Even if config.ml is generated by configure - isn't this what config.status is for (I know it's intended for where you change the .in file and don't want to re-run the whole of configure - I can't remember if it's also for minor changes like this)

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

Re: the list above: the mentionned rules files use OCAML_STDLIB_DIR
but in this context this refers to a variable defined in
/usr/share/ocaml/ocamlvars.mk, which they all include.

This file is provided by the dh-ocaml package and the variable is
defined there as the result of calling ocamlc -where so this PR shouldn't
affect these files.

@shindere shindere force-pushed the shindere:towards-autoconf-2 branch from 9fd9f06 to cf232c7 Sep 25, 2018

@shindere shindere force-pushed the shindere:towards-autoconf-2 branch from cf232c7 to 51493c4 Sep 25, 2018

@shindere shindere merged commit ccae1e2 into ocaml:trunk Sep 25, 2018

0 of 2 checks passed

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

@shindere shindere deleted the shindere:towards-autoconf-2 branch Sep 25, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@dra27 to respond to your comment
(#2059 (comment)), and as I
started to say, well, it's not that we have to generate the .ml files like
utils/config.ml at configure time. We certainly can continue to generate
them during make. It's just that, at first sight, autoconf's substitution
mechanism seemed nice to me so I started using it, withouth seeing
at first the problems this could yield.

The actual problem with this approach, as I understand it now, has to do
with directories and only directories. Paths like libdir are expanded
only at make time, so that their values is not known at configure
time. So, if one wants to take advantage of autoconf's substitution
mechanism in all other situations, where it does make a lot of sense
and where it really looks nicer, IMO, than all our sed stuff, one
solution would be to introduce a dirs module. That would be the
only module one would have to generate during make and that could be done
just by echoing things. Then the config module could rely on this dirs
module and still be generated by configure so one would get what looks to me
as the best of both worlds.

What do you think? Does this look like an acceptable compromise to you?

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

You could also generate config.mlp from config.mlp.in at configure time, then generate config.ml from config.mlp at build time.

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