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

configure: build the debug and instrumented runtimes by default #1526

Merged
merged 2 commits into from Dec 15, 2017

Conversation

Projects
None yet
4 participants
@gasche
Copy link
Member

gasche commented Dec 11, 2017

For a user that discovers a use for either of those, not having built
by default makes the experience pretty bad: they have to recompile
their own OCaml compiler, possibly from a source checkout if the
option is not available on opam...

With this change the total compilation time only increased by
7 seconds on my machine, from 3m24s to 3m33s -- these are sequential
builds, with parallel builds the difference is lost in the noise.

build the debug and instrumented runtimes by default
For a user that discovers a use for either of those, not having built
by default makes the experience pretty bad: they have to recompile
their own OCaml compiler, possibly from a source checkout if the
option is not available on opam...

With this change the total compilation time only increased by
7 seconds on my machine, from 3m24s to 3m33s -- these are sequential
builds, with parallel builds the difference is lost in the noise.
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Dec 12, 2017

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Dec 12, 2017

@gasche in addition to my previous comment, look for "CAML_INSTR" in
byterun/misc.c, around line 200.

Once we have autoconf I think the right approach will be to automatically
detect whether the system is able to support this and adjust whether the
instrumented runtime is compiled accordingly.

@gasche gasche changed the title build the debug and instrumented runtimes by default configure: build the debug and instrumented runtimes by default Dec 12, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Dec 12, 2017

I propose to restrict the scope of this PR to changes in systems affected by the ./configure script. I don't have a Windows machine to test with, so I would rather let someone else write a Windows-specific PR if they wish to.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Dec 12, 2017

It's a small tweak to config/Makefile.m* to build the debug runtimes on Windows, and Inria CI and AppVeyor can verify them... they were working last time I tested them

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Dec 12, 2017

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Dec 12, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Dec 12, 2017

I don't want to work on Windows in this PR. I don't even know the file extension for libcamlrun there to mention in the Changes entry. Again, feel free to send a pull request.

I have to deal with the issue of whether setting the instrumented runtime by default on non-linux systems is correct. Maybe the simplest approach is to only enable it by default on Linux systems.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 12, 2017

I have to deal with the issue of whether setting the instrumented runtime by default on non-linux systems is correct. Maybe the simplest approach is to only enable it by default on Linux systems.

It's not a Linux / other-OS issue, it's a clock_gettime is available / is not available issue. If you read the fine configure script, you'll see that configuring with instrumented runtime will fail if clock_gettime can't be found. You could change the configure logic so that if clock_gettime is not found, no error is generated and with_instrumented_runtime is set to false. But it would be nice to get confirmation from whomever wrote this test initially.

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Dec 12, 2017

(Whoever is probably @damiendoligez.)

Thanks, I'll have a look.

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Dec 12, 2017

In ae8e487, missing clock_gettime is an error only if -with-instrumented-runtime was passed explicitly by the user. In the default setting, if it is missing then the instrumented runtime is disabled without failure.

configure: missing clock_gettime is a soft error for -with-instrument…
…ed-runtime

We wish to make -with-instrumented-runtime the default, but not break
the build on systems that do not support it -- when clock_gettime is
missing. This is done by tracking whether -with-instrumented-runtime
was set explicitly or is the default value; in the latter case,
a missing clock_gettime defaults to no instrumented runtime, without
raising an error at configure-time.
@xavierleroy
Copy link
Contributor

xavierleroy left a comment

LGTM.

@xavierleroy xavierleroy merged commit dc3086f into ocaml:trunk Dec 15, 2017

2 checks passed

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

gasche referenced this pull request May 25, 2018

Inria CI main job fix
This commit stops enabling the instrumented and debug runtime at
configure time. Given that they are now enabled by default this is
no longer necessary.
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.