feat(melange): install melange libraries #6602

anmonteiro · 2022-11-29T03:27:41Z

First part of feature(melange): installation of libraries #6600:
- Installs melange library artifacts in $LIB_DIR/melange/*.cm{i,j,t}
- resolves libraries with modes melange
- melange.emit emits public library dependency JS to $emit_target/node_modules/lib_name/**/*.js
if refactor(melange): switch --bs-package-name to library name #6841 lands, we need to adapt this PR to install node_modules/<lib_name> rather than node_modules/<pkg-name>
figure out a solution that doesn't use Lib_info.src_dir for public libraries

Signed-off-by: Antonio Nuno Monteiro [email protected]

anmonteiro · 2022-11-29T03:28:20Z

Currently installing melange libraries in LIB_DIR/melange/*.cm{i,j,t}. I'm not sure if this is correct / conflicts with other features. Need some guidance on whether that's what we want

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

…ame/*.js" Signed-off-by: Antonio Nuno Monteiro <[email protected]>

rgrinberg · 2022-12-01T01:14:49Z

src/dune_rules/melange_rules.ml

+  let lib_dir =
+    match Lib.Local.of_lib lib with
+    | None ->
+      let package_name = Option.value_exn (Lib_info.package info) in


Why is value_exn okay here?

I don't know if it is. What would you suggest we do if it's None?

This brings up another design question:

I think I'd rather use the library public_name as the --bs-package-name .. so that we could depend on multiple public libraries within the same package. The naming scheme for those is alphanumerical-maybe-dashes.sublibrary, which I think can be a valid folder name for the node.js module resolution algorithm

What would you suggest we do if it's None?

Work the way it worked before I guess? Right now this will not work for private libraries.

I think I'd rather use the library public_name as the --bs-package-name .. so that we could depend on multiple public libraries within the same package. The naming scheme for those is alphanumerical-maybe-dashes.sublibrary, which I think can be a valid folder name for the node.js module resolution algorithm

Better ask @jchavarri about that.

Why not? The tests are passing. This is in the branch where Lib.Local.of_lib is None. I thought that'd be the branch for external libraries.

I see. But then you're distinguishing between public libraries that are installed and ones that are in the workspace.

I think you should distinguish between public (external or in-workspace) and private libraries instead.

We could also always use Lib.name now that we have the node_modules emission stuff.

There's Lib_info.Status.t. In particular, when it's Private (_, None)

I think I'd rather use the library public_name as the --bs-package-name ..
Better ask @jchavarri about that.

I thought about this and I believe it should be fine. The only caveat is that in order to consume a public library from the same workspace, the library should be installed first, but I guess that's ok? Are workspace libraries "constantly installed" when running e.g. dune build -w?

In particular, when it's Private (_, None)

I am taking a stab at fixing this, but I am confused about this comment. If we want to distinguish between public and private, why does the second part of the payload (None, the package) matter?

I made all tests pass with something like this:

let lib_dir = match Lib_info.status info with | Installed | Installed_private | Private (_, Some _) -> let package_name = Option.value_exn (Lib_info.package info) in let bctx, _ = Path.Build.extract_build_context_dir_exn target_dir in Path.Build.( relative ((relative bctx) "node_modules") (Package.Name.to_string package_name)) | Private (_, None) | Public _ -> let lib = Lib.Local.of_lib_exn lib in let info = Lib.Local.info lib in Lib_info.src_dir info

but it does not make much sense to me.

Public libraries have the following guarantee: their behavior is the same regardless of whether they're installed or are present in the current workspace.

The second part of the payload matters because there's also private libraries with the above guarantee. Those private libraries with the package field set.

rgrinberg · 2022-12-01T01:15:28Z

src/dune_rules/melange_rules.ml

+      let bctx, _ = Path.Build.extract_build_context_dir_exn target_dir in
+      Path.Build.(
+        relative
+          ((relative bctx) "node_modules")


We have Path.Build.L.relative to make this easier to read

Added in anmonteiro@4224f26, can be merged separately or together with anmonteiro#3.

rgrinberg · 2022-12-01T01:18:39Z

test/blackbox-tests/test-cases/melange/emit-installed.t

+  > EOF
+  $ cat > a/dune <<EOF
+  > (library
+  >  (modes melange)


Is there a test that has (modes melange byte)? To make sure they don't step over each other.

I think I have one untracked in git that I can add

rgrinberg · 2022-12-01T01:21:58Z

src/dune_rules/install_rules.ml

+                        , Obj_dir.Module.cmt_file obj_dir m ~ml_kind
+                            ~cm_kind:(Melange Cmi) )
+                    ]
+                    |> List.concat)


Perhaps remove the duplication with:

List.concat_map [ (native || byte), Ocaml Cmi ; melange, Melange Cmi ] ~f:(...

Done in anmonteiro@27905d1.

rgrinberg · 2022-12-01T13:58:39Z

It would also be useful to add a test for private libraries that belong to package. For example:

(library
 (name foo)
 (package bar))

The findlib name for these is mangled.

Signed-off-by: Javier Chavarri <[email protected]>

jchavarri · 2022-12-15T15:50:47Z

The latest from main can be picked up from anmonteiro#3.

Signed-off-by: Javier Chavarri <[email protected]>

jchavarri · 2022-12-15T17:38:32Z

It would also be useful to add a test for private libraries that belong to package.

I added a test for this in anmonteiro@21de584, where there is a melange.emit and library that belong to the same package, but the library does not have public name, as mentioned.

But something fundamental seems to be broken in this case because dune can't find the rules for the cmjs (unless I am doing something wrong on the test setup itself). Still investigating.

Signed-off-by: Javier Chavarri <[email protected]>

jchavarri · 2022-12-16T10:32:16Z

But something fundamental seems to be broken in this case because dune can't find the rules for the cmjs (unless I am doing something wrong on the test setup itself). Still investigating.

Well, this is embarrassing. I was missing the (modes melange) field on the library. I hope we can do something about #6412 🙏 because at this point I think I've spent more than 1 hour in total while debugging different instances of these very confusing "No rule found for foo.cmj" errors 😕

The private lib test seems to be passing (see anmonteiro@0e7225a), so the only 2 things remaining that I am aware of are:

Usage of Lib_info.status: asked about it here
Add a test with (modes melange byte), @anmonteiro you mentioned you had some test around, but I can add it myself otherwise

Signed-off-by: Javier Chavarri <[email protected]>

anmonteiro · 2022-12-17T18:10:24Z

Add a test with (modes melange byte), @anmonteiro you #6602 (comment) you had some test around, but I can add it myself otherwise

Please do, I lost my untracked changes in my other computer.

Signed-off-by: Javier Chavarri <[email protected]>

jchavarri · 2022-12-19T18:00:50Z

Please do, I lost my untracked changes in my other computer.

Done in anmonteiro#5. Added as separate test but can merge into the existing one if it's preferable.

Signed-off-by: Javier Chavarri <[email protected]>

jchavarri · 2022-12-28T15:39:42Z

test/blackbox-tests/test-cases/melange/public.t/my_project/dune

@@ -1,5 +1,4 @@
 (melange.emit
- (package pkg)


We discussed this privately, but leaving here a note for posterity:

The approach I ended up taking is to always put js artifacts of public libs in node_modules, regardless which package or dune project they belong to:

If two public libraries belong to the same package, relative paths like require('../foo/bar.js') will work because the original source layout is honored under the node_modules folder

If two public libs belong to different packages, non-relative paths like require('foo/bar.js') will work because at the top level there will be a node_modules folder that includes foo.

This approach breaks only in one case: when the entries in melange.emit belong to the same package as some public library. I was able to fix this by removing the package entry from melange.emit, so the entries in emit become "private" (in dune's Lib_info.Status.t terms).

Rudi answered:

For executables, we have a package field. But this is mainly useful when you want to:

Install the executable

Include the executable as part of the test suite for the package. so that dune build -p pkg @runtest will see this executable

We probably want to fix this at some point in the future, either by removing package field in melange.emit (at least in the short term), or figuring out how to perform installation of emit entries.

rgrinberg · 2022-12-29T20:16:26Z

src/dune_rules/melange_rules.ml

+      let package_name = Option.value_exn (Lib_info.package info) in
+      let bctx = (Super_context.context sctx).build_dir in
+      Path.Build.L.relative bctx
+        [ "node_modules"; Package.Name.to_string package_name ]


Public is handled differently than | Installed | Installed_private which means that installation is going to be broken. I think for Public libraries you need to remove the src_dir component. If a library is public, the user can't make assumptions as to where its located

Signed-off-by: Javier Chávarri <[email protected]>

…-libs melange: fix install for public libs in different dune projects Signed-off-by: Javier Chavarri <[email protected]>

Signed-off-by: Javier Chávarri <[email protected]>

…tall

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

anmonteiro added the melange Melange rules and generator label Nov 29, 2022

anmonteiro force-pushed the anmonteiro/melange-install branch 3 times, most recently from 485665a to 6c7babd Compare November 30, 2022 18:45

anmonteiro marked this pull request as ready for review November 30, 2022 19:08

anmonteiro force-pushed the anmonteiro/melange-install branch from 6c7babd to f3b69e5 Compare November 30, 2022 19:29

rgrinberg linked an issue Nov 30, 2022 that may be closed by this pull request

feature(melange): installation of libraries #6600

Closed

anmonteiro added 5 commits November 30, 2022 12:33

feat(melange): install melange libraries

fa99710

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

test: dune build @all now works

ff26d4f

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

feat: serialize (modes melange) in dune package

0aa59c8

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

feat: allow depending on melange mode libraries from other libraries

aaaf06a

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

feat: emit public library modules to "$emit_target/node_modules/pkg_n…

7e0cb06

…ame/*.js" Signed-off-by: Antonio Nuno Monteiro <[email protected]>

anmonteiro force-pushed the anmonteiro/melange-install branch from f3b69e5 to 7e0cb06 Compare November 30, 2022 20:34

rgrinberg reviewed Dec 1, 2022

View reviewed changes

Merge branch 'main' into jchavarri/melange-install

f3ed7a8

Signed-off-by: Javier Chavarri <[email protected]>

jchavarri added 3 commits December 15, 2022 15:59

use Path.Build.L.relative

4224f26

Signed-off-by: Javier Chavarri <[email protected]>

simplify cmt_files calculation

27905d1

Signed-off-by: Javier Chavarri <[email protected]>

add test for private libs with package

21de584

Signed-off-by: Javier Chavarri <[email protected]>

fix test for private libs with package

0e7225a

Signed-off-by: Javier Chavarri <[email protected]>

repro bug with 2 dune projects

a936818

Signed-off-by: Javier Chavarri <[email protected]>

melange: fix install for public libs in diff dune projects

cb1d1cc

Signed-off-by: Javier Chavarri <[email protected]>

This was referenced Dec 19, 2022

melange: fix install for public libs in different dune projects anmonteiro/dune#4

Merged

melange: add installation test for two modes anmonteiro/dune#5

Merged

jchavarri added 6 commits December 20, 2022 14:59

melange: improve 2 dune projs test + add complex one to show failure

4457c7d

Signed-off-by: Javier Chavarri <[email protected]>

melange: fix installation in complex cases

f121864

Signed-off-by: Javier Chavarri <[email protected]>

melange: add cyclic dep btw dune-projects

005c41d

Signed-off-by: Javier Chavarri <[email protected]>

melange: update test output

941549e

Signed-off-by: Javier Chavarri <[email protected]>

melange: install public libs always

3be0727

Signed-off-by: Javier Chavarri <[email protected]>

melange: inline local fun

0ebe416

Signed-off-by: Javier Chavarri <[email protected]>

jchavarri force-pushed the anmonteiro/melange-install branch from a8ab73d to 73e66b3 Compare December 28, 2022 10:04

jchavarri requested a review from rgrinberg December 28, 2022 10:55

jchavarri reviewed Dec 28, 2022

View reviewed changes

rgrinberg reviewed Dec 29, 2022

View reviewed changes

jchavarri and others added 4 commits January 6, 2023 14:14

melange: add installation test for two modes (#5)

505d383

Signed-off-by: Javier Chávarri <[email protected]>

Merge pull request #4 from jchavarri/jchavarri/melange-install-public…

a39fa72

…-libs melange: fix install for public libs in different dune projects Signed-off-by: Javier Chavarri <[email protected]>

Merge branch 'main' into anmonteiro/melange-install

4064992

Signed-off-by: Javier Chávarri <[email protected]>

Merge remote-tracking branch 'ocaml/main' into anmonteiro/melange-ins…

ccf117a

…tall

anmonteiro force-pushed the anmonteiro/melange-install branch from e8814b4 to ccf117a Compare January 6, 2023 22:16

promote failing test

78dd6fa

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

anmonteiro force-pushed the anmonteiro/melange-install branch from 7a4cd04 to 78dd6fa Compare January 6, 2023 22:56

chore: remove commented code

97c1bf2

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

jchavarri mentioned this pull request Jan 9, 2023

fix(melange): include_subdirs #6811

Merged

rgrinberg merged commit 7222f67 into ocaml:main Jan 11, 2023

anmonteiro mentioned this pull request Feb 22, 2023

fix(melange): unify public libraries (in-workspace vs external) #7163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(melange): install melange libraries #6602

feat(melange): install melange libraries #6602

anmonteiro commented Nov 29, 2022 •

edited

Loading

anmonteiro commented Nov 29, 2022

rgrinberg Dec 1, 2022

anmonteiro Dec 1, 2022

rgrinberg Dec 1, 2022

anmonteiro Dec 1, 2022

rgrinberg Dec 1, 2022

anmonteiro Dec 1, 2022

rgrinberg Dec 1, 2022

jchavarri Dec 5, 2022

jchavarri Dec 15, 2022 •

edited

Loading

rgrinberg Dec 19, 2022

rgrinberg Dec 1, 2022

jchavarri Dec 15, 2022

rgrinberg Dec 1, 2022

anmonteiro Dec 1, 2022

rgrinberg Dec 1, 2022

jchavarri Dec 15, 2022

rgrinberg commented Dec 1, 2022

jchavarri commented Dec 15, 2022

jchavarri commented Dec 15, 2022

jchavarri commented Dec 16, 2022 •

edited

Loading

anmonteiro commented Dec 17, 2022

jchavarri commented Dec 19, 2022

jchavarri Dec 28, 2022

rgrinberg Dec 29, 2022

feat(melange): install melange libraries #6602

feat(melange): install melange libraries #6602

Conversation

anmonteiro commented Nov 29, 2022 • edited Loading

anmonteiro commented Nov 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchavarri Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Dec 1, 2022

jchavarri commented Dec 15, 2022

jchavarri commented Dec 15, 2022

jchavarri commented Dec 16, 2022 • edited Loading

anmonteiro commented Dec 17, 2022

jchavarri commented Dec 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro commented Nov 29, 2022 •

edited

Loading

jchavarri Dec 15, 2022 •

edited

Loading

jchavarri commented Dec 16, 2022 •

edited

Loading