Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Feb 23, 2023
+32 −27
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions src/dune_rules/melange/melange_rules.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,13 @@ let output_of_lib ~target_dir lib =
let info = Lib.info lib in
match Lib_info.status info with
| Private _ -> `Private_library_or_emit target_dir
| Installed | Installed_private ->
| Installed | Installed_private | Public _ ->
let lib_name = Lib_info.name info in
let src_dir = Lib_info.src_dir info in
`Public_library
( src_dir
, Path.Build.L.relative target_dir
[ "node_modules"; Lib_name.to_string lib_name ] )
| Public _ ->
let lib_name = Lib_info.name info in
let src_dir = Lib_info.src_dir info in
`Public_library
( src_dir
, Path.Build.L.relative target_dir
[ "node_modules"
; Lib_name.to_string lib_name
; Path.Source.to_string (Path.drop_build_context_exn src_dir)
] )

let make_js_name ~js_ext ~output m =
let basename = Melange.js_basename m ^ js_ext in
Expand Down
41 changes: 28 additions & 13 deletions src/dune_rules/module_compilation.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,39 @@ let copy_interface ~sctx ~dir ~obj_dir ~cm_kind m =
(Path.build (Obj_dir.Module.cm_file_exn obj_dir m ~kind:cmi_kind))
~dst:(Obj_dir.Module.cm_public_file_exn obj_dir m ~kind:cmi_kind)))

let melange_args (cm_kind : Lib_mode.Cm_kind.t) lib_name module_ =
let melange_args (cctx : Compilation_context.t) (cm_kind : Lib_mode.Cm_kind.t)
module_ =
match cm_kind with
| Ocaml (Cmi | Cmo | Cmx) | Melange Cmi -> []
| Melange Cmj ->
let bs_package_name =
match lib_name with
| None -> []
let bs_package_name, bs_package_output =
let package_output =
Module.file ~ml_kind:Impl module_ |> Option.value_exn |> Path.parent_exn
in
match Compilation_context.public_lib_name cctx with
| None -> ([], package_output)
| Some lib_name ->
[ Command.Args.A "--bs-package-name"; A (Lib_name.to_string lib_name) ]
in
let package_output =
Module.file ~ml_kind:Impl module_ |> Option.value_exn |> Path.parent_exn
let dir =
let package_output = Path.as_in_build_dir_exn package_output in
let lib_root_dir =
Path.Build.to_string (Compilation_context.dir cctx)
in
let src_dir = Path.Build.to_string package_output in
let build_dir =
(Compilation_context.super_context cctx |> Super_context.context)
.build_dir
in
String.drop_prefix src_dir ~prefix:lib_root_dir
|> Option.value_exn
|> String.drop_prefix_if_exists ~prefix:"/"
|> Path.Build.relative build_dir
in

( [ Command.Args.A "--bs-package-name"; A (Lib_name.to_string lib_name) ]
, Path.build dir )
in
Command.Args.A "--bs-stop-after-cmj" :: A "--bs-package-output"
:: Command.Args.Path package_output :: A "--bs-module-name"
:: Command.Args.Path bs_package_output :: A "--bs-module-name"
:: A (Melange.js_basename module_)
:: bs_package_name

Expand Down Expand Up @@ -238,10 +256,7 @@ let build_cm cctx ~force_write_cmi ~precompiled_cmi ~cm_kind (m : Module.t)
; Command.Args.as_any
(Lib_mode.Cm_kind.Map.get (CC.includes cctx) cm_kind)
; As extra_args
; S
(melange_args cm_kind
(Compilation_context.public_lib_name cctx)
m)
; S (melange_args cctx cm_kind m)
; A "-no-alias-deps"
; opaque_arg
; As (Fdo.phase_flags phase)
Expand Down
6 changes: 3 additions & 3 deletions test/blackbox-tests/test-cases/melange/public.t/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Cmj rules should include --bs-package-output
$ dune rules my_project/app/.app.objs/melange/app.cmj |
> grep -e "--bs-package-output" --after-context=1
--bs-package-output
my_project/app
.

Cmj rules should include --bs-package-name
$ dune rules my_project/app/.app.objs/melange/app.cmj |
Expand All @@ -15,13 +15,13 @@ Cmj rules should include --bs-package-name
$ output=my_project/output

Js rules should include --bs-module-type
$ dune rules $output/node_modules/pkg.app/my_project/app/b.js |
$ dune rules $output/node_modules/pkg.app/b.js |
> grep -e "--bs-module-type" --after-context=1
--bs-module-type
commonjs

Js rules should include --bs-package-name
$ dune rules $output/node_modules/pkg.app/my_project/app/b.js |
$ dune rules $output/node_modules/pkg.app/b.js |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this pr, but shouldn't this be pkg/app?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks correct to me. the library name is pkg.app, we use that name for the node_modules folder.

> grep -e "--bs-package-name" --after-context=1
--bs-package-name
pkg
Expand Down