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

…rtual lib (#10051) * fix(melange): depend on selected impl when setting up JS rules for virtual lib chore: add changelog entry * chore: use dev version of melange for testing fix: unrelated melange private-lib-dep test due to output change --------- Signed-off-by: Antonio Nuno Monteiro <[email protected]>
ocaml · Feb 17, 2024 · 579d59d · 579d59d
1 parent 6aa0b8a
commit 579d59d
Show file tree

Hide file tree

Showing 10 changed files with 72 additions and 29 deletions.
diff --git a/Makefile b/Makefile
@@ -25,7 +25,7 @@ ppx_inline_test \
 ppxlib \
 ctypes \
 "utop>=2.6.0" \
-"melange>=3.0.0"
+"melange"
 # Dependencies recommended for developing dune locally,
 # but not wanted in CI
 DEV_DEPS := \
@@ -74,14 +74,22 @@ install-ocamlformat:
 dev-depext:
 	opam depext -y $(TEST_DEPS)
 
+.PHONY: melange
+melange:
+	opam pin add -n melange.dev https://github.com/melange-re/melange.git#v4-414-dev
+
 .PHONY: dev-deps
-dev-deps:
+dev-deps: melange
 	opam install -y $(TEST_DEPS)
 
 .PHONY: coverage-deps
 coverage-deps:
 	opam install -y bisect_ppx
 
+.PHONY: dev-deps-sans-melange
+dev-deps-sans-melange:
+	opam install -y $(TEST_DEPS)
+
 .PHONY: dev-switch
 dev-switch:
 	opam update
@@ -109,6 +117,9 @@ test-melange: $(BIN)
 test-all: $(BIN)
 	$(BIN) build @runtest @runtest-js @runtest-coq @runtest-melange
 
+test-all-sans-melange: $(BIN)
+	$(BIN) build @runtest @runtest-js @runtest-coq
+
 test-coverage: $(BIN)
 	- $(BIN) build --instrument-with bisect_ppx --force @runtest
 	bisect-ppx-report send-to Coveralls

diff --git a/ci/build-test.sh b/ci/build-test.sh
@@ -114,10 +114,16 @@ windows_*)
     opamrun pin remove ppx_expect --no-action
     opamrun pin remove time_now --no-action
 
-    opamrun exec -- make dev-deps
+    # This should have been:
+    #   opam exec -- make dev-deps
+    # but melange requires ocaml 4.14 (DKML hasn't been updated as of 2022-11)
+    opamrun exec -- make dev-deps-sans-melange
 
     echo ======== Run test suite on Unix
-    opamrun exec -- make test
+    # This should have been:
+    #   opam exec -- make test
+    # but melange requires ocaml 4.14 (DKML hasn't been updated as of 2022-11)
+    opamrun exec -- make test-all-sans-melange
 esac
 
 echo ======== Build configurator

diff --git a/doc/changes/10051.md b/doc/changes/10051.md
@@ -0,0 +1,3 @@
+- melange: fix inconsistency in virtual library implementation. Concrete
+  modules within a virtual library can now refer to its virtual modules too
+  (#10051, fixes #7104, @anmonteiro)
diff --git a/flake.lock b/flake.lock
diff --git a/flake.nix b/flake.nix
@@ -8,8 +8,8 @@
       inputs.flake-utils.follows = "flake-utils";
     };
     melange = {
-      # When moving the compiler tests to OCaml 5.1, change to v3-51
-      url = "github:melange-re/melange/v3-414";
+      # When moving the compiler tests to OCaml 5.1, change to v4-51-dev
+      url = "github:melange-re/melange/v4-414-dev";
       inputs.nixpkgs.follows = "nixpkgs";
       inputs.flake-utils.follows = "flake-utils";
     };

diff --git a/src/dune_rules/melange/melange_rules.ml b/src/dune_rules/melange/melange_rules.ml
@@ -456,12 +456,27 @@ let setup_js_rules_libraries
         let* vlib = Resolve.Memo.read_memo vlib in
         let* includes =
           let+ requires_link =
-            Lib.Compile.for_lib
-              ~allow_overlaps:mel.allow_overlapping_dependencies
-              (Scope.libs scope)
-              vlib
-            |> Lib.Compile.requires_link
-            |> Memo.Lazy.force
+            let+ requires_link =
+              Lib.Compile.for_lib
+                ~allow_overlaps:mel.allow_overlapping_dependencies
+                (Scope.libs scope)
+                vlib
+              |> Lib.Compile.requires_link
+              |> Memo.Lazy.force
+            in
+            let open Resolve.O in
+            let+ requires_link = requires_link in
+            (* Whenever a `concrete_lib` implementation contains a field
+               `(implements virt_lib)`, we also set up the JS targets for the
+               modules defined in `virt_lib`.
+
+               In the cases where `virt_lib` (concrete) modules depend on any
+               virtual modules (i.e. programming against the interface), we
+               need to make sure that the JS rules that dune emits for
+               `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
           in
           cmj_includes ~requires_link ~scope
         in

diff --git a/test/blackbox-tests/test-cases/melange/private-lib-dep.t b/test/blackbox-tests/test-cases/melange/private-lib-dep.t
@@ -48,9 +48,9 @@ Melange public library depends on private library
   // Generated by Melange
   'use strict';
   
-  var Priv = require("priv/priv.js");
+  let Priv = require("priv/priv.js");
   
-  var x = "public lib uses " + Priv.x;
+  const x = "public lib uses " + Priv.x;
   
   exports.x = x;
   /* No side effect */

diff --git a/test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/dune b/test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/dune
@@ -7,4 +7,5 @@
  (target output)
  (alias mel)
  (modules mel)
+ (emit_stdlib false)
  (libraries vlib impl_melange))
diff --git a/test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/mel.ml b/test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/mel.ml
@@ -1 +1 @@
-print_endline Virt.t
+let () = print_endline Vlib_impl.hello
diff --git a/test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/run.t b/test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/run.t
@@ -19,18 +19,25 @@ The native build passes
       ocamlopt ml.exe
   Hello from ml
 
-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
+Any module requiring a virtual module (including modules within the virtual
+library itself) needs to consult the `.cmj` file for the concrete
+implementation being seleced to know where to `import` from in the generated
+JS. The following build works because Dune tracks concrete implementation
+`.cmj` files as dependencies of the JS rules.
 
   $ dune build @mel --display=short 2>&1 | grep -v 'node_modules/melange'
           melc vlib/.vlib.objs/melange/virt.{cmi,cmti}
       ocamldep impl_melange/.impl_melange.objs/virt.impl.d
-          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}
+          melc vlib/.vlib.objs/melange/shared.{cmi,cmj,cmt}
+          melc impl_melange/.impl_melange.objs/melange/virt.{cmj,cmt}
+          melc .output.mobjs/melange/melange__Mel.{cmi,cmj,cmt}
+          melc output/impl_melange/virt.js
+          melc output/vlib/shared.js
+          melc output/vlib/vlib_impl.js
+          melc output/mel.js
 
   $ output=_build/default/output/mel.js
   $ test -f "$output" && node "$output"
-  [1]
+  Hello from melange
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		print_endline Virt.t
		let () = print_endline Vlib_impl.hello