[esy] Out of source compilation #3856

ulrikstrid · 2019-09-30T07:34:50Z

This is heavily based on #2514 from @bsansouci and the goal is to build everything in a directory that is okay to build into for esy.

This will probably only be a "proof of concept" and I'm going to need help from someone that know more about the internals.

relevant issues: #2418 and #3276

bobzhang · 2019-10-01T00:18:43Z

@ulrikstrid hi, can you write down what directory layout you are trying to achieve

ulrikstrid · 2019-10-01T07:37:08Z

I don't really care where the files are put, I guess passing a flag about where to put the files would be the best since it would then work outside of esy.

My current thought is that I should just put everything under ./lib/... but if you have something else in mind that's probably better! @bobzhang

ulrikstrid · 2019-10-02T15:50:02Z

Now it actually puts the .js files somewhere so it's getting closer. There seems to be some issues with the paths in the generated output and the output is suspiciously empty, but this might be intended because they are 0-cost bindings?

var Ref = { };

var Context = { };

var Fragment = { };

var Suspense = { };

export {
  Ref ,
  Context ,
  Fragment ,
  Suspense ,
  
}
/* No side effect */

ulrikstrid · 2019-10-02T17:33:09Z

There are 2 bigger issues left in this PR

Somewhere in bucklescript there seems to be a assumption that the folder name where the source lives is the package name which is not true with esy, this gives us paths like this:
_esy/default/store/b/reason_sample_project-d306440c/lib/js/reason_react__0.7.0__8e25ac00/src/React.js
There is a assumption that dependencies can be resolved from node_modules so we get javacsript like this:

import * as ReactDOMRe from "reason-react/src/ReactDOMRe.js";

But it should be something like this:

import * as ReactDOMRe from "../lib/js/reason-react/src/ReactDOMRe.js";

When those are solved I need to cleanup the PR from all the extra logging and see how I can remove hard coded assumptions on esys environment.

bsansouci

There are a couple of things that I'd change in there, overall I think it looks pretty good.
The module that's in charge of finding deps seems to already support an env variable: https://github.com/BuckleScript/bucklescript/blob/7cea069a7507f8db510e8e506a7e4c211df1e444/jscomp/bsb/bsb_pkg.ml#L90 (for looking up deps in alternative places).

For the import statements maybe @bobzhang can give better recommendation, but maybe doing some work inside here: https://github.com/BuckleScript/bucklescript/blob/master/jscomp/core/js_packages_info.ml

bsansouci · 2019-10-07T05:02:12Z

jscomp/bsb/bsb_build_util.ml

+  | None -> "not_set" in
+  let () = Bsb_log.info "@{<info>build_artifacts_dir:@} %s @." bad in
+  (* If the project's parent folder is not node_modules, we know it's the top level one. *)
+  if Sys.getenv "cur__root" = cwd then 


I think getenv might throw an exception

Yes, this is currently "safe" because we are probable in the esy environment. But this is mostly a placeholder for something like a flag.

bsansouci · 2019-10-07T05:03:07Z

jscomp/bsb/bsb_clean.ml

@@ -39,7 +39,8 @@ let ninja_clean  proj_dir =
    Bsb_log.warn "@{<warning>ninja clean failed@} : %s @." (Printexc.to_string e)

 let clean_bs_garbage proj_dir =
-  Bsb_log.info "@{<info>Cleaning:@} in %s@." proj_dir ;
+  let proj_dir = Bsb_build_util.get_build_artifacts_location proj_dir in


Since you call this function below, inside clean_bs_deps, you might not need to call it here.

bsansouci · 2019-10-07T05:04:40Z

jscomp/bsb/bsb_ninja_regen.ml

@@ -53,26 +60,32 @@ let regenerate_ninja
  | Other _ -> 
    if check_result = Bsb_bsc_version_mismatch then begin 
      Bsb_log.warn "@{<info>Different compiler version@}: clean current repo@.";
-      Bsb_clean.clean_self  per_proj_dir; 
-    end ; 
+      Bsb_clean.clean_self  build_artifacts_dir; 


Actually since you're passing the right path here, you don't need to call the get_build_artifacts_location inside of it.

bsansouci · 2019-10-07T05:05:17Z

jscomp/bsb/bsb_ninja_regen.ml

+      begin
+        Bsb_log.info "@{<info>toplevel@} %s @." (lib_bs_dir // Literals.sourcedirs_meta);
+        Bsb_watcher_gen.generate_sourcedirs_meta
+          ~name:(lib_bs_dir // Literals.sourcedirs_meta)


this probably needs to be build_artifacts_dir as well

It already is:

let build_artifacts_dir = Bsb_build_util.get_build_artifacts_location per_proj_dir in let lib_bs_dir = build_artifacts_dir // Bsb_config.lib_bs in

bsansouci · 2019-10-07T05:06:31Z

jscomp/bsb/bsb_package_specs.ml

@@ -131,8 +131,7 @@ let package_flag ({format; in_source } : spec) dir =
    (Ext_string.concat3
       (string_of_format format)
       Ext_string.single_colon
-       (if in_source then dir else
-        prefix_of_format format // dir))
+         dir)


I think you still want to support what these lines where doing, insource builds or building inside the prefix-ed sub directory (like es6, commonjs etc...).

bsansouci · 2019-10-07T05:07:34Z

jscomp/main/bsb_main.ml

@@ -160,14 +160,17 @@ let install_target config_opt =

 (* see discussion #929, if we catch the exception, we don't have stacktrace... *)
 let () =
+  let getenv_opt env = try Some(Sys.getenv env)


I'd love it if we could make this a command line arg, like -build-artifacts-dir that esy would automatically pass. It would make the flags reusable for other purposes, and usable from the command line.

I agree, the current solution is mostly to move quickly, but I can add this flag now as well 👍

bobzhang

Can you write down the typical layout under esy and the expected path in JS import statements

bobzhang · 2019-10-12T09:50:13Z

jscomp/bsb/bsb_build_util.ml

-let rec mkp dir = 
+let rec mkp dir =
+Bsb_log.info
+    "@{<info>mkp @} dir: %s @." dir;   


can you comment these lines temporarily to make sure this diff is small

bobzhang · 2019-10-12T09:50:24Z

jscomp/bsb/bsb_build_util.ml

@@ -178,6 +180,7 @@ let rec walk_all_deps_aux
  (top : bool) 
  (dir : string) 
  (cb : package_context -> unit) =
+  let () = Bsb_log.info "@{<info>walk_all_deps_aux dir:@} %s @." dir in


bobzhang · 2019-10-12T09:50:35Z

jscomp/bsb/bsb_build_util.ml

@@ -229,5 +232,31 @@ let rec walk_all_deps_aux


 let walk_all_deps dir cb = 
-  let visited = String_hashtbl.create 0 in 
+  let visited = String_hashtbl.create 0 in
+  Bsb_log.info "@{<info>walk_all_deps dir:@} %s @." dir;


bobzhang · 2019-10-12T09:51:02Z

jscomp/bsb/bsb_build_util.ml

  walk_all_deps_aux visited [] true dir cb 
+
+let build_artifacts_dir = ref None
+


Can you add a type annotation and a one line comment about what it is

bobzhang · 2019-10-12T09:52:59Z

jscomp/bsb/bsb_build_util.ml

+let get_build_artifacts_location cwd =
+  let () = Bsb_log.info "@{<info>get_build_artifacts_location cwd:@} %s @." cwd in
+  let () = Bsb_log.info "@{<info>get_build_artifacts_location cur__root:@} %s @." (Sys.getenv "cur__root") in
+  let bad = match !build_artifacts_dir with


cur_root can be lazily shared

bobzhang · 2019-10-12T09:54:21Z

jscomp/bsb/bsb_build_util.mli

+
+val build_artifacts_dir : (string option) ref
+
+val get_build_artifacts_location : string -> string 


can you add some comments here for easy review

bobzhang · 2020-10-10T02:33:33Z

it seems this PR did not gain traction

Initial work on out of source compilation

202b16d

ulrikstrid mentioned this pull request Oct 2, 2019

[esy] Read dependencies in esy from installation.json #3860

Closed

Actually output .js files

c238620

bsansouci reviewed Oct 7, 2019

View reviewed changes

bobzhang reviewed Oct 12, 2019

View reviewed changes

ulrikstrid mentioned this pull request Nov 6, 2019

Current satus of esy integration #3933

Closed

8 tasks

bobzhang closed this Oct 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[esy] Out of source compilation #3856

[esy] Out of source compilation #3856

ulrikstrid commented Sep 30, 2019 •

edited

Loading

bobzhang commented Oct 1, 2019

ulrikstrid commented Oct 1, 2019

ulrikstrid commented Oct 2, 2019

ulrikstrid commented Oct 2, 2019 •

edited

Loading

bsansouci left a comment

bsansouci Oct 7, 2019

ulrikstrid Oct 7, 2019

bsansouci Oct 7, 2019

bsansouci Oct 7, 2019

bsansouci Oct 7, 2019

ulrikstrid Oct 7, 2019 •

edited

Loading

bsansouci Oct 7, 2019

bsansouci Oct 7, 2019

ulrikstrid Oct 7, 2019

bobzhang left a comment

bobzhang Oct 12, 2019

bobzhang Oct 12, 2019

bobzhang Oct 12, 2019

bobzhang Oct 12, 2019

bobzhang Oct 12, 2019

bobzhang Oct 12, 2019

bobzhang commented Oct 10, 2020

		walk_all_deps_aux visited [] true dir cb

		let build_artifacts_dir = ref None


		val build_artifacts_dir : (string option) ref

		val get_build_artifacts_location : string -> string

[esy] Out of source compilation #3856

[esy] Out of source compilation #3856

Conversation

ulrikstrid commented Sep 30, 2019 • edited Loading

bobzhang commented Oct 1, 2019

ulrikstrid commented Oct 1, 2019

ulrikstrid commented Oct 2, 2019

ulrikstrid commented Oct 2, 2019 • edited Loading

bsansouci left a 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

ulrikstrid Oct 7, 2019 • 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

bobzhang left a 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

bobzhang commented Oct 10, 2020

ulrikstrid commented Sep 30, 2019 •

edited

Loading

ulrikstrid commented Oct 2, 2019 •

edited

Loading

ulrikstrid Oct 7, 2019 •

edited

Loading