Fix/good error msg pkg enabled #12802

benodiwal · 2025-11-27T13:47:48Z

src/dune_lang/dune_project.ml

Leonidas-from-XIV

Thanks for the PR. I have made some comments on how to improve the code.

However I think I would ask the opinion of other Dune maintainers to ask whether adding support for pkg in dune-project only to state that you don't support is is a good idea. It feels a bit.. odd. Likewise, should we add stanzas for other error cases too (lock_dir etc)?

src/dune_lang/dune_project.ml

test/blackbox-tests/test-cases/workspaces/pkg-in-dune-project.t/dune-project

test/blackbox-tests/test-cases/pkg/pkg-in-dune-project-error.t

src/dune_lang/dune_project.ml

benodiwal · 2025-11-27T14:25:25Z

Thanks for the PR. I have made some comments on how to improve the code.

However I think I would ask the opinion of other Dune maintainers to ask whether adding support for pkg in dune-project only to state that you don't support is is a good idea. It feels a bit.. odd. Likewise, should we add stanzas for other error cases too (lock_dir etc)?

Thanks for the feedback @Leonidas-from-XIV ! I've implemented all your suggestions ! You raise a good point about whether this approach feels odd. I can see the concern, we're essentially adding a field just to reject it with a better error message.

Concerns addressed

Alizter · 2025-11-27T16:11:29Z

@Leonidas-from-XIV Given that we are/will be pushing people to use package management it would be good to handle this particular case since it does occur if users are new to the dune-workspace file. It's better than the confusing message we get otherwise. #12761 (comment)

Leonidas-from-XIV · 2025-11-27T16:11:31Z

Thanks for addressing the issues, quite nice.

I've been looking at this a little and if we decide that we want to support a feature like this maybe we should extend Syntax.check_supported with an additional case for something like `Unsupported_in_dune_project. That way it would be easier to add stanzas that are unsupported without having to write an ad-hoc error message for every single of them.

src/dune_lang/dune_project.ml

noticed something

src/dune_lang/dune_project.ml

Alizter · 2025-11-28T12:04:39Z

test/blackbox-tests/test-cases/pkg/pkg-in-dune-project-error.t

+  File "dune-project", line 2, characters 0-13:
+  2 | (pkg enabled)
+      ^^^^^^^^^^^^^
+  Error: The (pkg ...) configuration is only valid in dune-workspace, not a
+  dune-project.
+  Hint: Add this configuration to your dune-workspace file (create one in your
+  workspace root if you don't have one).
+  [1]


cc @avsm, since you encountered this situation before, could you give this a read and see if you would have found it useful with your issue before?

Alizter

Sorry to keep iterating on the wording but I am still not happy. Let me block this to avoid others merging.

Alizter · 2025-11-28T12:22:10Z

File "dune-project", line 2, characters 0-13:
2 | (pkg enabled)
    ^^^^^^^^^^^^^
Error: The (pkg ...) stanza is only valid in dune-workspace files.
Hint: Move this stanza to your dune-workspace file. If you don't have one,
create one in your workspace root.

This should be better I think.

benodiwal · 2025-11-28T12:33:38Z

Sorry to keep iterating on the wording but I am still not happy. Let me block this to avoid others merging.

Sure @Alizter

benodiwal · 2025-12-01T08:01:04Z

Hey @Alizter, is there any update on the new message text

Alizter · 2025-12-01T10:47:21Z

@benodiwal Did you see my proposed wording above? #12802 (comment)

benodiwal · 2025-12-01T10:50:27Z

@benodiwal Did you see my proposed wording above? #12802 (comment)

Ohh shoot, missed it. Making the change now

Signed-off-by: Sachin Beniwal <[email protected]>

Alizter · 2025-12-01T11:00:02Z

src/dune_lang/dune_project.ml

+                 "Move this stanza to your dune-workspace file. If you don't have one,\n\
+                 \                 create one in your workspace root."


You don't need to put new lines in the text. Pp will break it appropriately for you.

I have fixed it

Signed-off-by: Sachin Beniwal <[email protected]>

Alizter

Looks great. Thanks!

Alizter · 2025-12-01T11:11:49Z

@shonfeder This will be in 3.21, but I don't know if you want to keep the changelog entry since it is package management related. I think its fine to keep in this case but I defer judgement to you.

@ElectreAAS

Before this change, it was producing the category subheadings at the wrong level (level 2), instead of level 3. An example of the corrected changelog produced: ``` 3.21.0 (2025-12-08) -------------------- ### Fixed - Fix `include_subdirs qualified` incorrectly picking the furthest module instead of the closest when resolving module name ambiguities. (#12587, @ElectreAAS and @Alizter) - Fix: include the module alias in the transitive dependency closure with `(include_subdirs qualified)`. (#12299, @anmonteiro) - Pass private modules with -H when this is available (#12666, @rgrinberg) - Allow multiple modules in `(modules_flags ...)`, in `coq.theory` (#12733, @rlepigre) - Improve error message for invalid version formats in both `(lang dune ...)` and `(using extension ...)` declarations. Changes "Atom of the form NNN.NNN expected" to "Invalid version. Version must be two numbers separated by a dot." (#12833, @benodiwal) - Fix crash when running `dune build @check` on a library with virtual modules. (#12644, fixes #12636, @Alizter) - Provide a more informative error message when `(pkg enabled)` is put in `dune-project` instead of `dune-workspace`. (#12802, fixes #12801, @benodiwal) - Improve error message when invalid version strings are used in `dune-project` files. Non-ASCII characters and malformed versions now show a helpful hint with an example of the correct format. (#12794, fixes #12751, @benodiwal) - Stop hiding the `root_module` from the include path (#12239, @rgrinberg) - Allow `$ dune init` to work on absolute paths (#12601, fixes #7806, @rgrinberg) - `(include_subdirs qualified)`: Add missing alias dependency to module group. (#12530, @anmonteiro) - Add Melange compilation to the `@all` alias in libraries (#12628, @anmonteiro) - melange support: don't emit empty JavaScript modules for generated module aliases. (#12464, @anmonteiro) ### Added - (Experimental): Introduce the `library_parameter` stanza. It allows users to declare a parameter when using the OxCaml compiler. (#11963, implements #12084, @maiste) - Added the ability to scroll horizontally in TUI. (#12386, @Alizter) - Feature: Include shell command that was executed when a cram test has occurred in the error message (#12307, @rgrinberg) - Add support for `%{cmt:...}` and `%{cmti:...}` variables to reference compiled annotation files (.cmt and .cmti) containing typed abstract syntax trees with location and type information. (#12634, grants #12633, @Alizter) - Add `$ dune describe tests` to describe the tests in the workspace (@Gromototo, #12545, fixes #12030) - Allow `dune runtest` to properly run while a watch mode server is running. (#12473, grants #8114, @gridbugs and @ElectreAAS) - Use copy-on-write (COW) when copying files on filesystems that support it (Btrfs, ZFS, XFS, etc), under Linux. (#12074, fixes #12071, @nojb) - Add support for Tangled ATproto-based code repositories (#12197, @avsm) - Add support for instantiating OxCaml parameterised libraries. (#12561, @art-w) ``` Note that the `Fixed` and `Added` headers are at level 3. Compare with the previous, incorrect logic, produced initially in the current release branch: https://github.com/shonfeder/dune/blob/c5af78ff322225de3e839982600a1180caa951bf/CHANGES.md?plain=1 This incorrect formatting caused the documented release process to produce release with no changelog (e.g., https://github.com/ocaml/dune/releases/tag/3.21.0_alpha0) Signed-off-by: Shon Feder <[email protected]>

@ElectreAAS

CHANGES: ### Fixed - Fix `include_subdirs qualified` incorrectly picking the furthest module instead of the closest when resolving module name ambiguities. (ocaml/dune#12587, @ElectreAAS and @Alizter) - Fix: include the module alias in the transitive dependency closure with `(include_subdirs qualified)`. (ocaml/dune#12299, @anmonteiro) - Improve error messages for invalid version formats containing non-ASCII characters. Previously, non-ASCII characters in version strings (e.g., `(lang dune è)` or `(using menhir π3.14)`) would fail with a generic "Invalid file" error. Now they display a clear message: "Invalid atom: contains non-ASCII character(s). Atoms must only contain ASCII characters." The fix is implemented at the lexer level, providing consistent error handling across all s-expression parsing. (ocaml/dune#12844, fixes ocaml/dune#12836, @benodiwal) - Pass private modules with -H when this is available (ocaml/dune#12666, @rgrinberg) - Allow multiple modules in `(modules_flags ...)`, in `coq.theory` (ocaml/dune#12733, @rlepigre) - Improve error message for invalid version formats in both `(lang dune ...)` and `(using extension ...)` declarations. Changes "Atom of the form NNN.NNN expected" to "Invalid version. Version must be two numbers separated by a dot." (ocaml/dune#12833, @benodiwal) - Fix crash when running `dune build @check` on a library with virtual modules. (ocaml/dune#12644, fixes ocaml/dune#12636, @Alizter) - Provide a more informative error message when `(pkg enabled)` is put in `dune-project` instead of `dune-workspace`. (ocaml/dune#12802, fixes ocaml/dune#12801, @benodiwal) - Improve error message when invalid version strings are used in `dune-project` files. Non-ASCII characters and malformed versions now show a helpful hint with an example of the correct format. (ocaml/dune#12794, fixes ocaml/dune#12751, @benodiwal) - Stop hiding the `root_module` from the include path (ocaml/dune#12239, @rgrinberg) - Allow `$ dune init` to work on absolute paths (ocaml/dune#12601, fixes ocaml/dune#7806, @rgrinberg) - `(include_subdirs qualified)`: Add missing alias dependency to module group. (ocaml/dune#12530, @anmonteiro) - Add Melange compilation to the `@all` alias in libraries (ocaml/dune#12628, @anmonteiro) - Fix greedy version location in lang declarations. Previously, error locations for invalid lang versions would span multiple bytes for multi-byte UTF-8 characters, causing carets to appear misaligned and seemingly include the closing parenthesis. Now, error locations for ASCII strings show the full length (e.g., "Ali" shows `^^^`), while non-ASCII strings show only the first byte (e.g., "è" shows `^`) to avoid multi-byte character display issues. (ocaml/dune#12869, fixes ocaml/dune#12806, @benodiwal) - melange support: don't emit empty JavaScript modules for generated module aliases. (ocaml/dune#12464, @anmonteiro) ### Added - (Experimental): Introduce the `library_parameter` stanza. It allows users to declare a parameter when using the OxCaml compiler. (ocaml/dune#11963, implements ocaml/dune#12084, @maiste) - Added the ability to scroll horizontally in TUI. (ocaml/dune#12386, @Alizter) - Feature: Include shell command that was executed when a cram test has occurred in the error message (ocaml/dune#12307, @rgrinberg) - support expanding variables in `(promote (into ..))` (ocaml/dune#12832, fixes ocaml/dune#12742, @anmonteiro) - Add support for `%{cmt:...}` and `%{cmti:...}` variables to reference compiled annotation files (.cmt and .cmti) containing typed abstract syntax trees with location and type information. (ocaml/dune#12634, grants ocaml/dune#12633, @Alizter) - Add `$ dune describe tests` to describe the tests in the workspace (@Gromototo, ocaml/dune#12545, fixes ocaml/dune#12030) - Add `argv`, the process environment, and the dune version to the config event in the trace (ocaml/dune#12909, @rgrinberg) - Allow `dune runtest` to properly run while a watch mode server is running. (ocaml/dune#12473, grants ocaml/dune#8114, @gridbugs and @ElectreAAS) - Use copy-on-write (COW) when copying files on filesystems that support it (Btrfs, ZFS, XFS, etc), under Linux. (ocaml/dune#12074, fixes ocaml/dune#12071, @nojb) - Add support for Tangled ATproto-based code repositories (ocaml/dune#12197, @avsm) - Add support for instantiating OxCaml parameterised libraries. (ocaml/dune#12561, @art-w) - Add a `(conflict_markers error|ignore)` option to the cram stanza. When `(conflict_markers error)` is set, the cram test will fail in the presence of conflict markers. Git, diff3 and jujutsu conflict markers are detected. (ocaml/dune#12538, ocaml/dune#12617, ocaml/dune#12655, fixes ocaml/dune#12512, @rgrinberg, @Alizter) - Introduce a `%{ppx:lib1+..+libn}` stanza to make it possible to refer to ppx executables built by dune. This is useful for writing tests (ocaml/dune#12711, @rgrinberg) - Introduce a `(dir ..)` field on packages defined in the `dune-project`. This field allows to associate a directory with a particular package. This makes dune automatically filter out all stanzas in this directory and its descendants with `--only-packages`. All users are recommended to switch to using this field. (ocaml/dune#12614, fixes ocaml/dune#3255, @rgrinberg) - Add support for `DUNE_ROOT` environment variable, similar to the existing `--root` CLI parameter. (fixes ocaml/dune#12399 @sir4ur0n) - Introduce an `unused-libs` alias to detect unused libraries. (ocaml/dune#12623, fixes ocaml/dune#650, @rgrinberg) - Add `--files` flag to `dune describe opam-files` to print only the names of the opam files line by line. (ocaml/dune#9793, @reynir and @Alizter) - `dune exec` now accepts absolute paths inside the workspace. (ocaml/dune#12094, @Alizter) - Add `coqdoc_header` and `coqdoc_footer` fields to the `coq` field of the `env` stanza, and to the `coq.theory` stanza, allowing to configure a custom header or footer respectively in the HTML output of `coqdoc`. (ocaml/dune#11131, @rlepigre) - Allow `dune fmt` to properly run while a watch mode server is running. Note that the `--preview` flag is not supported in this mode. (ocaml/dune#12064, @ElectreAAS) - Support for generating `_CoqProject` files for `coq.theory` stanzas. (ocaml/dune#11752, @rlepigre) - Added `(files)` stanza, similar to `(dirs)` to control which files are visible to Dune on a per-directory basis. (ocaml/dune#12879, @nojb) - Add support for %{ocaml-config:ox} (ocaml/dune#12236, @jonludlam) - Introduce `dune promotion show` command to display the contents of corrected files that are ready for promotion. This allows users to preview changes before running `dune promote`. The command accepts file arguments to show specific files, or displays all promotable files when called without arguments. (ocaml/dune#12669, fixes ocaml/dune#3883, @MixiMaxiMouse) - New `(lang rocq)` build mode for Rocq 9.0 and later. This new mode is very similar to the existing `(lang coq)`, except that it doesn't need the `coq*` compatibility wrappers. As of today `(lang rocq)` doesn't support yet composed builds with Rocq itself, this will be added later. `(lang coq)` is deprecated, development is frozen, and will be removed at some point in the future. (ocaml/dune#12035, @ejgallego, @Lysxia, fixes ocaml/dune#11572) ### Changed - Don't run `ocamldep` to compute false dependencies on the `root_module` (ocaml/dune#12227, @rgrinberg) - `dune format-dune-file` now uses the syntax version of the Dune project that contains the file being formatted (if any) instead of using the latest version available, which remains the default if there is no Dune project in scope. (ocaml/dune#11865, @nojb) - Persistent DB and process events have been slightly modified. Persistent DB events have more concise names and job events always include full information. (ocaml/dune#12867, @rgrinberg) - Removed the `--trace-extended` flag. Its functionality is always enabled when tracing is active (ocaml/dune#12908, @rgrinberg) - The `test/dune` file generated by `dune init proj` now depends on the project library. (ocaml/dune#12791, @shonfeder) - Starting with version 3.21 of the Dune language, Dune no longer changes the default set of compiler warnings. For users that would like to keep the old behaviour, the variable `%{dune-warnings}` can be used in an `(env)` stanza in a top-level Dune file: `(env (dev (flags :standard %{dune-warnings})))`. (ocaml/dune#12766, @nojb) - Fix: stop generating `cmt` files for cinaps binaries (ocaml/dune#12530, @rgrinberg)

benodiwal force-pushed the fix/good-error-msg-pkg-enabled branch from 47184b5 to 80dd5e8 Compare November 27, 2025 13:50

Leonidas-from-XIV requested a review from Alizter November 27, 2025 13:54

Alizter reviewed Nov 27, 2025

View reviewed changes

src/dune_lang/dune_project.ml Outdated Show resolved Hide resolved

Leonidas-from-XIV previously requested changes Nov 27, 2025

View reviewed changes

benodiwal force-pushed the fix/good-error-msg-pkg-enabled branch from 73e634e to 5bc200f Compare November 27, 2025 15:42

Alizter self-requested a review November 27, 2025 17:25

Alizter self-assigned this Nov 27, 2025

benodiwal force-pushed the fix/good-error-msg-pkg-enabled branch from 5bc200f to 39c60f3 Compare November 27, 2025 18:23

Alizter reviewed Nov 28, 2025

View reviewed changes

src/dune_lang/dune_project.ml Outdated Show resolved Hide resolved

benodiwal force-pushed the fix/good-error-msg-pkg-enabled branch from 39c60f3 to d031b30 Compare November 28, 2025 10:08

Alizter previously approved these changes Nov 28, 2025

View reviewed changes

benodiwal force-pushed the fix/good-error-msg-pkg-enabled branch from c74120e to 58b4cfe Compare November 28, 2025 11:00

Alizter reviewed Nov 28, 2025

View reviewed changes

src/dune_lang/dune_project.ml Show resolved Hide resolved

Alizter reviewed Nov 28, 2025

View reviewed changes

Alizter approved these changes Nov 28, 2025

View reviewed changes

Alizter requested changes Nov 28, 2025

View reviewed changes

benodiwal force-pushed the fix/good-error-msg-pkg-enabled branch from b740483 to 47180f5 Compare November 28, 2025 21:47

benodiwal added 3 commits December 1, 2025 16:21

fix: good error message for pkg enabled put in dune-project

7792499

Signed-off-by: Sachin Beniwal <[email protected]>

tests

0cbcd31

Signed-off-by: Sachin Beniwal <[email protected]>

fix: improve code

5e3dcd9

Signed-off-by: Sachin Beniwal <[email protected]>

benodiwal added 3 commits December 1, 2025 16:21

fix: a more helpful message

6a6228e

Signed-off-by: Sachin Beniwal <[email protected]>

chore: added entry for CHANGES.md

b8fca75

Signed-off-by: Sachin Beniwal <[email protected]>

fix: remove unnecessary version check for pkg field in dune-project

a95cd99

Signed-off-by: Sachin Beniwal <[email protected]>

benodiwal force-pushed the fix/good-error-msg-pkg-enabled branch from 47180f5 to a95cd99 Compare December 1, 2025 10:51

fix: improved error text

97623ce

Signed-off-by: Sachin Beniwal <[email protected]>

Alizter reviewed Dec 1, 2025

View reviewed changes

fix: fmt

f240732

Signed-off-by: Sachin Beniwal <[email protected]>

Alizter approved these changes Dec 1, 2025

View reviewed changes

Alizter enabled auto-merge December 1, 2025 11:11

Alizter merged commit 79117e3 into ocaml:main Dec 1, 2025
27 checks passed

shonfeder mentioned this pull request Dec 11, 2025

[new release] dune (17 packages) (3.21.0~alpha3) ocaml/opam-repository#29070

Closed

shonfeder mentioned this pull request Dec 15, 2025

[new release] dune (17 packages) (3.21.0~alpha4) ocaml/opam-repository#29096

Draft

		"Move this stanza to your dune-workspace file. If you don't have one,\n\
		\ create one in your workspace root."

Fix/good error msg pkg enabled #12802

Fix/good error msg pkg enabled #12802

Uh oh!

Conversation

benodiwal commented Nov 27, 2025

Uh oh!

Uh oh!

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benodiwal commented Nov 27, 2025

Uh oh!

Alizter commented Nov 27, 2025

Uh oh!

Leonidas-from-XIV commented Nov 27, 2025

Uh oh!

Uh oh!

Uh oh!

Alizter Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Alizter left a comment

Choose a reason for hiding this comment

Uh oh!

Alizter commented Nov 28, 2025

Uh oh!

benodiwal commented Nov 28, 2025

Uh oh!

benodiwal commented Dec 1, 2025

Uh oh!

Alizter commented Dec 1, 2025

Uh oh!

benodiwal commented Dec 1, 2025

Uh oh!

Alizter Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

benodiwal Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Alizter left a comment

Choose a reason for hiding this comment

Uh oh!

Alizter commented Dec 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants