Skip to content

Update to ocaml 4.11.1 - #885

Closed
SteveBronder wants to merge 6 commits into
stan-dev:masterfrom
SteveBronder:update-4.11.1-attempt2
Closed

Update to ocaml 4.11.1 #885
SteveBronder wants to merge 6 commits into
stan-dev:masterfrom
SteveBronder:update-4.11.1-attempt2

Conversation

@SteveBronder

Copy link
Copy Markdown
Contributor

This updates the setup scripts and code to work with ocaml 4.11.1. This PR just has the changes necessary to 4.11.1 but has not been formatted so Jenkins will fail. It does in fact have several differences in the MIR and generated C++ that we should investigate before doing all the formatting stuff as that will make some things harder to sort out.

Most of the code changes are very small, things like using let ( = ) = Stdlib.( = ) as ocaml by default in these versions no longer allows for polymorphic equality comparison operators by default. Should we just do away with any code that does =, <>, etc. and just have equals a b, not_equals a b etc. functions for each type? Or is the above safe? tbh I just don't understand the implications of performing the above import vs. writing our own.

All the other changes are imported from #843 except for the run_*_on_args.ml files in the tests which use Core's unix and channels stuff.

The changes to the MIR and the C++ are related to the optimizer. My guess is that something in the optimizer is hitting something weird during the lazy code motion phase that deals with comparison operators

Release notes

Updates Ocaml version to 4.11.1 along with dependencies

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@SteveBronder

Copy link
Copy Markdown
Contributor Author

Tagging @mandel I think this has all the correct changes, though I'm kind of worried about this comparison operator thing. Some of the changes to the MIR are fine, but others seem kind of sus and make me thing we should either. @seantalts why were the location statements opaque before? Do we need things to be opaque or were those just to shrinks the size of the printed out mir?

  1. Go through every file doing all the imports, then just go one by one deleting imports from StdLib until it compiles.
  2. Do our own comparison operators from each type.

@SteveBronder

Copy link
Copy Markdown
Contributor Author

Also I have not tested this on windows if anyone is able to

@rok-cesnovar

Copy link
Copy Markdown
Member

Also I have not tested this on windows if anyone is able to

if the Windows build step on Jenkins passes that is all we need. We dont support development on native Windows (not sure if that is even doable). We only need to be able to cross compile for Windows on Linux.

@mandel

mandel commented Apr 19, 2021

Copy link
Copy Markdown
Contributor

@SteveBronder do you want my help on this?

@SteveBronder

SteveBronder commented Apr 19, 2021

Copy link
Copy Markdown
Contributor Author

@mandel Yes that would be awesome! Right now this is giving a weird error for the mock that menhir generates

~/stan/stevebronder/stanc3$ make
dune build src/stanc/stanc.exe
      menhir src/frontend/parsing_errors.ml
Read 501 sample input sentences and 357 error messages.
File "src/frontend/parser__mock.ml.mock", line 4127, characters 9-18:
4127 |   (raise Not_found : 'tv_vardecl_or_statement * 'tv_var_decl * 'tv_unsized_type * 'tv_unsized_dims * 'tv_type_constraint * 'tv_truncation * 'tv_transformed_parameters_block * 'tv_transformed_data_block * 'tv_top_vardecl_or_statement * 'tv_top_var_type * 'tv_top_var_decl_no_assign * 'tv_top_var_decl * 'tv_string_literal * 'tv_statement * 'tv_sized_basic_type * 'tv_separated_nonempty_list_COMMA_id_and_optional_assignment_no_assign__ * 'tv_separated_nonempty_list_COMMA_id_and_optional_assignment_expression__ * 'tv_separated_nonempty_list_COMMA_expression_ * 'tv_separated_nonempty_list_COMMA_arg_decl_ * 'tv_separated_list_COMMA_expression_ * 'tv_separated_list_COMMA_arg_decl_ * 'tv_return_type * 'tv_range_constraint * 'tv_range * 'tv_program * 'tv_printables * 'tv_prefixOp * 'tv_postfixOp * 'tv_parameters_block * 'tv_pair_COMMA_expression_ * 'tv_pair_ASSIGN_no_assign_ * 'tv_pair_ASSIGN_expression_ * 'tv_optional_assignment_no_assign_ * 'tv_optional_assignment_expression_ * 'tv_option_unsized_dims_ * 'tv_option_truncation_ * 'tv_option_transformed_parameters_block_ * 'tv_option_transformed_data_block_ * 'tv_option_parameters_block_ * 'tv_option_pair_COMMA_expression__ * 'tv_option_pair_ASSIGN_no_assign__ * 'tv_option_pair_ASSIGN_expression__ * 'tv_option_model_block_ * 'tv_option_generated_quantities_block_ * 'tv_option_function_block_ * 'tv_option_expression_ * 'tv_option_data_block_ * 'tv_option_DATABLOCK_ * 'tv_offset_mult * 'tv_non_lhs * 'tv_no_assign * 'tv_nested_statement * 'tv_model_block * 'tv_loption_separated_nonempty_list_COMMA_expression__ * 'tv_loption_separated_nonempty_list_COMMA_arg_decl__ * 'tv_logicalBinOp * 'tv_list_vardecl_or_statement_ * 'tv_list_top_vardecl_or_statement_ * 'tv_list_top_var_decl_no_assign_ * 'tv_list_function_def_ * 'tv_list_COMMA_ * 'tv_lhs * 'tv_ioption_lhs_ * 'tv_infixOp * 'tv_indexes * 'tv_identifier * 'tv_id_and_optional_assignment_no_assign_ * 'tv_id_and_optional_assignment_expression_ * 'tv_generated_quantities_block * 'tv_function_def * 'tv_function_block * 'tv_expression * 'tv_dims * 'tv_decl_identifier * 'tv_decl_top_var_type_no_assign_ * 'tv_decl_top_var_type_expression_ * 'tv_decl_sized_basic_type_expression_ * 'tv_data_block * 'tv_constr_expression * 'tv_common_expression * 'tv_basic_type * 'tv_atomic_statement * 'tv_assignment_op * 'tv_arithmeticBinOp * 'tv_arg_decl * 'tv_always_unsized_dims_)
                ^^^^^^^^^
Error (alert deprecated): Not_found
[since 2018-02] Instead of raising [Not_found], consider using [raise_s] with an
informative error message.  If code needs to distinguish [Not_found] from other
exceptions, please change it to handle both [Not_found] and [Not_found_s].  Then, instead
of raising [Not_found], raise [Not_found_s] with an informative error message.
make: *** [Makefile:2: all] Error 1

I wrapped them with a

(env
 (dev
  (flags (:standard -warn-error -3-9))))

which works as duct-tape but not ignoring that would be a much nicer option

@mandel

mandel commented Apr 20, 2021

Copy link
Copy Markdown
Contributor

My understanding is that there is no good solution for now for this error. See ocaml/dune#2450

@SteveBronder

Copy link
Copy Markdown
Contributor Author

It looks there's something weird going on with lsp server that only works with dune on version 4.12 for the type stuff in the editors.

ocamllabs/vscode-ocaml-platform#532

Unless lsp server downgrades to 4.11.1 or the windows cross compiler updates to 4.12 we might be in a bit of a limbo :-(

@WardBrian

WardBrian commented Oct 26, 2021

Copy link
Copy Markdown
Member

Limbo should be ending soon - ocaml-cross/opam-cross-windows#206

We should also identify if there are any dependencies we would like to update and I can try to get those updated in the opam-cross-windows world too.

I think at the very least we want core_kernel updated, though this is what leads to = and <> not working as before. There are some really good reasons for that, but in the meantime we can simply add open Core_kernel.Poly (this is better than let ( = ) = Stdlib.( = ) if we're inside the Core ecosystem anyway) to any files we need it in

See this discussion for more: https://discuss.ocaml.org/t/removing-polymorphic-compare-from-core/2994/10

@rok-cesnovar

Copy link
Copy Markdown
Member

Awesome!

@WardBrian

Copy link
Copy Markdown
Member

We will also need:

  • core_kernel v0.14.2 (a lot of dependencies for this need to be updated as well)
  • ppx_deriving 5.2.1
  • menhir 20211012 (technically optional, but using a version of menhir that is 3 years out of date probably isn't a good idea)

Fmt and Yojson are fine at their current versions

@WardBrian WardBrian mentioned this pull request Nov 1, 2021
11 tasks
@WardBrian WardBrian added cleanup Code simplification or clean-up release/packaging labels Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Code simplification or clean-up release/packaging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants