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

Delete the vmthreads library #2289

Merged
merged 3 commits into from Mar 11, 2019

Conversation

@diml
Copy link
Member

commented Mar 5, 2019

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

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

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?
@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

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

configure Outdated
if test "${enable_vmthreads+set}" = set; then :
enableval=$enable_vmthreads;
fi

This comment has been minimized.

Copy link
@avsm

avsm Mar 10, 2019

Member

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

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

diml added 3 commits Mar 5, 2019
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
It is no longer necessary

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@diml diml force-pushed the diml:delete-vmthreads branch from a226ff1 to a87bd9d Mar 11, 2019
@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

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

Copy link
Contributor

left a comment

I reviewed the diff and it looks good to me.

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

@xavierleroy xavierleroy merged commit 705054b into ocaml:trunk Mar 11, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

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

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@nojb has a fix in #2310.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

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

@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Got it, thanks.

dra27 added a commit to dra27/ocaml that referenced this pull request Mar 12, 2019
xavierleroy added a commit that referenced this pull request Mar 13, 2019
This GPR restores -vmthread with an adapted version of the deprecation message as an error message and also keeps the use_vmthreads part of ppx contexts.

* Partially revert #2289
* Convert -vmthread to an error
* Neuter use_vmthreads in ppx context
* Remove Clflags.use_vmthreads
@gerdstolpmann

This comment has been minimized.

Copy link

commented Mar 18, 2019

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.

@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

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

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