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.

Sign up for GitHub

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

Jump to bottom

Update to Dune 3, fix new warnings #406

Merged
merged 3 commits into from
Nov 14, 2022
Merged

Update to Dune 3, fix new warnings #406

merged 3 commits into from
Nov 14, 2022

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Nov 9, 2022

No description provided.

@Leonidas-from-XIV Leonidas-from-XIV added the no changelog This PR doesn't require a changelog update label Nov 9, 2022
@Leonidas-from-XIV Leonidas-from-XIV self-requested a review November 10, 2022 08:46
@Leonidas-from-XIV
Copy link
Collaborator

I am not sure how this happens because the values indeed don't seem to be read, but the tests fail, can you take a look what is going on there?

@MisterDA
Copy link
Contributor Author

The failures are due to Dune 3.0. This PR categorizes the cram tests as "misbehaving" ;)
ocaml/dune#4981
Apparently you shouldn't write syntactically incorrect cram tests or unreachable code in cram tests.

@Leonidas-from-XIV
Copy link
Collaborator

But the tests aren't supposed to be run by dune cram, these are ocaml-mdx test cram tests (which support things like ... etc), so dune shouldn't execute them. Pretty sure that even before Dune 3.0 they weren't valid Dune Cram tests.

According to the docs, these are automatically enabled in Dune 3.0, so I think they have to be explicitly turned off in this project otherwise dune will attempt to run the MDX cram tests through its own Cram implementation.

@Leonidas-from-XIV Leonidas-from-XIV merged commit dd31995 into realworldocaml:main Nov 14, 2022
@Leonidas-from-XIV
Copy link
Collaborator

Great, thanks!

@MisterDA MisterDA deleted the dune-3 branch November 14, 2022 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Leonidas-from-XIV Leonidas-from-XIV Awaiting requested review from Leonidas-from-XIV

Assignees
No one assigned
Labels
no changelog This PR doesn't require a changelog update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MisterDA @Leonidas-from-XIV