-
Notifications
You must be signed in to change notification settings - Fork 454
Add support for -H compiler flag #10644
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
a4a6bf9
663ec11
42f1798
1bf8bf8
1893536
0c07a77
366ca51
d9df9e4
9fae1f7
3607a91
923a0bc
8e1757f
cdd08f7
c8f4f7f
6ef7f54
51d6e2f
83bb651
2812ddb
80a1c02
9d8c029
c11722d
f1882cc
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 |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| - Add support for the -H flag (introduced in OCaml compiler 5.2) in dune | ||
| (requires lang versions 3.17). This adaptation gives | ||
| the correct semantics for `(implicit_transitive_deps false)`. | ||
| (#10644, fixes #9333, ocsigen/tyxml#274, #2733, #4963, @MA0100) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,36 +3,42 @@ open Import | |
| module Includes = struct | ||
| type t = Command.Args.without_targets Command.Args.t Lib_mode.Cm_kind.Map.t | ||
|
|
||
| let make ~project ~opaque ~requires : _ Lib_mode.Cm_kind.Map.t = | ||
| let make ~project ~opaque ~direct_requires ~hidden_requires : _ Lib_mode.Cm_kind.Map.t = | ||
| (* TODO : some of the requires can filtered out using [ocamldep] info *) | ||
| let open Resolve.Memo.O in | ||
| let iflags libs mode = Lib_flags.L.include_flags ~project libs mode in | ||
| let iflags direct_libs hidden_libs mode = | ||
| Lib_flags.L.include_flags ~project ~direct_libs ~hidden_libs mode | ||
| in | ||
| let make_includes_args ~mode groups = | ||
| Command.Args.memo | ||
| (Resolve.Memo.args | ||
| (let+ libs = requires in | ||
| (let+ direct_libs = direct_requires | ||
| and+ hidden_libs = hidden_requires in | ||
| Command.Args.S | ||
| [ iflags libs mode; Hidden_deps (Lib_file_deps.deps libs ~groups) ])) | ||
| [ iflags direct_libs hidden_libs mode | ||
| ; Hidden_deps (Lib_file_deps.deps (direct_libs @ hidden_libs) ~groups) | ||
| ])) | ||
| in | ||
| let cmi_includes = make_includes_args ~mode:(Ocaml Byte) [ Ocaml Cmi ] in | ||
| let cmx_includes = | ||
| Command.Args.memo | ||
| (Resolve.Memo.args | ||
| (let+ libs = requires in | ||
| (let+ direct_libs = direct_requires | ||
| and+ hidden_libs = hidden_requires in | ||
| Command.Args.S | ||
| [ iflags libs (Ocaml Native) | ||
| [ iflags direct_libs hidden_libs (Ocaml Native) | ||
| ; Hidden_deps | ||
| (if opaque | ||
| then | ||
| List.map libs ~f:(fun lib -> | ||
| List.map (direct_libs @ hidden_libs) ~f:(fun lib -> | ||
| ( lib | ||
| , if Lib.is_local lib | ||
| then [ Lib_file_deps.Group.Ocaml Cmi ] | ||
| else [ Ocaml Cmi; Ocaml Cmx ] )) | ||
| |> Lib_file_deps.deps_with_exts | ||
| else | ||
| Lib_file_deps.deps | ||
| libs | ||
| (direct_libs @ hidden_libs) | ||
| ~groups:[ Lib_file_deps.Group.Ocaml Cmi; Ocaml Cmx ]) | ||
| ])) | ||
| in | ||
|
|
@@ -74,6 +80,7 @@ type t = | |
| ; modules : modules | ||
| ; flags : Ocaml_flags.t | ||
| ; requires_compile : Lib.t list Resolve.Memo.t | ||
| ; requires_hidden : Lib.t list Resolve.Memo.t | ||
| ; requires_link : Lib.t list Resolve.t Memo.Lazy.t | ||
| ; includes : Includes.t | ||
| ; preprocessing : Pp_spec.t | ||
|
|
@@ -99,6 +106,7 @@ let obj_dir t = t.obj_dir | |
| let modules t = t.modules.modules | ||
| let flags t = t.flags | ||
| let requires_compile t = t.requires_compile | ||
| let requires_hidden t = t.requires_hidden | ||
| let requires_link t = Memo.Lazy.force t.requires_link | ||
| let includes t = t.includes | ||
| let preprocessing t = t.preprocessing | ||
|
|
@@ -139,10 +147,24 @@ let create | |
| = | ||
| let open Memo.O in | ||
| let project = Scope.project scope in | ||
| let requires_compile = | ||
| let context = Super_context.context super_context in | ||
| let* ocaml = Context.ocaml context in | ||
| let direct_requires, hidden_requires = | ||
| if Dune_project.implicit_transitive_deps project | ||
| then Memo.Lazy.force requires_link | ||
| else requires_compile | ||
| then Memo.Lazy.force requires_link, Resolve.Memo.return [] | ||
| else if Version.supports_hidden_includes ocaml.version | ||
| && Dune_project.dune_version project >= (3, 17) | ||
|
Member
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 this mode still needs to be guarded behind
Contributor
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. Sorry, but I do not seem to have understood well your point. The new feature is invoked only when Thanks for the review.
Member
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. Yeah, that wasn't very clear at all. Let me explain the behavior I would like to achieve: When we're using When we're using When we're using The issue with the current behavior is that users do expect upgrading to 3.17 to be relatively easy. With this change, they will now need to look at all their transitive deps. While I agree that it's a good thing, it's too much of a breakage to introduce in a minor version bump. Therefore, I suggest that we guard this behind a feature flag. We already have a feature flag though |
||
| then ( | ||
| let requires_hidden = | ||
| let open Resolve.Memo.O in | ||
| let+ requires_compile = requires_compile | ||
| and+ requires_link = Memo.Lazy.force requires_link in | ||
| let requires_table = Table.create (module Lib) 5 in | ||
| List.iter ~f:(fun lib -> Table.set requires_table lib ()) requires_compile; | ||
| List.filter requires_link ~f:(fun l -> not (Table.mem requires_table l)) | ||
| in | ||
| requires_compile, requires_hidden) | ||
| else requires_compile, Resolve.Memo.return [] | ||
| in | ||
| let sandbox = Sandbox_config.no_special_requirements in | ||
|
Collaborator
Author
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. Is this change needed? |
||
| let modes = | ||
|
|
@@ -153,8 +175,6 @@ let create | |
| in | ||
| Option.value ~default modes |> Lib_mode.Map.map ~f:Option.is_some | ||
| in | ||
| let context = Super_context.context super_context in | ||
| let* ocaml = Context.ocaml context in | ||
| let opaque = | ||
| let profile = Context.profile context in | ||
| eval_opaque ocaml profile opaque | ||
|
|
@@ -180,9 +200,10 @@ let create | |
| ; obj_dir | ||
| ; modules = { modules; dep_graphs } | ||
| ; flags | ||
| ; requires_compile | ||
| ; requires_compile = direct_requires | ||
| ; requires_hidden = hidden_requires | ||
| ; requires_link | ||
| ; includes = Includes.make ~project ~opaque ~requires:requires_compile | ||
| ; includes = Includes.make ~project ~opaque ~direct_requires ~hidden_requires | ||
| ; preprocessing | ||
| ; opaque | ||
| ; stdlib | ||
|
|
@@ -263,8 +284,16 @@ let for_module_generated_at_link_time cctx ~requires ~module_ = | |
| their implementation must also be compiled with -opaque *) | ||
| Ocaml.Version.supports_opaque_for_mli cctx.ocaml.version | ||
| in | ||
| let direct_requires = requires in | ||
| let hidden_requires = Resolve.Memo.return [] in | ||
| let modules = singleton_modules module_ in | ||
| let includes = Includes.make ~project:(Scope.project cctx.scope) ~opaque ~requires in | ||
| let includes = | ||
| Includes.make | ||
| ~project:(Scope.project cctx.scope) | ||
| ~opaque | ||
| ~direct_requires | ||
| ~hidden_requires | ||
| in | ||
| { cctx with | ||
| opaque | ||
| ; flags = Ocaml_flags.empty | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,13 +85,16 @@ let link_deps sctx t mode = | |
| module L = struct | ||
| type nonrec t = Lib.t list | ||
|
|
||
| let to_iflags dirs = | ||
| let to_flags flag dirs = | ||
| Command.Args.S | ||
| (Path.Set.fold dirs ~init:[] ~f:(fun dir acc -> | ||
| Command.Args.Path dir :: A "-I" :: acc) | ||
| Command.Args.Path dir :: A flag :: acc) | ||
| |> List.rev) | ||
| ;; | ||
|
|
||
| let to_iflags dir = to_flags "-I" dir | ||
| let to_hflags dir = to_flags "-H" dir | ||
|
|
||
| let remove_stdlib dirs libs = | ||
| match libs with | ||
| | [] -> dirs | ||
|
|
@@ -155,8 +158,13 @@ module L = struct | |
| remove_stdlib dirs ts | ||
| ;; | ||
|
|
||
| let include_flags ?project ts mode = | ||
| to_iflags (include_paths ?project ts { lib_mode = mode; melange_emit = false }) | ||
| let include_flags ?project ~direct_libs ~hidden_libs mode = | ||
| let include_paths ts = | ||
| include_paths ?project ts { lib_mode = mode; melange_emit = false } | ||
| in | ||
| let hidden_includes = to_hflags (include_paths hidden_libs) in | ||
| let direct_includes = to_iflags (include_paths direct_libs) in | ||
| Command.Args.S [ direct_includes; hidden_includes ] | ||
|
Member
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. Isn't this re-ordering the include flags? e.g. if we have
Collaborator
Author
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. If I understand things correctly, I don't think anything is being re-ordered; instead some extra
Do you agree with this understanding @MA0010? |
||
| ;; | ||
|
|
||
| let melange_emission_include_flags ?project ts = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let x = 5 | ||
| let y = Foo.v |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| (library | ||
| (name foo) | ||
| (modules foo)) | ||
|
|
||
| (library | ||
| (name bar) | ||
| (modules bar) | ||
| (libraries foo)) | ||
|
|
||
| (executable | ||
| (name run) | ||
| (modules run) | ||
| (libraries bar)) | ||
|
|
||
| (executable | ||
| (name runf) | ||
| (modules runf) | ||
| (libraries bar)) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let v = 9 | ||
| let w = 4 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| let _ = Bar.y |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,57 @@ | ||||
| This test is guarded by ocaml version >= 5.2, so it should include foo with -H when | ||||
| implicit_transitive_deps is set to false. | ||||
|
|
||||
| $ getincludes () { | ||||
| > dune build --verbose ./run.exe 2>&1 | grep run.ml | grep -Eo '\-[IH] [a-z/.]+' | sort | ||||
| > } | ||||
|
|
||||
| $ cat >dune-project <<EOF | ||||
| > (lang dune 3.17) | ||||
| > (implicit_transitive_deps true) | ||||
| > EOF | ||||
|
|
||||
|
Collaborator
Author
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.
Suggested change
|
||||
| $ getincludes | ||||
| -I .bar.objs/byte | ||||
| -I .bar.objs/byte | ||||
| -I .bar.objs/native | ||||
| -I .foo.objs/byte | ||||
| -I .foo.objs/byte | ||||
| -I .foo.objs/native | ||||
| -I .run.eobjs/byte | ||||
| -I .run.eobjs/byte | ||||
| -I .run.eobjs/native | ||||
|
|
||||
| $ cat >dune-project <<EOF | ||||
| > (lang dune 3.17) | ||||
| > (implicit_transitive_deps false) | ||||
| > EOF | ||||
|
|
||||
| $ getincludes | ||||
| -H .foo.objs/byte | ||||
| -H .foo.objs/byte | ||||
| -H .foo.objs/native | ||||
| -I .bar.objs/byte | ||||
| -I .bar.objs/byte | ||||
| -I .bar.objs/native | ||||
| -I .run.eobjs/byte | ||||
| -I .run.eobjs/byte | ||||
| -I .run.eobjs/native | ||||
|
|
||||
| Test transitive deps can not be directly accessed, both for compiler versions supporting -H or not: | ||||
|
|
||||
| $ cat >dune-project <<EOF | ||||
| > (lang dune 3.17) | ||||
| > (implicit_transitive_deps false) | ||||
| > EOF | ||||
|
|
||||
| $ dune build ./runf.exe 2>&1 | grep -v ocamlc | ||||
| File "runf.ml", line 1, characters 16-21: | ||||
| 1 | let a = Bar.y + Foo.v | ||||
| ^^^^^ | ||||
| Error: Unbound module Foo | ||||
|
|
||||
| Test if #274 is fixed: | ||||
|
|
||||
| $ dune build --root=./tyxml | ||||
| Entering directory 'tyxml' | ||||
| Leaving directory 'tyxml' | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| let a = Bar.y + Foo.v |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| (executable | ||
| (name run) | ||
| (libraries tyxml)) | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| (lang dune 3.17) | ||
| (implicit_transitive_deps false) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| open Tyxml.Html | ||
| let _ = p [ a [ txt "a" ] ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let x = 5 | ||
| let y = Foo.v |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| (library | ||
| (name foo) | ||
| (modules foo)) | ||
|
|
||
| (library | ||
| (name bar) | ||
| (modules bar) | ||
| (libraries foo)) | ||
|
|
||
| (executable | ||
| (name run) | ||
| (modules run) | ||
| (libraries bar)) | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let v = 9 | ||
| let w = 4 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let _ = Bar.y | ||
| let _ = print_endline "yes" |
Uh oh!
There was an error while loading. Please reload this page.