fix(melange): depend on selected impl when setting up JS rules for virtual lib #10051

anmonteiro · 2024-02-16T06:28:30Z

TODO:

this change depends on fix: handle case of missing .cmj when trying to inline external field melange-re/melange#1067. need to:
- port it back to the melange 4.14 branch so that we can run it in dune's tests
- depend on the melange development version for CI in dune

Even though the PR is in draft, I'm requesting your reviews to make sure the approach makes sense.

anmonteiro · 2024-02-16T08:03:07Z

src/dune_rules/melange/melange_rules.ml

+               `virt_lib` depend on `concrete_lib`, such that Melange can find
+               the correct `.cmj` file, which is needed to emit the correct
+               path in `import` / `require`. *)
+            lib :: requires_link


the callout here is:

the JS targets of the virtual lib depend on the library artifacts (e.g. .cm{i,j,t}) of the selected (concrete) implementation

This is analogous to adding the correct .cmx to the link path in native compilation.

anmonteiro · 2024-02-16T08:05:17Z

test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/mel.ml

@@ -1 +1 @@
-print_endline Virt.t
+let () = print_endline Vlib_impl.hello


just changed this to be the same as the ml implementation. Vlib_impl calls Virt anyway, preserving what we want to test.

jchavarri

The fix and explanation make sense to me, the dependency flow (impl lib depends on virtual lib artifacts) seems "natural".

I wonder if there are cases where the newly added dependency can lead to cycles?

jchavarri · 2024-02-16T16:03:29Z

test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/run.t

-          melc vlib/.vlib.objs/melange/vlib_impl.{cmi,cmj,cmt} (exit 2)
-  File "vlib/vlib_impl.ml", line 1:
-  Error: Virt not found, it means either the module does not exist or it is a namespace
+          melc vlib/.vlib.objs/melange/vlib_impl.{cmi,cmj,cmt}


I think the description in line 22 needs to be updated.

anmonteiro · 2024-02-16T18:55:06Z

The fix and explanation make sense to me, the dependency flow (impl lib depends on virtual lib artifacts) seems "natural".

It’s actually the other way around: virtual lib JS depends on concrete lib artifacts (because it might call it)

anmonteiro · 2024-02-16T18:57:08Z

I wonder if there are cases where the newly added dependency can lead to cycles?

I wondered about it too, and I think it could only be a problem if there were more than 1 concrete implementations, which is why I opened #10049 to verify it’s not possible.

anmonteiro · 2024-02-17T01:48:58Z

this is now ready for review. Not sure if @rgrinberg needs to take a look?

here's a summary of the included commits:

225564d fixes the issue and promotes the corresponding blackbox-test
7f90d9f adds the development version of Melange on 4.14 that includes a needed fix
a48d0f8 promotes an unrelated test due to an upstream change in melange JS output
da68148 adds a changelog entry

…rtual lib chore: add changelog entry Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix: unrelated melange private-lib-dep test due to output change Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

@frejsoya

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)

@frejsoya

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)

anmonteiro force-pushed the anmonteiro/fix-melange-virtual-libs branch from 0f31513 to 225564d Compare February 16, 2024 06:34

anmonteiro added the melange label Feb 16, 2024

anmonteiro requested review from rgrinberg and jchavarri February 16, 2024 06:34

anmonteiro mentioned this pull request Feb 16, 2024

Inconsistent behaviour for virtual libraries using melange mode #7104

Closed

anmonteiro commented Feb 16, 2024

View reviewed changes

jchavarri approved these changes Feb 16, 2024

View reviewed changes

anmonteiro force-pushed the anmonteiro/fix-melange-virtual-libs branch from a030134 to 7f90d9f Compare February 17, 2024 01:11

anmonteiro marked this pull request as ready for review February 17, 2024 01:29

rgrinberg added this to the 3.15.0 milestone Feb 17, 2024

rgrinberg approved these changes Feb 17, 2024

View reviewed changes

anmonteiro added 2 commits February 16, 2024 18:34

fix(melange): depend on selected impl when setting up JS rules for vi…

3a99d27

…rtual lib chore: add changelog entry Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

chore: use dev version of melange for testing

903a8f3

fix: unrelated melange private-lib-dep test due to output change Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

anmonteiro force-pushed the anmonteiro/fix-melange-virtual-libs branch from da68148 to 903a8f3 Compare February 17, 2024 02:34

anmonteiro enabled auto-merge (squash) February 17, 2024 03:26

Merge branch 'main' into anmonteiro/fix-melange-virtual-libs

a8d67ee

anmonteiro merged commit 579d59d into ocaml:main Feb 17, 2024
1 of 2 checks passed

anmonteiro deleted the anmonteiro/fix-melange-virtual-libs branch February 17, 2024 03:28

anmonteiro mentioned this pull request Feb 17, 2024

Cannot use virtual modules without specify implementation library in libraries melange-re/melange#658

Closed

jchavarri mentioned this pull request Feb 28, 2024

ci: fix melange benchmarks #10166

Merged

Leonidas-from-XIV mentioned this pull request Mar 26, 2024

[new release] dune (15 packages) (3.15.0~alpha1) ocaml/opam-repository#25585

Closed

Leonidas-from-XIV mentioned this pull request Apr 3, 2024

[new release] dune (15 packages) (3.15.0) ocaml/opam-repository#25615

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(melange): depend on selected impl when setting up JS rules for virtual lib #10051

fix(melange): depend on selected impl when setting up JS rules for virtual lib #10051

anmonteiro commented Feb 16, 2024 •

edited

Loading

anmonteiro Feb 16, 2024 •

edited

Loading

anmonteiro Feb 16, 2024 •

edited

Loading

jchavarri left a comment

jchavarri Feb 16, 2024

anmonteiro commented Feb 16, 2024

anmonteiro commented Feb 16, 2024

anmonteiro commented Feb 17, 2024 •

edited

Loading

		@@ -1 +1 @@
		print_endline Virt.t
		let () = print_endline Vlib_impl.hello

fix(melange): depend on selected impl when setting up JS rules for virtual lib #10051

fix(melange): depend on selected impl when setting up JS rules for virtual lib #10051

Conversation

anmonteiro commented Feb 16, 2024 • edited Loading

anmonteiro Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

anmonteiro Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

jchavarri left a comment

Choose a reason for hiding this comment

jchavarri Feb 16, 2024

Choose a reason for hiding this comment

anmonteiro commented Feb 16, 2024

anmonteiro commented Feb 16, 2024

anmonteiro commented Feb 17, 2024 • edited Loading

anmonteiro commented Feb 16, 2024 •

edited

Loading

anmonteiro Feb 16, 2024 •

edited

Loading

anmonteiro Feb 16, 2024 •

edited

Loading

anmonteiro commented Feb 17, 2024 •

edited

Loading