Skip to content

Conversation

@erikmd
Copy link
Contributor

@erikmd erikmd commented Feb 20, 2020

learn-ocaml 0.12 has just been released by @yurug, and this PR adds the 2 related opam packages (server and CLI client).

@camelus
Copy link
Contributor

camelus commented Feb 20, 2020

Commit: 9a31c92

@erikmd has posted 2 contributions.

☀️ All lint checks passed 9a31c92
  • These packages passed lint tests: learn-ocaml-client.0.12, learn-ocaml.0.12

☀️ Installability check (+2)
  • new installable packages (2): learn-ocaml.0.12 learn-ocaml-client.0.12

@kit-ty-kate
Copy link
Member

Is there a reason why these packages have such tight constraints? Using = <version> is not really good practice as there might be future bug fixes and it's annoying for people trying to use your package on other versions of OCaml (currently they cannot be installed with OCaml >= 4.07). Also this prevent other packages to be installed in the same switch.

Apart from that, dune should not be tagged as a build dependency (see #14266). Could you remove this? Also, could you add "-j" jobs to the dune build command. By default dune runs in parallel but users might want to reduce the number of jobs used.

Three "=" constraints need to be kept for 0.12 though:

* "ocaml" {= "4.05.0"}
* "decompress" {= "0.8.1"}
* "ocplib-json-typed" {= "0.6"}
@erikmd
Copy link
Contributor Author

erikmd commented Feb 21, 2020

@kit-ty-kate , thanks a lot for your your feedback.

Cc @yurug FYI

dune should not be tagged as a build dependency (see #14266). Could you remove this? Also, could you add "-j" jobs to the dune build command.

done, thanks.

Is there a reason why these packages have such tight constraints?

Partly yes (see below) but indeed most of = <version> constraints seemed unnecessarily tight.
I've pushed a new relaxed version that locally compile on my machine and I'm also going to submit these changes upstream with a PR for learn-ocaml master.

(currently they cannot be installed with OCaml >= 4.07)

AFAICT, learn-ocaml is currently tied with ocaml 4.05.0, notably because it incorporates some features that rely on OCaml_405.Ast (and the learn-ocaml CI test-build only runs with ocaml 4.05 anyway cf. Dockerfile and .ci-macosx.sh); maybe other ocaml versions could be supported later, but not for the current 0.12 release anyway…

So in the current version of this PR, apart from ocaml, only two "=" dependencies remain:

"decompress" {= "0.8.1"}
"ocplib-json-typed" {= "0.6"}

(because of some non-backward compatibility with their next version…)

Kind regards

@erikmd
Copy link
Contributor Author

erikmd commented Feb 22, 2020

Hi @kit-ty-kate , do you believe you'll have the time to take another look at this PR before Monday 2019-02-24? (as we're going to use learn-ocaml-client in a lab session on Monday morning…)

@kit-ty-kate
Copy link
Member

I'm just waiting for the CI to finish and I'll have a final look by tonight

@kit-ty-kate
Copy link
Member

Looks good. Thanks!

@kit-ty-kate kit-ty-kate merged commit 501077c into ocaml:master Feb 22, 2020
@erikmd erikmd deleted the add_learn-ocaml.0.12 branch February 22, 2020 17:13
erikmd added a commit to pfitaxel/learn-ocaml that referenced this pull request May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants