Minimal native support #3762

bsansouci · 2019-08-18T05:56:13Z

Hey @bobzhang !
I realized that it'd probably better to make a PR with a minimal support of native compilation, rather than go down the rabbit hole of making Belt work, mostly because a minimal native support is already fun and useful.
It might look scary, but most changes are hidden under #if BS_NATIVE then. The only changes that aren't hidden under that flag are done to help make the diff more readable, and don't cause a semantic change. For example I pass the backend arg to all functions, which defaults to Js, but it's not used by any function when compiled with BS_NATIVE=false.
For native compilation, I have to nest the build artifacts under lib/bs/js / lib/bs/bytecode / lib/bs/native to make sure the files didn't step on each other (like the cmi which aren't the same between JS and native/bytecode). That nesting is also compiled away for bsb, of course, but I do pass nested (the directory name) to the functions that need them.

Here is a list of things that PR does.

Add functions to access the ocaml stdlib vendored in bucklescript (Bsb_build_util)
Add some custom exceptions for native compilation.
Add -bs-binary-simple-ast flag to bsc, which generates a simple AST file (which ocamlc / ocamlopt understands)
Fix the .merlin file generation
Adds BS_NATIVE_PPX to differentiate between the part of the ppx used
inside bsc and the standalone ppx used by bsb-native for compiling
belt to native.
Make Bsb_ninja_check also check if the -backend argument changed to know to rebuild or not
Fixed old merged BS_NATIVE stuff (including bsb_helper)
Pass backend to all the functions, but compile out the -backend flag
Add a whole new file called bsb_ninja_native which handles outputting the ninja file for building, linking and packing. Super similar to Bsb_ninja_file_groups.
Modified Bsb_ninja_gen to add the global variables we need for native compilation and call the new Bsb_ninja_native.handle_file_groups function.
Make Bsb_world collect a list of the dependencies in topological order (since we're already iterating over them).
Simplify bsb_helper_depfile_gen
Remove -bs-super-errors from linker/packer for now (that requires changing the compiler to accept that flag and enable support error)
Make Ext_bytes.escape also escape $, I think because we might pass arguments that contain those characters.
Update scripts/ninja.js to take a -native flag to generate the ninja files with BS_NATIVE=true. This allows me to build the whole thing running ninja.js config -native && ninja.js build. Making a release will require a couple more changes but those can be done in a subsequent PR.

bobzhang · 2019-08-19T01:24:42Z

hi @bassjacob thanks for keeping working on this, I will do the review and get back to you asap

bobzhang

Hi, I made some suggestions, the general idea is that you can make use lazy evaluation to reduce the changes as much as possible (so you don't need pass more arguments), it is also more efficient

jscomp/bsb/bsb_build_util.ml

bobzhang · 2019-08-19T03:03:52Z

jscomp/bsb/bsb_config_parse.ml

+  let json = Ext_json_parse.parse_json_from_file Literals.bsconfig_json in
+  match json with
+    | Obj {map} -> extract_main_entries map
+    | _ -> assert false



The json file is parsed several time, shall we add a lazy field to avoid parsing it several times like

let json_lazy = lazy (Ext_json_parse.parse_json_... )

I like the idea! But it seems like it currently depends on cwd, do you think we can drop that dependency?
So instead of Ext_json_parse.parse_json_from_file (cwd // Literals.bsconfig_json) we would do Ext_json_parse.parse_json_from_file Literals.bsconfig_json.

bobzhang · 2019-08-19T03:07:50Z

jscomp/bsb/bsb_merlin_gen.ml

@@ -86,18 +98,27 @@ let output_merlin_namespace buffer ns=
    Buffer.add_string buffer "-open ";
    Buffer.add_string buffer x 

+#if BS_NATIVE then
+let bsc_flg_to_merlin_ocamlc_flg bsc_flags backend  =
+#else


could backend be made a lazy value so that the patch does not need be intrusive (the argument does not need be passed across each function), so is nest argument

let backend = lazy (...)

Are you saying that it should be in some module that I can access like Lazy.force Bsb_args.backend, or something like that, so that everywhere that needs backend would read it from there?
One issue I see is that it's more than just a value because it depends on reading the bsconfig, so it depends on cwd. Similar to my previous comment above, do you think we should just get rid of passing cwd down or even just use Sys.getcwd () instead?

bobzhang · 2019-08-19T03:09:15Z

jscomp/bsb/bsb_merlin_gen.ml

+      | Bsb_config_types.Native   -> Literals.native
+      | Bsb_config_types.Bytecode -> Literals.bytecode
+    in
+#end


This is the same thing, we can push it into lazy evaluation since it only needs to be evaluated once during the lifetime of such process

Is that really an optimization that is worth it? I didn't think that the matching would be worse than calling Lazy.force in a bunch of places.

Now that it doesn't take any args and it still has a side effect, do you think I should use lazy? It might make sense, I'm just scared of having the issue of calling this too early, because the backend is parsed and be stuck with that wrong value.

bobzhang · 2019-08-19T03:14:25Z

jscomp/bsb/bsb_ninja_check.ml

@@ -28,6 +28,7 @@ type t =
    dir_or_files : string array ;
    st_mtimes : float array;
    source_directory :  string ;    
+    cmdline_backend: Bsb_config_types.compilation_kind_t;
  }



maybe we don't need add a new field, we can just change the name of file, for js backend its name is xx, for native backend its name is yy so they don't conflict with each other

I am thinking to reduce such conflicts even for ninja file name, they can pick different names so less conflict is introduced, then you can call ninja -f xx.ninja

Oh interesting! I can append _js, _bytecode and _native to the file name, no problem.
I don't think we need to do the ninja file name because I have to put the build artifacts in different folders anyway!

bobzhang · 2019-08-19T03:17:10Z

jscomp/bsb/bsb_ninja_rule.ml

@@ -170,16 +188,19 @@ let make_custom_rules
    );
    if has_ppx then 
      Buffer.add_string buf " $ppx_flags"; 
+    if gen_simple then
+      Buffer.add_string buf " -bs-simple-binary-ast";
    Buffer.add_string buf " $bsc_flags -o $out -bs-syntax-only -bs-binary-ast $in";   
    Buffer.contents buf


how do you generate .d file in this case?

Can you explain a bit what you mean?
The .d file handling's done in bsb_helper_depfile_gen. I don't see what this change relates to .d file generation.

bobzhang · 2019-08-19T03:20:09Z

jscomp/bsb_helper/bsb_helper_packer.ml

    (fun acc v -> match String_map.find_opt module_to_filepath v with
      | Some file -> (file ^ suffix_object_files) :: acc
      | None -> failwith @@ "build.ninja is missing the file '" ^ v ^ "' that was used in the project. Try force-regenerating but this shouldn't happen."
      )
    []
    sorted_tasks in
+  let warning_command = if String.length warnings > 0 then


note to help me review it easier, the changes to bsb_native alone could be sent separately and merged quickly

Did you mean to bsb_helper?

bobzhang · 2019-08-19T03:26:40Z

jscomp/ext/ext_bytes.ml

+#if BS_NATIVE then
+      | '$' ->
+           Bytes.unsafe_set s' !n '$'; incr n; Bytes.unsafe_set s' !n '$'
+#end
      | '\n' ->


where is this function used in bs_native? The escaping rules does not seem correct to me

Oh I meant to escape : I think. Because ninja doesn't like : and bsb_native generates some windows path that are absolute so C:\something and ninja dies on those.

What's not correct?

If you read the docs of {String.ecaped}, it escape the special character following ocaml lexical convention which is not relevant to windows path convention, I guess you need a special function for windows path escaping

bobzhang · 2019-08-19T03:31:56Z

scripts/ninja.js

@@ -1588,7 +1589,7 @@ function nativeNinja() {
  var templateNative = `
 subninja ${getPreprocessorFileName()}
 rule optc
-    command = $ocamlopt -safe-string -I +compiler-libs -opaque ${includes} -g -w +6-40-30-23 -warn-error +a-40-30-23 -absname -c $in
+    command = BS_NATIVE=${BS_NATIVE} $ocamlopt -safe-string -I +compiler-libs -opaque ${includes} -g -w +6-40-30-23 -warn-error +a-40-30-23 -absname -c $in
    description = $out : $in
 rule archive


where is this env variable read?

This is read by the native compiler! It uses env vars for the #if BS_NATIVE macros.

bsansouci

Pushed some fixes and added a bunch of questions!

jscomp/bsb/bsb_build_util.ml

bsansouci · 2019-08-20T03:39:00Z

jscomp/bsb/bsb_config_parse.ml

+  let json = Ext_json_parse.parse_json_from_file Literals.bsconfig_json in
+  match json with
+    | Obj {map} -> extract_main_entries map
+    | _ -> assert false



I like the idea! But it seems like it currently depends on cwd, do you think we can drop that dependency?
So instead of Ext_json_parse.parse_json_from_file (cwd // Literals.bsconfig_json) we would do Ext_json_parse.parse_json_from_file Literals.bsconfig_json.

bsansouci · 2019-08-20T03:44:49Z

jscomp/bsb/bsb_merlin_gen.ml

@@ -86,18 +98,27 @@ let output_merlin_namespace buffer ns=
    Buffer.add_string buffer "-open ";
    Buffer.add_string buffer x 

+#if BS_NATIVE then
+let bsc_flg_to_merlin_ocamlc_flg bsc_flags backend  =
+#else


Are you saying that it should be in some module that I can access like Lazy.force Bsb_args.backend, or something like that, so that everywhere that needs backend would read it from there?
One issue I see is that it's more than just a value because it depends on reading the bsconfig, so it depends on cwd. Similar to my previous comment above, do you think we should just get rid of passing cwd down or even just use Sys.getcwd () instead?

bsansouci · 2019-08-20T03:47:45Z

jscomp/bsb/bsb_ninja_check.ml

@@ -28,6 +28,7 @@ type t =
    dir_or_files : string array ;
    st_mtimes : float array;
    source_directory :  string ;    
+    cmdline_backend: Bsb_config_types.compilation_kind_t;
  }



Oh interesting! I can append _js, _bytecode and _native to the file name, no problem.
I don't think we need to do the ninja file name because I have to put the build artifacts in different folders anyway!

bsansouci · 2019-08-20T03:54:21Z

jscomp/bsb/bsb_ninja_rule.ml

@@ -170,16 +188,19 @@ let make_custom_rules
    );
    if has_ppx then 
      Buffer.add_string buf " $ppx_flags"; 
+    if gen_simple then
+      Buffer.add_string buf " -bs-simple-binary-ast";
    Buffer.add_string buf " $bsc_flags -o $out -bs-syntax-only -bs-binary-ast $in";   
    Buffer.contents buf


Can you explain a bit what you mean?
The .d file handling's done in bsb_helper_depfile_gen. I don't see what this change relates to .d file generation.

bsansouci · 2019-08-20T03:54:59Z

jscomp/bsb_helper/bsb_helper_packer.ml

    (fun acc v -> match String_map.find_opt module_to_filepath v with
      | Some file -> (file ^ suffix_object_files) :: acc
      | None -> failwith @@ "build.ninja is missing the file '" ^ v ^ "' that was used in the project. Try force-regenerating but this shouldn't happen."
      )
    []
    sorted_tasks in
+  let warning_command = if String.length warnings > 0 then


Did you mean to bsb_helper?

bsansouci · 2019-08-20T04:00:22Z

jscomp/ext/ext_bytes.ml

+#if BS_NATIVE then
+      | '$' ->
+           Bytes.unsafe_set s' !n '$'; incr n; Bytes.unsafe_set s' !n '$'
+#end
      | '\n' ->


Oh I meant to escape : I think. Because ninja doesn't like : and bsb_native generates some windows path that are absolute so C:\something and ninja dies on those.

What's not correct?

bsansouci · 2019-08-20T04:01:43Z

scripts/ninja.js

@@ -1588,7 +1589,7 @@ function nativeNinja() {
  var templateNative = `
 subninja ${getPreprocessorFileName()}
 rule optc
-    command = $ocamlopt -safe-string -I +compiler-libs -opaque ${includes} -g -w +6-40-30-23 -warn-error +a-40-30-23 -absname -c $in
+    command = BS_NATIVE=${BS_NATIVE} $ocamlopt -safe-string -I +compiler-libs -opaque ${includes} -g -w +6-40-30-23 -warn-error +a-40-30-23 -absname -c $in
    description = $out : $in
 rule archive


This is read by the native compiler! It uses env vars for the #if BS_NATIVE macros.

bsansouci · 2019-08-21T05:50:59Z

jscomp/bsb/bsb_merlin_gen.ml

+      | Bsb_config_types.Native   -> Literals.native
+      | Bsb_config_types.Bytecode -> Literals.bytecode
+    in
+#end


Is that really an optimization that is worth it? I didn't think that the matching would be worse than calling Lazy.force in a bunch of places.

bsansouci · 2019-09-01T23:55:52Z

Friendly ping @bobzhang :)

bobzhang · 2019-09-02T02:43:19Z

Friendly ping @bobzhang :)

Looking into it

bobzhang

hi @bsansouci I would like to merge you patch to unblock your work, but the diff is so big that is really hard to do a review.
I made a diff #3798 (hopefully to help reduce your changes)

bobzhang · 2019-09-02T07:53:53Z

jscomp/bsb/bsb_build_util.ml

+let get_ocaml_lib_dir ~is_js ~cwd =
+  if is_js then (Filename.dirname (get_bsc_dir ~cwd)) // "lib" // "ocaml"
+  else (get_ocaml_dir ~cwd) // "lib" // "ocaml"
+#end


see Bsb_global_paths module so that you don't need function here.
Also I see is_js always passed false here, why do we need a parameter for it?

I changed this to be inside Bsb_global_paths! Thanks

bobzhang · 2019-09-02T08:00:19Z

jscomp/bsb/bsb_ninja_check.ml

      | Other s -> s)

-let rec check_aux cwd (xs : string array) (ys: float array) i finish =
+let rec check_aux cwd ~nested (xs : string array) (ys: float array) i finish =
  if i = finish then Good


what's nested? can we make the diff as small as possible

Good catch, that was very useless. I used your change to make this less intrusive (I think) by ~200 lines :D

bsansouci · 2019-09-06T03:01:15Z

jscomp/bsb_helper/bsb_helper_depfile_gen.ml

-      | None -> 
-        raise (Arg.Bad ("don't know what to do with  " ^ fn))
-    end
-#end


This is part of my cleanup of old broken stuff.

bsansouci · 2019-09-06T03:07:43Z

Updated the diff! It's quite a bit smaller thanks for that!
I added Bsb_global_backend which encapsulates what we talked about. Tell me what you think.

And this sets us better for rescript-lang#3762 :) This shouldn't affect bsb / bsc, everything's compiled away.

bobzhang · 2019-10-28T02:20:49Z

jscomp/common/js_config.mli

@@ -89,6 +89,9 @@ val sort_imports : bool ref
 val syntax_only  : bool ref
 val binary_ast : bool ref

+#if BS_NATIVE then
+val simple_binary_ast : bool ref
+#end

 val bs_suffix : bool ref
 val debug : bool ref


This does not have to be on conditional compilation, we can support it natively in a separate PR

bobzhang · 2019-10-28T02:21:03Z

jscomp/core/js_implementation.ml

+     let oc = open_out_bin (outputprefix ^ Literals.suffix_mliast_simple) in 
+     Ml_binary.write_ast Mli !Location.input_name ast oc;
+     close_out oc ;
+   end;


same as above

bobzhang · 2019-10-28T02:30:36Z

jscomp/ext/ext_bytes.ml

+#if BS_NATIVE then
+      | '$' ->
+           Bytes.unsafe_set s' !n '$'; incr n; Bytes.unsafe_set s' !n '$'
+#end
      | '\n' ->


If you read the docs of {String.ecaped}, it escape the special character following ocaml lexical convention which is not relevant to windows path convention, I guess you need a special function for windows path escaping

bobzhang · 2019-10-28T02:32:12Z

jscomp/syntax/native_ast_derive_abstract.ml

+  | Asttypes.Nolabel -> assert false
+  | Optional s
+  | Labelled s -> s
+#else
   if arg_name.[0] = '?' then
     String.sub arg_name 1 (String.length arg_name - 1)
   else arg_name


can you make it a separate PR

Make it build in 4.06! Fixes to make sure this stuff compiles

bsansouci · 2020-03-23T20:21:40Z

Closing! Working on a simpler approach.

bobzhang reviewed Aug 19, 2019

View reviewed changes

bsansouci commented Aug 21, 2019

View reviewed changes

bobzhang reviewed Sep 2, 2019

View reviewed changes

bsansouci force-pushed the minimal-native-support branch 2 times, most recently from dc9c324 to 3dd2db1 Compare September 6, 2019 02:34

bsansouci commented Sep 6, 2019

View reviewed changes

bsansouci force-pushed the minimal-native-support branch 2 times, most recently from fc91112 to eb81d9f Compare September 6, 2019 03:07

bsansouci force-pushed the minimal-native-support branch from eb81d9f to 27bcfa4 Compare October 2, 2019 01:29

bsansouci added a commit to bsansouci/bucklescript that referenced this pull request Oct 2, 2019

Fix existing code compiled away with BS_NATIVE

c6a41f7

And this sets us better for rescript-lang#3762 :) This shouldn't affect bsb / bsc, everything's compiled away.

bsansouci mentioned this pull request Oct 2, 2019

Fix existing code compiled away with BS_NATIVE #3859

Merged

bsansouci force-pushed the minimal-native-support branch from 27bcfa4 to 2c86556 Compare October 6, 2019 03:29

bsansouci force-pushed the minimal-native-support branch 2 times, most recently from 01f0b36 to a1fb3e3 Compare October 26, 2019 06:07

bobzhang reviewed Oct 28, 2019

View reviewed changes

bsansouci force-pushed the minimal-native-support branch from a1fb3e3 to ed665ef Compare December 17, 2019 06:32

This was referenced Dec 17, 2019

Add -bs-simple-binary-ast #4054

Merged

Fix native ast_deriving for 4.06 #4055

Merged

bsansouci force-pushed the minimal-native-support branch from 9049274 to 80310a3 Compare December 17, 2019 07:31

bsansouci force-pushed the minimal-native-support branch from 80310a3 to 5ff554a Compare January 12, 2020 02:16

bsansouci mentioned this pull request Jan 12, 2020

Small changes to internals of bsb for native compilation #4086

Merged

bsansouci force-pushed the minimal-native-support branch 2 times, most recently from 76e79d8 to 97e6e2f Compare February 6, 2020 09:24

Schmavery mentioned this pull request Feb 24, 2020

Conflict with global bs-platform bsansouci/reprocessing-example#24

Open

Fix compiling issues

ef9179f

Make it build in 4.06! Fixes to make sure this stuff compiles

bsansouci force-pushed the minimal-native-support branch from 97e6e2f to ef9179f Compare February 26, 2020 18:05

bsansouci closed this Mar 23, 2020

Minimal native support #3762

Minimal native support #3762

Conversation

bsansouci commented Aug 18, 2019 • edited

bobzhang commented Aug 19, 2019

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsansouci commented Sep 1, 2019

bobzhang commented Sep 2, 2019

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

bsansouci commented Sep 6, 2019

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

bsansouci commented Mar 23, 2020

bsansouci commented Aug 18, 2019 •

edited