Update the configure script for OCaml 5.00#10871
Conversation
1d45ae3 to
aae04cc
Compare
|
Xavier Leroy (2022/01/11 01:46 -0800):
This PR is a collection of updates to configure.ac that reflect the current status of OCaml 5.00.
See the individual commits for a detailed description.
Feel free to suggest other things that need updating in configure.
Cc: @shindere @dra27
You can view, comment on, or merge this pull request online at:
#10871
-- Commit Summary --
* configure: update the --disable-systhreads option
* configure: reject --enable-flambda
* configure: more careful configuration for the native-code compiler
* configure: update the list of configurations that support native code
-- File Changes --
M configure (267)
M configure.ac (235)
M tools/ci/inria/other-configs/script (1)
-- Patch Links --
https://github.com/ocaml/ocaml/pull/10871.patch
https://github.com/ocaml/ocaml/pull/10871.diff
ery nice work, many thanks!
About the message
```
POSIX threads are required but not supported on this platform
```
I wonder whether we should say somehting like `POSIX threads are
necessary to build the runtime`, to make it clear that it's not that
they have been requested by the user or so. What do you think?
|
|
About the flambda change:
- Shouldn't the `--enable-flambda-invariants` be treated the same way?
At the moment the configure script accepts it.
You may want to disable the flambda check in Inria's other-configs job,
that is, in `tools/ci/inria/other-configs/script`.
Lastly, do you think it's worth docuemnting in the helpstirng of the
configure options that they are not supported?
|
|
Is there an issue somewhere tracking the status of flambda support ? I'm a bit disappointed that we seem to be considering a release without flambda support, and if the only issues are mysterious error messages I could probably find some time to investigate before the release. |
I would hazard the guess that this is mostly a matter of contributors/maintainers with flambda knowledge chiming in and looking at what needs to be fixed, if any. There is ample time left before a release, but we need people volunteering to do the work. My (non-expert) understanding is the same as yours: some new code plays badly with flambda's mutability inference (which is already known to not be fully compatible with GADTs for example), and it's a matter of massaging it, or finding out how to tune the flambda analysis. But there should be nothing fundamentally incompatible between multicore and flambda. |
|
I created an issue to track Flambda support in trunk: #10878. @xavierleroy unless I'm missing something, I see an issue with the present PR for people who wish to contribute better support for flambda, or for currently not-supported architectures. How are they supposed to configure the compiler to experiment with the currently-broken support? If (Naturally I would expect the options to still exist and put me in a half-broken state, printing a warning that the configuration is currently not support for users. Or maybe it could fail for release builds, and just print the warning for development builds?) (I guess that the PR's secret motive was to motivate people to work on better support; well done :-) |
|
No, I think the motivation for the PR was to make CI skip those
architectures or configurations where OCaml is not supported yet.
|
|
The purpose of the present PR is to avoid reports such as #10869 from unsuspecting users who try to build the trunk with unsupported configurations and end up with mysterious errors that they report as bugs. If something is known to be broken, and is planned to stay broken for the next release, it should not be configurable.
I think this was the plan for the "minimal viable product", i.e. release 5.00. But if flambda is made to work before the release, it's even better.
After configure, edit Makefile.config by hand or by script, replacing Or: uncomment the flambda-related code in configure.ac (the one that this PR comments out), make configure, configure --enable-flambda. |
The old configure script accepts and ignores
We hope these options to be back in 5.01 or 5.02. Again I'm shooting for minimal changes. |
|
Xavier Leroy (2022/01/11 09:11 -0800):
After configure, edit Makefile.config by hand or by script, replacing `FLAMBDA=false` by `FLAMBDA=true`.
Or: uncomment the flambda-related code in configure.ac (the one that
this PR comments out), make configure, configure --enable-flambda.
As suggested privately, perhaps we could have a distinction between
developent and release builds.
In developent builds we can let people configure what they like so that
they can work on restauring the new architectures of flambda or whatever
and in release mode yes, we can refuse to configure on those platforms
that are known not to be supported.
|
Users also build from trunk, see #10869 for an example. |
|
Xavier Leroy (2022/01/11 09:37 -0800):
> As suggested privately, perhaps we could have a distinction between development and release builds.
Users also build from trunk, see #10869 for an example.
I would perhaps not be too hard to check an environmnet variable to see
whether unsafe configs can be allowed, like
`OCAML_ALLOW_UNSAFE_CONFIGS`, or something like that.
|
|
If users build from the development version, right after a major change that was largely advertised, they shouldn't expect a polished experience. When the code works, we release :-) I'm not sure it's a good deal to make life harder for people wishing to hack on the compiler, right when we would benefit from having people look at what's broken and consider helping fix it. |
|
Gabriel Scherer (2022/01/11 12:23 -0800):
If users build from the development version, right after a major
change that was largely advertised, they shouldn't expect a polished
experience. When the code works, we release :-) I'm not sure it's a
good deal to make life harder for people wishing to hack on the
compiler, right when we would benefit from having people look at
what's broken and consider helping fix it.
Yeah, I tend to agree. It's easy to have CI ignore some configurations
without having to change anything to the code outside of the CI scripts.
|
98efa22 to
bc0b3d6
Compare
|
I reverted the removal of the --with-flambda option now that Flambda is back. However, I still think the 3 remaining commits in this PR are useful, and necessary to un-break Inria's CI. |
|
Sorry for the naive question, but out of curiosity: why do we need to change the configure script to un-break the CI? I would naively think that the CI script has several configuration that it tests, and that we could disable/comment the invocations that are currently not working, or explicitly change their configuration to bytecode-only for example. (I hadn't realized at all that this PR was related to the CI before it was mentioned by @shindere earlier.) |
Because a number of configurations previously tested now break for silly reasons. E.g. one of them configures with |
gasche
left a comment
There was a problem hiding this comment.
I don't really understand, but I'm glad to have confirmation that I'm too dumb to work on the CI and leave it to other people. In the meantime, here is an approval that the configure.ac changes look, ahem, not actively terrible.
configure.ac
Outdated
| # [x86_64-*-cygwin*], | ||
| # [arch=amd64; system=cygwin], | ||
| # [riscv64-*-linux*], | ||
| # [arch=riscv; model=riscv64; system=linux] |
There was a problem hiding this comment.
I think it would be nice to keep these comments positioned in such a way that, if I want to hack on a backend for one of those systems, I can just uncomment the two lines and start running.
There was a problem hiding this comment.
These are shell comments, not m4 comments, so there's a risk of messing with the m4 macros.
There was a problem hiding this comment.
I agree that it would be nice to keep the diff quieter - the "appropriate" syntax in this case is to use dnl which is m4's comment indicator. I double-checked this in dra27@43020f7. By putting mingw-w64 last, it does mean that deleting the dnl MVP: is sufficient to re-add any case, as @gasche requests.
| AC_MSG_NOTICE([the threads library is disabled])], | ||
| [systhread_support=true | ||
| otherlibraries="$otherlibraries systhreads" | ||
| AC_MSG_NOTICE([the threads library is supported])]) |
There was a problem hiding this comment.
If I understand correctly, previously we were trying to guess whether systhreads can be enabled (by checking whether (1) Windows or (2) both pthreads and sigwait are available), and currently you ask for an explicit setting without auto-detection logic? (Or do you know assume yes unless unix is known to not be supported?)
(It's a bit sad to see the auto-detection logic go away, but I guess the people that will work on Windows support or whatever will know to just look at the 4.14 configure script to take inspiration from.)
There was a problem hiding this comment.
POSIX threads or the Win32 equivalent are now required by the runtime system (how many times will I have to write this?), so systhreads can always be built, and it's the default. It is not built only if the user configured with --disable-systhreads.
|
If it's just a question of CI, at least as far as Inria's CI is
concerned, I don't think it's true that a modification of the configure
script is required.
I am not saying it is not useful for anything, though, it may have other
uses. But as far as CI is concerned no modification of the configure
script is requred in my opinion and I will submit code soon to
workaroudn the Incria CI issues.
|
dra27
left a comment
There was a problem hiding this comment.
Completely agree with the first two commits (one code "suggestion" to appease check-typo). I agree with @gasche that it would be nice both to keep the diff quieter on the platform list and also be able to uncomment cases easily - possible additional commit to squash into the last one if you agree.
configure.ac
Outdated
| AC_CHECK_FUNC([sigwait], [AC_DEFINE([HAS_SIGWAIT])]) | ||
| LIBS="$saved_LIBS" | ||
| CFLAGS="$saved_CFLAGS"], | ||
| [AC_MSG_ERROR([POSIX threads are required but not supported on this platform])])]) |
There was a problem hiding this comment.
To please check-typo (the generated configure file is unaffected by this change):
| [AC_MSG_ERROR([POSIX threads are required but not supported on this platform])])]) | |
| [AC_MSG_ERROR(m4_normalize([POSIX threads are required but not supported on | |
| this platform]))])]) |
There was a problem hiding this comment.
Thanks for the hint! (m4_normalize, indeed.) Merged and squashed with the associated commit.
configure.ac
Outdated
| # [x86_64-*-cygwin*], | ||
| # [arch=amd64; system=cygwin], | ||
| # [riscv64-*-linux*], | ||
| # [arch=riscv; model=riscv64; system=linux] |
There was a problem hiding this comment.
I agree that it would be nice to keep the diff quieter - the "appropriate" syntax in this case is to use dnl which is m4's comment indicator. I double-checked this in dra27@43020f7. By putting mingw-w64 last, it does mean that deleting the dnl MVP: is sufficient to re-add any case, as @gasche requests.
bc0b3d6 to
d2fa191
Compare
The core runtime system needs to be linked with POSIX threads. Hence, we can disable the systhreads library if we really want to, but this should not break the core runtime system. This commit separates - detecting POSIX threads: mandatory for the system to build - activating the systhreads library: on by default, controlled by --disable-systhreads and --enable-systhreads like before. It also reverts "Do not `disable-systhreads` for the minimal build" (commit ae85a4a) since this should now work again.
If the native-code compiler is not supported for the given platform, do not define "native_compiler", resulting in a bytecode-only build. If, in addition, `--enable-native-compiler` was selected, fail at configure-time instead of continuing with a bytecode-only build.
For 5.00 it's only x86_64 on Linux, macOS, BSD, and Mingw, IIUC.
d2fa191 to
c40b14b
Compare
|
I switched to I didn't do anything special to have a non-commented case as the last case. The generated configure script seems OK even with an extra empty default case at the end. |
|
Note to @shindere: the CI job I'm specifically trying to unbreak here is not the "main" job but the "other-configs" job, which currently fails at the first config it tries because |
|
Note to everyone: there may be other configurations that are not yet supported by 5.00 and should ideally not be configurable. For example: bytecode-only on a 32-bit platform. |
That definitely wants to be fixed before release - for now I opened #10916 to track it in the 5.00.0 milestone. |
|
Despite a bit of grumpiness about this PR, I'll take @dra27's approval and my own gut feeling that it is a (small) improvement as excuses to merge it now. |
|
And of course: stay tuned for more updates to the configure file as we figure out which features we can support in release 5.00 and which features need to be temporarily deactivated. |
|
Xavier Leroy (2022/01/18 14:32 +0000):
Note to @shindere: the CI job I'm specifically trying to unbreak here
is not the "main" job but the "other-configs" job, which currently
fails at the first config it tries because `--disable-systhreads` is
not correctly handled.
Okay, sorry for having missed that point.
Shall we try to make the different tests independent of each other?
I guess this could be achieved either by creating one specific job per
configuration to be tried or, perhaps more lightly, by ignoring the
failures of the invocations of main in general.
|
This PR is a collection of updates to configure.ac that reflect the current status of OCaml 5.00.
See the individual commits for a detailed description.
Feel free to suggest other things that need updating in configure.
Cc: @shindere @dra27