-
Notifications
You must be signed in to change notification settings - Fork 469
[esy] Out of source compilation #3856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,7 +124,9 @@ let resolve_bsb_magic_file ~cwd ~desc p : result = | |
mkp "/a/b/c/d" | ||
]} | ||
*) | ||
let rec mkp dir = | ||
let rec mkp dir = | ||
Bsb_log.info | ||
"@{<info>mkp @} dir: %s @." dir; | ||
if not (Sys.file_exists dir) then | ||
let parent_dir = Filename.dirname dir in | ||
if parent_dir = Filename.current_dir_name then | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
let bsconfig_json = dir // Literals.bsconfig_json in | ||
match Ext_json_parse.parse_json_from_file bsconfig_json with | ||
| Obj {map; loc} -> | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
walk_all_deps_aux visited [] true dir cb | ||
|
||
let build_artifacts_dir = ref None | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a type annotation and a one line comment about what it is |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| Some(x) -> x | ||
| 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think getenv might throw an exception There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is currently "safe" because we are probable in the |
||
match !build_artifacts_dir with | ||
| None -> cwd | ||
| Some dir -> dir | ||
else begin | ||
match !build_artifacts_dir with | ||
| None -> | ||
cwd | ||
(* (Filename.dirname (Filename.dirname cwd)) // Bsb_config.lib_lit // Bsb_config.node_modules // project_name *) | ||
| Some dir -> | ||
let project_name = Filename.basename cwd in | ||
let () = Bsb_log.info "@{<info>build_artifacts_location:@} %s @." (dir // Bsb_config.lib_js // project_name) in | ||
dir // Bsb_config.lib_js // project_name | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,3 +96,7 @@ type package_context = { | |
} | ||
|
||
val walk_all_deps : string -> (package_context -> unit) -> unit | ||
|
||
val build_artifacts_dir : (string option) ref | ||
|
||
val get_build_artifacts_location : string -> string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add some comments here for easy review |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you call this function below, inside |
||
let () = Bsb_log.info "@{<info>Cleaning:@} in %s@." proj_dir in | ||
let try_remove x = | ||
let x = proj_dir // x in | ||
if Sys.file_exists x then | ||
|
@@ -56,8 +57,7 @@ let clean_bs_garbage proj_dir = | |
let clean_bs_deps proj_dir = | ||
Bsb_build_util.walk_all_deps proj_dir (fun pkg_cxt -> | ||
(* whether top or not always do the cleaning *) | ||
clean_bs_garbage pkg_cxt.proj_dir | ||
clean_bs_garbage (Bsb_build_util.get_build_artifacts_location proj_dir) | ||
) | ||
|
||
let clean_self proj_dir = | ||
clean_bs_garbage proj_dir | ||
let clean_self proj_dir = clean_bs_garbage (Bsb_build_util.get_build_artifacts_location proj_dir) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,9 +34,16 @@ let regenerate_ninja | |
~(toplevel_package_specs : Bsb_package_specs.t option) | ||
~forced ~per_proj_dir | ||
: Bsb_config_types.t option = | ||
let build_artifacts_dir = Bsb_build_util.get_build_artifacts_location per_proj_dir in | ||
let toplevel = toplevel_package_specs = None in | ||
let lib_bs_dir = per_proj_dir // Bsb_config.lib_bs in | ||
let lib_bs_dir = build_artifacts_dir // Bsb_config.lib_bs in | ||
let output_deps = lib_bs_dir // bsdeps in | ||
let () = Bsb_log.info | ||
"@{<info>regenerate_ninja@} per_proj_dir : %s @." per_proj_dir in | ||
let () = Bsb_log.info | ||
"@{<info>regenerate_ninja@} lib_bs_dir : %s @." lib_bs_dir in | ||
let () = Bsb_log.info | ||
"@{<info>regenerate_ninja@} output_deps : %s @." output_deps in | ||
let check_result = | ||
Bsb_ninja_check.check | ||
~per_proj_dir:per_proj_dir | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually since you're passing the right path here, you don't need to call the |
||
end ; | ||
|
||
let config = | ||
Bsb_config_parse.interpret_json | ||
~toplevel_package_specs | ||
~per_proj_dir in | ||
|
||
(* create directory, lib/bs, lib/js, lib/es6 etc *) | ||
Bsb_build_util.mkp lib_bs_dir; | ||
Bsb_build_util.mkp build_artifacts_dir; | ||
|
||
Bsb_package_specs.list_dirs_by config.package_specs | ||
(fun x -> | ||
let dir = per_proj_dir // x in (*Unix.EEXIST error*) | ||
(fun x -> | ||
let dir = build_artifacts_dir // x in (*Unix.EEXIST error*) | ||
if not (Sys.file_exists dir) then Unix.mkdir dir 0o777); | ||
if toplevel then | ||
Bsb_watcher_gen.generate_sourcedirs_meta | ||
~name:(lib_bs_dir // Literals.sourcedirs_meta) | ||
config.file_groups | ||
if toplevel then | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this probably needs to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
config.file_groups | ||
end | ||
; | ||
Bsb_merlin_gen.merlin_file_gen ~per_proj_dir | ||
(Bsb_global_paths.vendor_bsppx) config; | ||
|
||
(*Bsb_merlin_gen.merlin_file_gen ~per_proj_dir | ||
(Bsb_global_paths.vendor_bsppx) config;*) | ||
Bsb_ninja_gen.output_ninja_and_namespace_map | ||
~per_proj_dir ~toplevel config ; | ||
|
||
|
@@ -81,5 +94,3 @@ let regenerate_ninja | |
Bsb_ninja_check.record ~per_proj_dir ~file:output_deps | ||
(Literals.bsconfig_json::config.file_groups.globbed_dirs) ; | ||
Some config | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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...). |
||
|
||
let package_flag_of_package_specs (package_specs : t) | ||
(dirname : string ) : string = | ||
|
@@ -161,7 +160,7 @@ let get_list_of_output_js | |
output_file_sans_extension | ||
(if bs_suffix then Literals.suffix_bs_js else Literals.suffix_js) | ||
in | ||
(Bsb_config.proj_rel @@ (if spec.in_source then basename | ||
(Bsb_config.build_artifacts_dir @@ (if spec.in_source then basename | ||
else prefix_of_format spec.format // basename)) | ||
:: acc | ||
) package_specs [] | ||
|
@@ -174,4 +173,4 @@ let list_dirs_by | |
Spec_set.iter (fun (spec : spec) -> | ||
if not spec.in_source then | ||
f (prefix_of_format spec.format) | ||
) package_specs | ||
) package_specs |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -501,4 +501,3 @@ let clean_re_js root = | |
end | ||
| _ -> () | ||
| exception _ -> () | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,22 +89,22 @@ let exec_command_then_exit command = | |
exit (Sys.command command ) | ||
|
||
(* Execute the underlying ninja build call, then exit (as opposed to keep watching) *) | ||
let ninja_command_exit ninja_args = | ||
let ninja_command_exit cwd ninja_args = | ||
let ninja_args_len = Array.length ninja_args in | ||
if Ext_sys.is_windows_or_cygwin then | ||
let path_ninja = Filename.quote Bsb_global_paths.vendor_ninja in | ||
exec_command_then_exit | ||
(if ninja_args_len = 0 then | ||
Ext_string.inter3 | ||
path_ninja "-C" Bsb_config.lib_bs | ||
path_ninja "-C" (cwd // Bsb_config.lib_bs) | ||
else | ||
let args = | ||
Array.append | ||
[| path_ninja ; "-C"; Bsb_config.lib_bs|] | ||
[| path_ninja ; "-C"; cwd // Bsb_config.lib_bs|] | ||
ninja_args in | ||
Ext_string.concat_array Ext_string.single_space args) | ||
else | ||
let ninja_common_args = [|"ninja.exe"; "-C"; Bsb_config.lib_bs |] in | ||
let ninja_common_args = [|"ninja.exe"; "-C"; cwd // Bsb_config.lib_bs |] in | ||
let args = | ||
if ninja_args_len = 0 then ninja_common_args else | ||
Array.append ninja_common_args ninja_args in | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd love it if we could make this a command line arg, like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, the current solution is mostly to move quickly, but I can add this flag now as well 👍 |
||
with | Not_found -> None in | ||
let () = Bsb_build_util.build_artifacts_dir := getenv_opt "cur__target_dir" in | ||
try begin | ||
match Sys.argv with | ||
| [| _ |] -> (* specialize this path [bsb.exe] which is used in watcher *) | ||
Bsb_ninja_regen.regenerate_ninja | ||
~toplevel_package_specs:None | ||
~forced:false | ||
~per_proj_dir:Bsb_global_paths.cwd |> ignore; | ||
ninja_command_exit [||] | ||
ninja_command_exit (Bsb_build_util.get_build_artifacts_location Bsb_global_paths.cwd) [||] | ||
|
||
| argv -> | ||
begin | ||
|
@@ -208,7 +211,7 @@ let () = | |
[bsb -regen ] | ||
*) | ||
end else if make_world then begin | ||
ninja_command_exit [||] | ||
ninja_command_exit (Bsb_build_util.get_build_artifacts_location Bsb_global_paths.cwd) [||] | ||
end else if do_install then begin | ||
install_target config_opt | ||
end) | ||
|
@@ -228,7 +231,7 @@ let () = | |
if !do_install then | ||
install_target config_opt; | ||
if !watch_mode then program_exit () | ||
else ninja_command_exit ninja_args | ||
else ninja_command_exit (Bsb_build_util.get_build_artifacts_location Bsb_global_paths.cwd) ninja_args | ||
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you comment these lines temporarily to make sure this diff is small