-
Notifications
You must be signed in to change notification settings - Fork 454
Stop changing the default warning set of the compiler #12766
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
src/dune_rules/expander.ml
Outdated
| 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))) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
|
Needs a changes entry and an update to the documentation. |
Changes entry done. Which documentation should I look at? |
|
Here are a few places I could find:
|
|
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... |
|
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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
|
I'm not involved actively in the dune release management process anymore but I would advice to delay this for after the If it may help with this change and make it smoother 🤗 Thanks @nojb for tackling this subject 😁 |
|
Since this doesn't affect the |
|
@rgrinberg Are you happy with the present state of the PR? |

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).