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

melange: the compiler does not support -bin-annot-occurrences #10622

Closed
wants to merge 1 commit into from

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Jun 7, 2024

The Melange compiler does not accept the -bin-annot-occurrences flag.

The test relying on OCaml's version is therefore incorrect when building with melange compiler.
This PR adds a special case to prevent use of this flag when building with melange.

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@emillon
Copy link
Collaborator

emillon commented Jun 7, 2024

cc @anmonteiro - do we want this, or fix this on the melange side?
(@gridbugs we might need to backport this to 3.16 if we go with the patch route)

@anmonteiro
Copy link
Collaborator

We’ll have to fix it in both sides I suppose. Where can I read more about this flag?

let fn = Option.value_exn (Obj_dir.Module.cmt_file obj_dir m ~cm_kind ~ml_kind) in
let annots =
if Version.supports_bin_annot_occurrences ocaml.version
then [ "-bin-annot"; "-bin-annot-occurrences" ]
Copy link
Member

Choose a reason for hiding this comment

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

spurious change? the previous version did not duplicate the flag name here

let annots =
[ "-bin-annot" ]
@
if Version.supports_bin_annot_occurrences ocaml.version
Copy link
Member

Choose a reason for hiding this comment

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

How about just maintaining the old code but introducing:

let supports_bin_annot_occurences = Version.supports_bin_annot_occurences ocaml.version && not cm_kind <> Melange in

@anmonteiro
Copy link
Collaborator

actually, since melange supports 5.2, this is just a matter of supporting the flag.

I bet it'd be nice to have occurrences support in Melange projects, too, so let me release a Melange version with support for this flag.

@anmonteiro
Copy link
Collaborator

released melange 4.0.1-52 with this change: ocaml/opam-repository#26043

@voodoos
Copy link
Collaborator Author

voodoos commented Jun 10, 2024

Thanks @anmonteiro

I guess that means this PR is not useful anymore ?

@anmonteiro
Copy link
Collaborator

I’d vote to close it

@voodoos voodoos closed this Jun 10, 2024
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 14, 2024
When dune 3.16 uses ocaml 5.2 (or melange acting like a 5.2 compiler), it will pass `-bin-annot-occurences`, but `melange.4.0.0-52` doesn't support it (4.0.1-52 does support it). See melange-re/melange#1132 and ocaml/dune#10622.
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
When dune 3.16 uses ocaml 5.2 (or melange acting like a 5.2 compiler), it will pass `-bin-annot-occurences`, but `melange.4.0.0-52` doesn't support it (4.0.1-52 does support it). See melange-re/melange#1132 and ocaml/dune#10622.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants