From 3a99d27afad43a0a7949d82e7481e98a9f89291d Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Thu, 15 Feb 2024 22:22:06 -0800 Subject: [PATCH 1/2] fix(melange): depend on selected impl when setting up JS rules for virtual lib chore: add changelog entry Signed-off-by: Antonio Nuno Monteiro --- doc/changes/10051.md | 3 +++ src/dune_rules/melange/melange_rules.ml | 27 ++++++++++++++----- .../melange/virtual_lib_compilation.t/dune | 1 + .../melange/virtual_lib_compilation.t/mel.ml | 2 +- .../melange/virtual_lib_compilation.t/run.t | 21 ++++++++++----- 5 files changed, 40 insertions(+), 14 deletions(-) create mode 100644 doc/changes/10051.md diff --git a/doc/changes/10051.md b/doc/changes/10051.md new file mode 100644 index 00000000000..7bded732fd8 --- /dev/null +++ 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/src/dune_rules/melange/melange_rules.ml b/src/dune_rules/melange/melange_rules.ml index 40d05ed960f..4f51081bfb4 100644 --- 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/virtual_lib_compilation.t/dune b/test/blackbox-tests/test-cases/melange/virtual_lib_compilation.t/dune index e7144d1d56c..4a13dc392c0 100644 --- 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 index c25ecb35241..d25c15c88fe 100644 --- 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 index 6aed6d7aada..4b4a8fff4bc 100644 --- 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 From 903a8f39abc02992bc5efa5ec5c81520ab322643 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Fri, 16 Feb 2024 17:11:42 -0800 Subject: [PATCH 2/2] 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 --- Makefile | 15 +++++++++++++-- ci/build-test.sh | 10 ++++++++-- flake.lock | 14 +++++++------- flake.nix | 4 ++-- .../test-cases/melange/private-lib-dep.t | 4 ++-- 5 files changed, 32 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index 3f2aa22e5af..8ea63c0f895 100644 --- 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 index 2f3b18ea48d..9f7fcc0547f 100755 --- 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/flake.lock b/flake.lock index 393b2b7123a..5e5b3107a51 100644 --- a/flake.lock +++ b/flake.lock @@ -30,16 +30,16 @@ ] }, "locked": { - "lastModified": 1707255164, - "narHash": "sha256-eJ2+nbjEGnTinf67lhUPuE8fzfGXeC2ErGRvU2omUgs=", + "lastModified": 1708131076, + "narHash": "sha256-ekKM7U7t3I2WFFxryjib7+IetZuXSuB+fVghghQKSaQ=", "owner": "melange-re", "repo": "melange", - "rev": "71cc9a89b4236cbf02668b21602e86b653b9a6a0", + "rev": "24e21cc4284ffb18b3a856c1d730f06f34d32737", "type": "github" }, "original": { "owner": "melange-re", - "ref": "v3-414", + "ref": "v4-414-dev", "repo": "melange", "type": "github" } @@ -56,11 +56,11 @@ ] }, "locked": { - "lastModified": 1705107784, - "narHash": "sha256-tN+g+m2yXUL5Mik2I0SbMDhhoqatHL/it0VR9cjVYag=", + "lastModified": 1707270552, + "narHash": "sha256-ReZQve1Pl8EHASolPUvD5mUjycxfwaxVdiLRQhHnQIE=", "owner": "melange-re", "repo": "melange-compiler-libs", - "rev": "2c06d6ea9257c52a18cef82f154307eaa2a1bbae", + "rev": "a4edad243a0236a7ece4afc8a6a3ada4b476938a", "type": "github" }, "original": { diff --git a/flake.nix b/flake.nix index 0f749d3cd14..0255a0c0069 100644 --- 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/test/blackbox-tests/test-cases/melange/private-lib-dep.t b/test/blackbox-tests/test-cases/melange/private-lib-dep.t index 18a9e0a5e17..281fa1f2dee 100644 --- 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 */