Delete the vmthreads library

[ghost]

This PR deletes the vmthreads library that was deprecated in 4.08.

[xavierleroy]

You're not wasting any time :-)

I'm favorable in principle, but would like to know:

• are there any plans to make a separate package out of this library? (knowing that it depends a lot on internals of the bytecode runtime system)?
• is there something to be changed in ocamlfind? if yes, did you submit a matching pull request for ocamlfind?

[ghost]

Indeed :)

are there any plans to make a separate package out of this library? (knowing that it depends a lot on internals of the bytecode runtime system)?

Nope. It does seems like a non-negligible amount of work, and given that we don't know if anyone is still using vmthreads, I'd much prefer to not do this work eagerly. If someone comes forward in the future and announce that they were relying on vmthreads, we can always discuss what can be done to help at this point.

is there something to be changed in ocamlfind? if yes, did you submit a matching pull request for ocamlfind?

No, vmthreads is exposed as the threads.vm ocamlfind library and this library will automatically disappear when we stop installing the file <stdlib-dir>/vmthreads/threads.cma.

[xavierleroy]

Thanks for the explanations. I'm in favor of merging this PR.

[avsm]

I'm obviously in favour of merging this, but should we make --enable-vmthreads be a configure-time error if the option is passed? That'll help package maintainers clean up their configure flags if there's an actual error at build time for a release.

[shindere]

Anil Madhavapeddy (2019/03/10 14:32 -0700):

avsm commented on this pull request. > @@ -3017,12 +3015,6 @@ else fi -# Check whether --enable-vmthreads was given. -if test "${enable_vmthreads+set}" = set; then : - enableval=$enable_vmthreads; -fi - I'm obviously in favour of merging this, but should we make `--enable-vmthreads` be a configure-time error if the option is passed? That'll help package maintainers clean up their configure flags if there's an actual error at build time for a release.

That sounds good to me, too. Just one thing, @avsm: beware, you used generated code in your comment, which may be misleading. It's probably better to comment on the diffs in `configure.ac`.

[ghost]

Agreed. I updated configure.ac to error out when passing --{enable,disable}-vmthreads.

[xavierleroy]

I reviewed the diff and it looks good to me.

Removing code gives me much pleasure, so I'm happy to merge right now!

[gasche]

The INRIA CI appears to be broken by this change, with errors in partialclean and alldepend. I'm trying to reproduce the failure locally to see if there is an obvious fix -- there probably is.

[gasche]

I can reproduce locally: OTHERLIBRARIES still contains threads and for example make -C otherlibs partialclean tries to access otherlibs/threads. This looks like a stale ./configure issue, but I don't know how to fix it. (I tried autoconf naively, but I'm not sure how this works.)

[gasche]

(to reproduce I had to git clean -xfd first to start from a completely fresh clone -- removes everything not committed.)

[gasche]

@nojb has a fix in #2310.

[shindere]

The bootstrap job on Inria's CI also needed to be updated, see
2ac104c

[nojb]

Got it, thanks.

[gerdstolpmann]

Turns out findlib needs a patch to support this. Created https://gitlab.camlcity.org/gerd/lib-findlib/merge_requests/22 - any comment welcome. I'll merge in the next days.

[ghost]

Ah sorry about this, I only looked at the META file but I didn't try to actually build findlib.