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

Fix the reftests on OCaml 5.0 #5402

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Conversation

kit-ty-kate
Copy link
Member

With this PR, the reftests are all green on my local machine.

@@ -362,7 +364,7 @@ let do_upgrade repo_root =
O.with_substs [OpamFilename.Base.of_string conf_script_name] |>
O.with_build [
List.map (fun s -> CString s, None)
[ "ocaml"; "unix.cma"; conf_script_name ],
[ "ocaml"; conf_script_name ],
Copy link
Member

Choose a reason for hiding this comment

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

This really shouldn't matter any more, as this only affects the 1.2 upgrade, but is there an issue I haven't spotted with not doing the change above and instead having:

Suggested change
[ "ocaml"; conf_script_name ],
[ "ocaml"; "-I"; "+unix"; "unix.cma"; conf_script_name ],

(apart from the fact that my proposed version is ugly!)

Being ultra-paranoid, the file generated before is compilable, where with the current change it becomes a toplevel-only script?

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

with david's comment, lgtm

@kit-ty-kate kit-ty-kate merged commit a29280f into ocaml:master Feb 14, 2023
Out of release : doc, test, install, etc. automation moved this from PR to done Feb 14, 2023
@kit-ty-kate kit-ty-kate deleted the 500-tests branch February 14, 2023 12:50
@kit-ty-kate kit-ty-kate added this to PR in progress in Opam 2.2.0 via automation Feb 14, 2023
@kit-ty-kate kit-ty-kate added this to the 2.2.0~alpha milestone Feb 14, 2023
@kit-ty-kate kit-ty-kate moved this from PR in progress to Done in Opam 2.2.0 Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants