Skip to content

Delete the vmthreads library#2289

Merged
xavierleroy merged 3 commits intotrunkfrom
unknown repository
Mar 11, 2019
Merged

Delete the vmthreads library#2289
xavierleroy merged 3 commits intotrunkfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Mar 5, 2019

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

@xavierleroy
Copy link
Copy Markdown
Contributor

xavierleroy commented Mar 6, 2019 •
edited
Loading

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
Copy link
Copy Markdown
Author

ghost 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
Copy link
Copy Markdown
Contributor

xavierleroy commented Mar 7, 2019

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

avsm
avsm reviewed Mar 10, 2019
View reviewed changes
configure
if test "${enable_vmthreads+set}" = set; then :
enableval=$enable_vmthreads;
fi

Copy link
Copy Markdown
Member

@avsm avsm Mar 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor

shindere commented Mar 11, 2019 via email

@jeremiedimino
Delete the deprecated vmthreads library
ce9f620 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Remove the byte/native argument of init_path
6b86b3c 
It is no longer necessary

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Error out when passing --{enable,disable}-vmthreads to ./configure
a87bd9d 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 11, 2019

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

xavierleroy
xavierleroy approved these changes Mar 11, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@gasche
Copy link
Copy Markdown
Member

gasche 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
Copy link
Copy Markdown
Member

gasche 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
Copy link
Copy Markdown
Member

gasche commented Mar 11, 2019

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

@nojb nojb mentioned this pull request Mar 11, 2019
Merged
@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 11, 2019

@nojb has a fix in #2310.

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Mar 12, 2019

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

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 12, 2019

Got it, thanks.

@dra27 dra27 mentioned this pull request Mar 12, 2019
Merged
dra27 added a commit to dra27/ocaml that referenced this pull request Mar 12, 2019
@dra27
Partially revert ocaml#2289
98b094f
xavierleroy pushed a commit that referenced this pull request Mar 13, 2019
@dra27 @xavierleroy
Restore -vmthreads flag as an error (#2312)
6e84987 
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
@stedolan stedolan mentioned this pull request Mar 15, 2019
Closed
@gerdstolpmann
Copy link
Copy Markdown

gerdstolpmann 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.

@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 19, 2019

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

@dra27 dra27 mentioned this pull request Mar 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@xavierleroy @shindere @gasche @nojb @gerdstolpmann @avsm @jeremiedimino