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.

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

Inconsistent behaviour for virtual libraries using melange mode #7104

Closed
rizo opened this issue Feb 17, 2023 · 3 comments · Fixed by #10051 or ocaml/opam-repository#25615
Closed

Inconsistent behaviour for virtual libraries using melange mode #7104

rizo opened this issue Feb 17, 2023 · 3 comments · Fixed by #10051 or ocaml/opam-repository#25615

Comments

@rizo
Copy link
Contributor

rizo commented Feb 17, 2023

Expected Behavior

When creating a virtual library in native or byte modes, it is possible to assign a module to be virtual and add an implementation and interface that wraps around the virtual module in the virtual library.

Specifically the following structure illustrates this:

- vlib/
  - virt.mli     # virtual module
  - vlib.mli     # interface of vlib
  - vlib.ml      # implementation of vlib (uses virt!)
- vlib_impl_1/
  - virt.ml      # implementation of the virt module for vlib

This works as expected in native and byte modes and allows providing multiple implementations for virt.mli while offering a high-level interface in vlib.mli.

This doesn't work in melange mode.

Actual Behavior

This doesn't work when (modes melange) is used. Not only it is not possible to replicate the described workflow, but having (modes melange) also prevents the non-melange implementation to be compiled.

Error when compiling a melange target:

$ dune build @melange
File "vlib/vlib.ml", line 1:        
Error: Virt not found, it means either the module does not exist or it is a namespace

Additionally, if (modes melange) is present in vlib, the native compilation fails with:

File "src/dune_rules/virtual_rules.ml", line 11, characters 30-37:
Error: No rule found for vlib/.vlib.objs/byte/virt.cmi
File "src/dune_rules/virtual_rules.ml", line 11, characters 30-37:
Error: No rule found for vlib/.vlib.objs/byte/vlib.cmi
File "src/dune_rules/virtual_rules.ml", line 11, characters 30-37:
Error: No rule found for vlib/.vlib.objs/byte/vlib.cmo
File "src/dune_rules/virtual_rules.ml", line 11, characters 30-37:
Error: No rule found for vlib/.vlib.objs/native/vlib.cmx
File "src/dune_rules/virtual_rules.ml", line 11, characters 30-37:
Error: No rule found for vlib/.vlib.objs/native/vlib.o

Reproduction

The following repository fully replicates the above scenario based on the blackbox test included in dune:

Specifications

  • Version of dune: ~3.7 05797cd3f505beca4d6d62c576506d7608a6d29d
  • Version of melange: f4ddebbc7755af5e983c4df2ac51498f286e1074
  • Version of ocaml: 4.14.0
  • Operating system (distribution and version): 22.3.0 Darwin
@rizo rizo mentioned this issue Feb 28, 2023
3 tasks
anmonteiro added a commit to anmonteiro/dune that referenced this issue Mar 1, 2023
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this issue Mar 1, 2023
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
rgrinberg pushed a commit that referenced this issue Mar 1, 2023
* test(melange): add test exercising #7104

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@jchavarri
Copy link
Collaborator

From the test:

Melange can't produce a .cmj solely from a virtual module .cmi, because it
needs to consult the .cmj files of dependencies to know where the require
call should be emitted

I am trying to figure out if the solution for this is something that can be solved on dune side, on melange side, or a combination of both 😅

On Melange side:

  • The cmj shouldn't really require generating js requires, but I think at the moment melange embeds the resulting js output inside the cmj for simplicity?
  • I believe there are other performance optimization in melange that might inline functions from other modules by reading their cmjs, not sure of any solution that removes the cmj-cmj dependency would also be affected by that

On Dune side:

  • I believe there's no particular need for producing cmjs on the virtual library folder. Would it be possible to add copy rules so that the virtual lib modules and the implementation lib modules are placed in the same folder? At that point, we could proceed with regular compilation normally.

@anmonteiro @rgrinberg wdyt?

@jchavarri
Copy link
Collaborator

Adding a note here from @anmonteiro: solving this issue requires implementing "delayed cmj optimization" (melange-re/melange#464), so in terms of timing in won't be fixed as part of dune 3.8.

jchavarri added a commit to jchavarri/dune that referenced this issue Nov 15, 2023
* main: (56 commits)
  feature: add terminal ui backend based on NoTTY (ocaml#6996)
  doc(coq): update documentation about coqdep
  fix(rules): don't descend into automatic subdirs infinitely (ocaml#7208)
  benchmark: add warm run (ocaml#7198)
  test: vendored and public libs (ocaml#7197)
  test: use sh in concurrent test (ocaml#7205)
  fix: custom log file path (ocaml#7200)
  test(melange): add test exercising ocaml#7104 (ocaml#7204)
  test(melange): add a test that introduces rules in the target dir (ocaml#7196)
  test: duplicate packages in vendor dir (ocaml#7194)
  melange: interpret `melc --where` as a list of `:`-separated paths (ocaml#7176)
  perf: add synthetic benchmark (ocaml#7189)
  Test case for bug report (ocaml#6725)
  Add test illustrating ocaml#6575 (ocaml#6576)
  chore: add rule streaming proposal (ocaml#7195)
  test(stdlib): merge wrapped/unwrapped tests
  test: move all stdlib tests
  fix: allow unwrapped libraries with `(stdlib ..)`
  test: demonstrate crash in modules.ml when `(stdlib .. )` used with `(wrapped false)`
  fix(install): respect display options (ocaml#7116)
  ...
@anmonteiro
Copy link
Collaborator

I'm fixing this over in #10051.

@rizo if you're still interested in this fix and would like to take it for a spin, I'd appreciate your feedback.

In terms of setup, you'll need:

  1. pin melange to fix: handle case of missing .cmj when trying to inline external field melange-re/melange#1067
  2. pin dune to fix(melange): depend on selected impl when setting up JS rules for virtual lib #10051
  3. try compiling both a) your minimal reproduction and b) your real-world library where you'd like to use this feature

Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue Mar 26, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue Apr 3, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment