-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
[WIP] upgrade to OCaml 5 #199
base: master
Are you sure you want to change the base?
Conversation
Hey @akabe One of the reasons should be following deprecations:
Affected test cases:
Another reason is related to the toplevel change, and it doesn't accept
I've commented out these test cases and need your help on the details to fix these for OCaml 5. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kimmywork Thank you for creating a patch for OCaml 5. OCaml 5 had many disruptive changes and I skipped responding to them. I suggest ideas for solving the issue of Test_evaluation.test__directive
and merlin.
jupyter.opam
Outdated
@@ -17,7 +17,7 @@ build: [ | |||
[ "dune" "build" "-p" name "-j" jobs ] | |||
] | |||
depends: [ | |||
"ocaml" {>= "4.10.0" & < "4.15"} | |||
"ocaml" {(>= "4.10.0" & < "4.15") | (>= "5.0.0")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget updating the restriction of ocaml versions. I'd like to suggest
"ocaml" {>= "4.10.0"}
because ocaml-jupyter kernel now supports 4.15.
cf. jupyter.2.8.2/opam
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
tests/repl/test_evaluation.ml
Outdated
@@ -61,11 +61,11 @@ let test__multiple_phrases ctxt = | |||
assert_equal ~ctxt ~printer:[%show: status] SHELL_OK status ; | |||
assert_equal ~ctxt ~printer:[%show: reply list] expected actual | |||
|
|||
let test__directive ctxt = | |||
(* let test__directive ctxt = | |||
let status, actual = eval "#load \"str.cma\" ;; Str.regexp \".*\"" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case only checks whether a directive works fine, or not. We can check an other directive, not #load
.
In my environment, this patch works.
-(* let test__directive ctxt =
- let status, actual = eval "#load \"str.cma\" ;; Str.regexp \".*\"" in
+let test__directive ctxt =
+ let status, actual = eval "#require \"str\" ;; Str.regexp \".*\"" in
let expected = [iopub_success ~count:0 "- : Str.regexp = <abstr>\n"] in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
tests/completor/test_merlin.ml
Outdated
@@ -163,7 +163,8 @@ let test_complete ctxt = | |||
]; | |||
} in | |||
let actual = complete merlin code ~pos:15 in | |||
require expected actual ~msg:"module" ; | |||
require expected actual ~msg:"module" | |||
(* ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out lines are not important checks. Could you remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
config/ocaml_flags.sexp
Outdated
@@ -1,3 +1,3 @@ | |||
(-w A-4-31-33-34-39-41-42-43-44-45-48-49-50-58-66 | |||
-safe-string -strict-sequence -strict-formats | |||
-safe-string -strict-sequence -strict-formats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you delete a space at the end of line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
The issue of ppx is hard maybe. I have not yet been able to find a solution. In the official OCaml 5, $ ocaml
OCaml version 5.0.0
# #require "ppx_deriving.show";;
/Users/akabe/.opam/5.0.0/lib/result: added to search path
/Users/akabe/.opam/5.0.0/lib/result/result.cma: loaded
/Users/akabe/.opam/5.0.0/lib/ppx_deriving/runtime: added to search path
/Users/akabe/.opam/5.0.0/lib/ppx_deriving/runtime/ppx_deriving_runtime.cma: loaded
/Users/akabe/.opam/5.0.0/lib/ppx_deriving: added to search path
/Users/akabe/.opam/5.0.0/lib/ppx_deriving/./ppx_deriving: activated
/Users/akabe/.opam/5.0.0/lib/ppx_deriving/show: added to search path
ppx_deriving: package:ppx_deriving.show: option added |
CI fails due to code format. Try |
Same on my side. I've fixed the code in your comments above, but just found another issue when running Full output:
|
I see, all |
@kimmywork Thanks for fixing. I am also aware of the same issue. As it is likely a deep-rooted problem, I anticipate that it will take some time to find a solution, but I will continue my investigation as well. I have been a bit busy lately, so please give me some time. |
Has there been any progress on this issue? |
No description provided.