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

Deprecate `ocamlc -vmthread` #2038

Merged
merged 4 commits into from Sep 13, 2018

Conversation

Projects
None yet
4 participants
@diml
Copy link
Member

commented Sep 10, 2018

As discussed on caml-devel, this PR deprecates the otherlibs/threads library. It does so by deprecating the -vmthread command line flag of ocamlc.

@diml diml force-pushed the diml:deprecate-threads branch from b108603 to 62f22d8 Sep 10, 2018

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

git grep vmthread will suggest where the manual needs to be updated ;-)

let vmthread_deprecated_message = "\
The -vmthread argument of ocamlc is deprecated\n\
and will soon be abandoned. For VM-level scheduling, consider using a\n\
third-party library such as Lwt. Otherwise, please switch to system\n\

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Sep 10, 2018

Contributor

I don't find the mention of Lwt particularly enlighting here since IIRC vm-thread actually implemented a compatibility layer for the system threads library.

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Sep 10, 2018

Contributor

I think it is useful to mention LWT, but perhaps in position number 2? E.g.

The -vmthread argument of ocamlc is deprecated\n
and will soon be abandoned. Please switch to system threads,\n
which have the same API. Lightweight threads with VM-level scheduling\n
are provided by third-party libraries such as Lwt, but with a different API.

@@ -142,6 +142,12 @@ module Options = Main_args.Make_bytecomp_options (struct
let anonymous = anonymous
end)

let vmthread_deprecated_message = "\
The -vmthread argument of ocamlc is deprecated\n\
and will soon be abandoned. For VM-level scheduling, consider using a\n\

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Sep 10, 2018

Contributor

isn't "soon abandoned" synonym with "deprecated" ?

This comment has been minimized.

Copy link
@pmetzger

pmetzger Sep 10, 2018

Member

It's close enough to a synonym. "removed" would be a better word.

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Sep 10, 2018

Contributor

+1 for "removed".

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Sep 10, 2018

Contributor

What I meant is that one could simply say "VM-threads are deprecated".

This comment has been minimized.

Copy link
@diml

diml Sep 11, 2018

Author Member

I guess the information that is missing is when the feature will indeed be removed. We don't have to specify it, but in general it is useful for users to know this.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

xref MPR7687.

@diml diml force-pushed the diml:deprecate-threads branch from 62f22d8 to 7cad870 Sep 11, 2018

@diml

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

Thanks, I updated the deprecation message and the manual.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Sorry to bother you on such trivial matters but could you please indicate the version where it was deprecated in the manual, release notes are sometimes a bit difficult to skim through or not readily at hand.

Also maybe update the title of the PR it's a bit misleading.

@diml diml changed the title Deprecate otherlibs/threads Deprecate `ocamlc -vmthread` Sep 11, 2018

@diml

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

Sure, no problem. I added the version number in the error message as well since it seems useful to have it as soon as we get an error.

@diml

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

Is this good to merge then?

@xavierleroy
Copy link
Contributor

left a comment

Looks good to me and ready for merging. Thanks!

@@ -707,7 +707,7 @@ Useful to debug C library problems.

\comp{%
\item["-vmthread"]
Compile or link multithreaded programs, in combination with the
Deprecated. Compile or link multithreaded programs, in combination with the

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Sep 12, 2018

Contributor

Still missing the version number here.

This comment has been minimized.

Copy link
@diml

diml Sep 13, 2018

Author Member

Indeed, I added it

@dbuenzli
Copy link
Contributor

left a comment

The rest is ok.

@diml diml merged commit ee3730f into ocaml:trunk Sep 13, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
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.