Skip to content

fix: %{cmi:...} and other module artifact pforms under (include_subdirs qualified)#14498

Open
Alizter wants to merge 1 commit into
ocaml:mainfrom
Alizter:push-qnnqsrklzkrs
Open

fix: %{cmi:...} and other module artifact pforms under (include_subdirs qualified)#14498
Alizter wants to merge 1 commit into
ocaml:mainfrom
Alizter:push-qnnqsrklzkrs

Conversation

@Alizter
Copy link
Copy Markdown
Collaborator

@Alizter Alizter commented May 12, 2026

Under (include_subdirs qualified), building any module artifact pform target (%{cmi:...}, %{cmo:...}, %{cmx:...}, %{cmt:...}, %{cmti:...}) crashes with Map.add_exn: key already exists when two qualified subdirectories define modules with the same name.

Also reproduces in this repo: dune build %{cmi:bin/main} now works (bin/ uses (include_subdirs qualified) and contains four group.ml files in different qualified subdirs).

@Alizter Alizter force-pushed the push-qnnqsrklzkrs branch from 748d4fb to 6620ad5 Compare May 12, 2026 00:51
@shonfeder shonfeder requested review from ElectreAAS and shonfeder May 13, 2026 12:12
@Alizter Alizter force-pushed the push-qnnqsrklzkrs branch from 6620ad5 to cd39f49 Compare May 20, 2026 13:25
@Alizter Alizter marked this pull request as ready for review May 20, 2026 13:26
@Alizter Alizter force-pushed the push-qnnqsrklzkrs branch from cd39f49 to 03895d4 Compare May 20, 2026 13:30
Copy link
Copy Markdown
Member

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

I have one suggestion to improve (imo) readability, which is optional, but requests about test coverage for user errors, which, given my current understanding, should be addressed.

Comment thread src/dune_rules/expander.ml Outdated
Comment thread src/dune_rules/expander.ml Outdated
Comment thread src/dune_rules/expander.ml Outdated
@Alizter Alizter marked this pull request as draft May 20, 2026 18:16
@Alizter Alizter force-pushed the push-qnnqsrklzkrs branch from 03895d4 to bd8b1d3 Compare May 21, 2026 09:18
@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented May 21, 2026

I need to work out two things to make progress on this PR:

  1. Get rid of the ad-hoc path/module-path conversion. We do this properly for include_subdirs qualified somehwere.
  2. Consider accepting a wider range of syntax. We currently allow %{cmo:foo/bar} in line with include_subdirs unqualified, however for qualified we may want to allow some sort of module path here too, such as %{cmo:foo.bar}. It will need to be thought about.

@Alizter Alizter force-pushed the push-qnnqsrklzkrs branch 3 times, most recently from 3ce789a to d3ed695 Compare May 21, 2026 09:55
@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented May 21, 2026

Okay I've addressed (1), but (2) is trickier. I thought about ways we can mix paths and module paths like %{cmi:foo/bar.zar} but it got tricky. I think just having %{cmi:foo/bar/zar} is less confusing and is already what we do for unqualified.

@Alizter Alizter requested a review from shonfeder May 21, 2026 11:07
@Alizter Alizter marked this pull request as ready for review May 21, 2026 11:07
Copy link
Copy Markdown
Member

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

This looks like a really nice simplification over the first iteration! The only change that I think of as blocking here as the release note. Other suggestions are just stylistic or subjective readability notes.

I would like clarification my question about the safety of add_exn tho!

Comment thread doc/changes/fixed/14498.md Outdated
Comment thread src/dune_rules/artifacts_obj.ml Outdated
Comment thread src/dune_rules/artifacts_obj.ml
Comment thread src/dune_rules/artifacts_obj.ml
Comment thread src/dune_rules/artifacts_obj.ml
Comment thread src/dune_rules/expander.ml Outdated
…rs qualified)

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the push-qnnqsrklzkrs branch from d3ed695 to 254932b Compare May 21, 2026 20:55
@Alizter Alizter requested a review from shonfeder May 21, 2026 20:57
Copy link
Copy Markdown
Member

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the explanations!

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.

module artifact targets %{cmi:...} etc. crash on duplicate leaf module names

2 participants