melange: add `runtime_deps` field to library stanza #6643

jchavarri · 2022-12-06T12:16:18Z

Reuses the rules creation function from copy_rules.

One remaining question is about merlin integration. Dirs from copy_files stanzas are added to src_dirs here:

Lines 123 to 133 in 670b7fb

    
           | Copy_files { files = glob; _ } -> 
        
             let* source_dirs = 
        
               let loc = String_with_vars.loc glob in 
        
               let+ src_glob = Expander.No_deps.expand_str expander glob in 
        
               if Filename.is_relative src_glob then 
        
                 match Path.relative (Path.source src_dir) src_glob ~error_loc:loc with 
        
                 | In_source_tree s -> Some (Path.Source.parent_exn s) 
        
                 | In_build_dir _ | External _ -> None 
        
               else None 
        
             in 
        
             Memo.return { merlin = None; cctx = None; js = None; source_dirs }

However, in Melange case, these dirs will contain mostly assets that are needed at runtime. I can't think of any situations where they'd be needed at build time so I did not add any merlin-related functionality for this runtime_deps case.

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

jchavarri · 2022-12-06T12:17:22Z

src/dune_rules/dir_contents.ml

@@ -131,6 +131,23 @@ end = struct
    let f ~dir = Load.get sctx ~dir >>= artifacts in
    Expander.set_lookup_ml_sources expander ~f

+  let buildable_files (buildable : Buildable.t) =


Nothing changes in this function, just moved from lines 166-179 below, to be able to split the Library and Executable cases.

jchavarri · 2022-12-06T12:18:36Z

src/dune_rules/simple_rules.ml

 let copy_files sctx ~dir ~expander ~src_dir (def : Copy_files.t) =
  Expander.eval_blang expander def.enabled_if >>= function
  | true -> copy_files sctx ~dir ~expander ~src_dir def
  | false -> Memo.return Path.Set.empty

+let extra_files_melange sctx ~dir ~expander ~src_dir files =


I originally tried to create this function inside Melange_rules but got some cyclic dependency error:

Error: dependency cycle between modules in _build/default/src/dune_rules: Virtual_rules -> Dir_contents -> Compilation_context -> Dir_contents

jchavarri · 2022-12-07T15:14:20Z

I just remembered we discussed a couple of things:

@anmonteiro suggested to call the new field runtime_deps instead of extra_files
"Embed" the field inside modes, e.g. (modes (melange (runtime_deps foo.css))), to avoid ambiguous situations like OCaml libraries using the new field.

I will update the PR to apply both changes above, please hold on any reviews until then.

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

anmonteiro · 2022-12-08T04:03:42Z

Could adding extra files in modes conflict with equality in the Ordered Set Lang (#6611)?

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

jchavarri · 2022-12-08T12:26:17Z

Could adding extra files in modes conflict with equality in the Ordered Set Lang (#6611)?

Maybe. In any case, I took a stab at it and ultimately decided to leave the runtime deps as a separate field. Adding it inside modes caused a large amount of changes in Decoder.enum', the way modes is represented as a set later, and how the modes are decoded. In the end, the downsides of having a separate field are not that great: if some library has melange.runtime_deps and does not have melange mode, dune will just ignore the former, so I don't think it's a big deal.

This is ready to review now.

rgrinberg · 2022-12-09T17:30:17Z

suggested to call the new field runtime_deps instead of extra_files

It depends on when are these files being accessed. Are they being accessed at all by the compiler?

rgrinberg · 2022-12-09T17:31:18Z

if some library has melange.runtime_deps and does not have melange mode, dune will just ignore the former, so I don't think it's a big deal.

Seems like it's worth it to add a TODO to make this an error in the future.

jchavarri · 2022-12-12T08:45:39Z

It depends on when are these files being accessed. Are they being accessed at all by the compiler?

These files are fonts, images and / or css stylesheets that are included in the resulting bundle. See for example https://webpack.js.org/guides/asset-management/.

I can't think of any examples where they would be accessed by the compiler.

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

jchavarri · 2022-12-12T09:25:27Z

Seems like it's worth it to add a TODO to make this an error in the future.

I was not sure how the TODO should be added (inline or new issue) so I went ahead and handled the error already as part of this PR: 7fc58cf.

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

rgrinberg · 2022-12-12T15:01:19Z

src/dune_rules/dir_contents.ml

+                        "Library %S is using \"melange.runtime_deps\", but it \
+                         is not a Melange library. To fix this error, you must \
+                         add \"melange\" to \"modes\", or remove \
+                         \"melange.runtime_deps\"."


I just checked the other errors in our code, and we don't quote field names. I'd drop the quotes here for the same reason. By the way, can this validation be moved to Dune_file? We should do it as early as possible.

I don't think there is a lot of consistency, there are cases without quotes, with single quotes and with double quotes.

Happy to unify them in another PR.

For now, removed the quotes in this case and moved to dune_file in 4a7e8b5.

rgrinberg · 2022-12-12T15:13:10Z

src/dune_rules/simple_rules.ml

 let copy_files sctx ~dir ~expander ~src_dir (def : Copy_files.t) =
  Expander.eval_blang expander def.enabled_if >>= function
  | true -> copy_files sctx ~dir ~expander ~src_dir def
  | false -> Memo.return Path.Set.empty

+let melange_runtime_deps sctx ~dir ~expander ~src_dir files =
+  copy_files' sctx ~dir ~expander ~src_dir ~add_line_directive:false ~alias:None


Do you need the glob here? I don't see the glob functionality being tested anywhere.

I think the glob will be very useful to have, yes. I will add some tests to exercise the functionality.

While checking this, I realized a couple of things:

we want to allow multiple globs per field, e.g. (melange.runtime_deps assets/*.png fonts/file.ttf)

the current implementation is broken, the tests were only working by chance

I will update the PR with some improvements and fixes.

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

jchavarri · 2022-12-13T09:34:45Z

@rgrinberg I tried to start adapting implementation to Melange specific case, which differs from original copy_files. In particular:

Honor the relative paths from original sources in the build targets paths
Prepend the target folder to those targets paths

For now I am hard-coding the target value, just to be able to test a simple case. But I am not able to make it work, as I get the following error:

dune/test/blackbox-tests/test-cases/melange/runtime-deps.t

Lines 38 to 43 in afcca3c

    
           $ dune build output/main.js 
        
           Error: Dependency cycle between: 
        
              Computing directory contents of _build/default 
        
           -> Evaluating predicate in directory _build/default/output/assets 
        
           -> Computing directory contents of _build/default 
        
           [1]

The issue seems to happen in this line:

dune/src/dune_rules/simple_rules.ml

Lines 274 to 275 in afcca3c

    
           let* files = 
        
             Build_system.eval_pred (File_selector.of_glob ~dir:src_in_build glob)

I took a look at the eval_pred function, but I don't get how this cycle is introduced. Do you have any ideas?

Also, I think #6541 will heavily affect this PR, so if @anmonteiro is ok, I will probably switch focus to that PR to get it merged before continuing the work here.

jchavarri · 2022-12-13T09:39:24Z

Another realization is that melange.runtime_deps rules behavior will be much more similar to the way Dune copies over source files like .ml files to the build folder. So it might be that my original approach to emulate what is done on Dir_contents for copy_files stanza is not the right way to go, and I should look instead at create_copy_rules as reference. 🤔

* main: (148 commits) refactor(rpc): remove mutable callback (ocaml#6786) refactor(stdune): make [Id.t] immediate (ocaml#6795) refactor(melange): share mode handling (ocaml#6799) refactor(scheduler): remove duplicate types (ocaml#6785) refactor: move cram stanza definition (ocaml#6800) fix: correctly validate argument to top-module (ocaml#6796) refactor: move generate_sites_module to own module (ocaml#6798) fix(melange): check rules (ocaml#6789) refactor(rpc): make [Call.to_dyn] public (ocaml#6797) refactor(rpc): invalid session errors (ocaml#6787) refactor(melange): remove js globs (ocaml#6782) doc: fix version indication for "dune ocaml top-module" (ocaml#6792) refactor: print shutdown exception (ocaml#6784) refactor(rpc): add [Call.to_dyn] (ocaml#6790) fix: do not impose no_sandboxing on ocamldep (ocaml#6749) refactor(melange): move stanza definition (ocaml#6775) fix: handle missing CLICOLOR_FORCE correctly (ocaml#6781) Revert --display tui (ocaml#6780) fix: render pending messages refactor(coq): inline coqc_rule ...

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

jchavarri · 2023-02-07T09:13:00Z

test/blackbox-tests/test-cases/melange/runtime-deps.t

+  Error: Dependency cycle between:
+     Computing directory contents of _build/default
+  -> Evaluating predicate in directory _build/default/output/assets
+  -> Computing directory contents of _build/default
+  [1]


@rgrinberg do you have any ideas how to fix this dep cycle? It seems it is problematic to use globs with melange runtime_deps, should I just skip glob functionality for now?

I managed to get over this error by removing glob functionality in d330036. It is less convenient to not have globs, but if it is going to lead to confusing rule cycles errors, better to leave them out.

However, runtime_deps are still not copied, I believe because the copy rules are there, but nothing depends on the asset files. I wonder though, what exactly should depend on these files? Should these asset paths be passed down and added as dep of the cmj creation rules for each module? Otherwise I am not sure where else they could be added, as there is no such thing as "library-level target" in melange, like cma.

The targets should be added to the alias in the melange.emit stanza I'm thinking

You can use globs as long as you factor things into directories properly. You can also support globs and explicit files with constructors. E.g. (foo.txt (glob_files %{project_root}/*.txt))

The targets should be added to the alias in the melange.emit stanza I'm thinking

Is there a possibility to group all these copy rules under some "internal" alias that melange.emit can use? Duplicating the targets in melange.emit stanzas kind of defeats the point of having runtime_deps field, as it would roughly be the same as using copy_files.

One more thought about this coupling between library assets and emit stanza. In a future, there will be authors that publish Melange libraries. These libraries might need some assets to work properly. The problem as we are discussing is that the copying rules + alias deps are only known at melange.emit stanza time. But this is outside the control of the library author.

I am not sure how to represent this in Dune, but it seems we need some kind of "lazy rule", that contains all the information about the assets copying rules, except the prefix path which is the emit target. Then, at emission time, the rules would be "applied" with this prefix.

cc @anmonteiro

Another option is to "standardize" the output folders as we have discussed at some point in the past, always choosing either es6 or commonjs target folders, depending on module_system. ~~Or just remove target, as was implemented by Antonio in #6541.~~ Edit: removing target wouldn't solve the problem either, as the paths can differ based on where the melange.emit stanza is found.

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

jchavarri · 2023-03-13T21:10:13Z

Superseded by #7234.

melange: add extra_files field to library stanza

654aa02

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

jchavarri added the melange Melange rules and generator label Dec 6, 2022

jchavarri commented Dec 6, 2022

View reviewed changes

jchavarri changed the title ~~melange: add extra_files field to library stanza~~ melange: add extra_files field to library stanza Dec 6, 2022

Merge branch 'main' into melange/extra-files

c1b3f56

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

melange: rename extra_files to runtime_deps

485a40f

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

jchavarri changed the title ~~melange: add extra_files field to library stanza~~ melange: add runtime_deps field to library stanza Dec 8, 2022

jchavarri added 2 commits December 12, 2022 08:58

Merge branch 'main' into melange/extra-files

c06b646

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

handle error when not melange lib

7fc58cf

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

jchavarri requested a review from rgrinberg December 12, 2022 11:47

rename extra_files_melange to melange_runtime_deps

c016117

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

rgrinberg reviewed Dec 12, 2022

View reviewed changes

jchavarri added 3 commits December 12, 2022 16:47

move runtime_deps error to dune_file

4a7e8b5

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

failed attempt to copy files to target folder

b801c9d

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

include error on test

afcca3c

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

jchavarri added 4 commits December 29, 2022 13:43

melange: fix melange stanza

3b436a0

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

Merge branch 'main' into melange/extra-files

2f5dd25

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

melange: update runtime deps test

9308dad

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

jchavarri commented Feb 7, 2023

View reviewed changes

jchavarri mentioned this pull request Feb 7, 2023

melange: allow to define copying assets rules from the libraries #7013

Closed

jchavarri added 2 commits February 7, 2023 14:40

melange: don't use globs for now

d330036

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

melange: properly build build paths

a9de0e1

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

jchavarri mentioned this pull request Mar 2, 2023

feat(melange): add runtime_deps to copy assets to the target dir #7199

Merged

jchavarri closed this Mar 13, 2023

jchavarri deleted the melange/extra-files branch March 13, 2023 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

melange: add `runtime_deps` field to library stanza #6643

melange: add `runtime_deps` field to library stanza #6643

jchavarri commented Dec 6, 2022 •

edited

Loading

jchavarri Dec 6, 2022 •

edited

Loading

jchavarri Dec 6, 2022

jchavarri commented Dec 7, 2022

anmonteiro commented Dec 8, 2022

jchavarri commented Dec 8, 2022

rgrinberg commented Dec 9, 2022

rgrinberg commented Dec 9, 2022

jchavarri commented Dec 12, 2022

jchavarri commented Dec 12, 2022

rgrinberg Dec 12, 2022

jchavarri Dec 12, 2022

rgrinberg Dec 12, 2022

jchavarri Dec 12, 2022 •

edited

Loading

jchavarri commented Dec 13, 2022 •

edited

Loading

jchavarri commented Dec 13, 2022 •

edited

Loading

jchavarri Feb 7, 2023

jchavarri Feb 7, 2023 •

edited

Loading

rgrinberg Feb 7, 2023

jchavarri Feb 8, 2023

jchavarri Feb 8, 2023

jchavarri Feb 8, 2023 •

edited

Loading

jchavarri commented Mar 13, 2023

	\| Copy_files { files = glob; _ } ->
	let* source_dirs =
	let loc = String_with_vars.loc glob in
	let+ src_glob = Expander.No_deps.expand_str expander glob in
	if Filename.is_relative src_glob then
	match Path.relative (Path.source src_dir) src_glob ~error_loc:loc with
	\| In_source_tree s -> Some (Path.Source.parent_exn s)
	\| In_build_dir _ \| External _ -> None
	else None
	in
	Memo.return { merlin = None; cctx = None; js = None; source_dirs }

melange: add runtime_deps field to library stanza #6643

melange: add runtime_deps field to library stanza #6643

Conversation

jchavarri commented Dec 6, 2022 • edited Loading

jchavarri Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchavarri commented Dec 7, 2022

anmonteiro commented Dec 8, 2022

jchavarri commented Dec 8, 2022

rgrinberg commented Dec 9, 2022

rgrinberg commented Dec 9, 2022

jchavarri commented Dec 12, 2022

jchavarri commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchavarri Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

jchavarri commented Dec 13, 2022 • edited Loading

jchavarri commented Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

jchavarri Feb 7, 2023 • 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

jchavarri Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

jchavarri commented Mar 13, 2023

melange: add `runtime_deps` field to library stanza #6643

melange: add `runtime_deps` field to library stanza #6643

jchavarri commented Dec 6, 2022 •

edited

Loading

jchavarri Dec 6, 2022 •

edited

Loading

jchavarri Dec 12, 2022 •

edited

Loading

jchavarri commented Dec 13, 2022 •

edited

Loading

jchavarri commented Dec 13, 2022 •

edited

Loading

jchavarri Feb 7, 2023 •

edited

Loading

jchavarri Feb 8, 2023 •

edited

Loading