Skip to content

Conversation

@nojb
Copy link
Collaborator

@nojb nojb commented Nov 21, 2025

This PR follows the discussion in #8645 (comment) and in many other places.

It makes so that starting in version 3.21 of the Dune language, Dune no longer changes the warning set of the compiler. Instead, a variable of "recommended" warnings %{dune-warnings} is made available for interested users (see test).

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
let+ scope = scope in
let dune_version = Dune_project.dune_version (Scope.project scope) in
let profile = Context.profile context in
Value.L.strings (Ocaml_flags.dune_warnings ~dune_version ~profile)))
Copy link
Member

Choose a reason for hiding this comment

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

I think this value should not be dependant on the profile. The users can already insert this under dev in the env stanza.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Fixed in 68caf05

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@Alizter
Copy link
Collaborator

Alizter commented Nov 21, 2025

Needs a changes entry and an update to the documentation.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Nov 21, 2025

Needs a changes entry and an update to the documentation.

Changes entry done. Which documentation should I look at?

@Alizter
Copy link
Collaborator

Alizter commented Nov 21, 2025

Here are a few places I could find:

  • faq.rst has some discussion about warnings.
  • variables.rst could use an update together with introduced version
  • overview.rst has mention of "strict" warnings that might need updating

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Nov 21, 2025

Here are a few places I could find:

Thanks for the pointers. I updated what I could. The doc about the new variable renders as thus:

image

@nojb
Copy link
Collaborator Author

nojb commented Nov 21, 2025

Some of the NIX tests need investigating (maybe the failures are normal and the tests just need updating), but I don't have a NIX environment at hand...

@Alizter
Copy link
Collaborator

Alizter commented Nov 21, 2025

You should be able to run the tests without nix using the opam setup. The test failures should mostly be the same.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Nov 22, 2025

You should be able to run the tests without nix using the opam setup. The test failures should mostly be the same.

Thanks. The test failures revealed a small bug, which is fixed in 9ac4ca7

@@ -0,0 +1,4 @@
- Starting with version 3.21 of the Dune language, Dune no longer changes the
Copy link
Member

Choose a reason for hiding this comment

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

Since dune itself is already using 3.21, you should use %{dune-warnings} in our root dune file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Fixed in 5c4861b

nojb added 2 commits November 23, 2025 11:51
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@maiste
Copy link
Collaborator

maiste commented Nov 23, 2025

I'm not involved actively in the dune release management process anymore but I would advice to delay this for after the 3.21 release. There is a bigger delay than usual for the release. From my experience, it means there will already be a lot of risk of breaking packages. I would keep this for a 3.22 version, which would contain this specific change. It would allow the dune team to make sure it doesn't break any package, give time to user to adapt (by announcing it in advance), and provide the functionalities from the 3.21 without waiting.

If it may help with this change and make it smoother 🤗

Thanks @nojb for tackling this subject 😁

@Alizter
Copy link
Collaborator

Alizter commented Nov 24, 2025

Since this doesn't affect the release profile, I am less worried about breaking packages. If it does end up being problematic for whatever reason we can always revert, however I think that is unlikely.

nojb added 2 commits November 24, 2025 11:16
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Nov 24, 2025

@rgrinberg Are you happy with the present state of the PR?

@Alizter Alizter requested a review from rgrinberg November 24, 2025 11:05
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