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.

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

Do not use the transitive closure in generated META files #405

Merged
2 commits merged into from
Jan 12, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 3 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ next
[opam-cross-windows](https://github.com/whitequark/opam-cross-windows)
(#355)

- Simplify generated META files: do not generate the transitive
closure of dependencies in META files (#405)

1.0+beta16 (05/11/2017)
-----------------------

Expand Down
32 changes: 16 additions & 16 deletions src/gen_meta.ml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
open Import
open Jbuild
open Meta
open Build.O

module Pub_name = struct
type t =
Expand Down Expand Up @@ -121,7 +120,7 @@ let gen_lib pub_name (lib : Library.t) ~lib_deps ~ppx_runtime_deps:ppx_rt_deps ~
)
]

let gen ~package ~version ~stanzas ~lib_deps ~ppx_runtime_deps =
let gen ~package ~version ~stanzas ~resolve_lib_dep_names =
let items =
List.filter_map stanzas ~f:(fun (dir, stanza) ->
match (stanza : Stanza.t) with
Expand All @@ -131,20 +130,21 @@ let gen ~package ~version ~stanzas ~lib_deps ~ppx_runtime_deps =
| _ ->
None)
in
(version >>^ function
| None -> []
| Some s -> [rule "version" [] Set s])
>>>
Build.all
(List.map items ~f:(fun (Lib (dir, pub_name, lib)) ->
Build.fanout3
(Build.arr (fun x -> x))
(lib_deps ~dir (Stanza.Library lib))
(ppx_runtime_deps ~dir (Stanza.Library lib))
>>^ fun (version, lib_deps, ppx_runtime_deps) ->
(pub_name,
gen_lib pub_name lib ~lib_deps ~ppx_runtime_deps ~version)))
>>^ fun pkgs ->
let version =
match version with
| None -> []
| Some s -> [rule "version" [] Set s]
in
let pkgs =
List.map items ~f:(fun (Lib (dir, pub_name, lib)) ->
let lib_deps = resolve_lib_dep_names ~dir lib.buildable.libraries in
let ppx_runtime_deps =
resolve_lib_dep_names ~dir
(List.map lib.ppx_runtime_libraries ~f:Lib_dep.direct)
in
(pub_name,
gen_lib pub_name lib ~lib_deps ~ppx_runtime_deps ~version))
in
let pkgs =
List.map pkgs ~f:(fun (pn, meta) ->
match Pub_name.to_list pn with
Expand Down
11 changes: 3 additions & 8 deletions src/gen_meta.mli
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,7 @@ open! Import

val gen
: package:string
-> version:(unit, string option) Build.t
-> version:string option
-> stanzas:(Path.t * Jbuild.Stanza.t) list
-> lib_deps:(dir:Path.t
-> Jbuild.Stanza.t
-> (Meta.entry list, string list) Build.t)
-> ppx_runtime_deps:(dir:Path.t
-> Jbuild.Stanza.t
-> (Meta.entry list, string list) Build.t)
-> (unit, Meta.t) Build.t
-> resolve_lib_dep_names:(dir:Path.t -> Jbuild.Lib_dep.t list -> string list)
-> Meta.t
24 changes: 2 additions & 22 deletions src/gen_rules.ml
Original file line number Diff line number Diff line change
Expand Up @@ -881,31 +881,11 @@ Add it to your jbuild file to remove this warning.
Build.return ["# JBUILDER_GEN"]
in
let meta =
version >>^ fun version ->
Gen_meta.gen ~package:pkg.name
~version
~stanzas:(SC.stanzas_to_consider_for_install sctx)
~lib_deps:(fun ~dir jbuild ->
match jbuild with
| Library lib ->
Build.arr ignore
>>>
SC.Libs.load_requires sctx ~dir ~item:lib.name
>>^ List.map ~f:Lib.best_name
| Executables exes ->
let item = List.hd exes.names in
Build.arr ignore
>>>
SC.Libs.load_requires sctx ~dir ~item
>>^ List.map ~f:Lib.best_name
| _ -> Build.arr (fun _ -> []))
~ppx_runtime_deps:(fun ~dir jbuild ->
match jbuild with
| Library lib ->
Build.arr ignore
>>>
SC.Libs.load_runtime_deps sctx ~dir ~item:lib.name
>>^ List.map ~f:Lib.best_name
| _ -> Build.arr (fun _ -> []))
~resolve_lib_dep_names:(SC.Libs.best_lib_dep_names_exn sctx)
in
SC.add_rule sctx
(Build.fanout meta template
Expand Down
59 changes: 34 additions & 25 deletions src/lib_db.ml
Original file line number Diff line number Diff line change
Expand Up @@ -144,33 +144,36 @@ let create findlib ~scopes internal_libraries =
let internal_libs_without_non_installable_optional_ones t =
String_map.values t.instalable_internal_libs

let interpret_lib_dep t ~dir lib_dep =
match lib_dep with
| Lib_dep.Direct name -> begin
match find_exn t ~from:dir name with
| x -> Inl [x]
| exception e ->
(* Call [find] again to get a proper backtrace *)
Inr { fail = fun () -> ignore (find_exn t ~from:dir name : Lib.t); raise e }
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind explaining this trick a bit? How does calling find_exn again get us a proper back trace? Isn't it going to just raise and we won't re-raise e?

Copy link
Author

Choose a reason for hiding this comment

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

yes, that's the expectation. In fact we could just replace the raise e by assert false. By calling find_exn, we get a more precise backtrace. For instance it might start in findlib.ml

Copy link
Member

Choose a reason for hiding this comment

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

OK, I think doing assert false will be a little nicer since we won't even have to bind e in the pattern match.

end
| Select { choices; loc; _ } ->
match
List.find_map choices ~f:(fun { required; forbidden; _ } ->
if String_set.exists forbidden ~f:(lib_is_available t ~from:dir) then
None
else
match
List.map (String_set.elements required) ~f:(find_exn t ~from:dir)
with
| l -> Some l
| exception _ -> None)
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: Maybe we should just add Option.try_with I see that we do this idiom in a few places already.

Copy link
Author

Choose a reason for hiding this comment

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

indeed

with
| Some l -> Inl l
| None ->
Inr { fail = fun () ->
Loc.fail loc "No solution found for this select form"
}

let interpret_lib_deps t ~dir lib_deps =
let libs, failures =
List.partition_map lib_deps ~f:(function
| Lib_dep.Direct name -> begin
match find_exn t ~from:dir name with
| x -> Inl [x]
| exception e ->
(* Call [find] again to get a proper backtrace *)
Inr { fail = fun () -> ignore (find_exn t ~from:dir name : Lib.t); raise e }
end
| Select { choices; loc; _ } ->
match
List.find_map choices ~f:(fun { required; forbidden; _ } ->
if String_set.exists forbidden ~f:(lib_is_available t ~from:dir) then
None
else
match
List.map (String_set.elements required) ~f:(find_exn t ~from:dir)
with
| l -> Some l
| exception _ -> None)
with
| Some l -> Inl l
| None ->
Inr { fail = fun () ->
Loc.fail loc "No solution found for this select form"
})
List.partition_map lib_deps ~f:(interpret_lib_dep t ~dir)
in
let internals, externals =
List.partition_map (List.concat libs) ~f:(function
Expand All @@ -182,6 +185,12 @@ let interpret_lib_deps t ~dir lib_deps =
| [] -> None
| f :: _ -> Some f)

let best_lib_dep_names_exn t ~dir lib_deps =
List.concat_map lib_deps ~f:(fun lib_dep ->
match interpret_lib_dep t ~dir lib_dep with
| Inl libs -> List.map libs ~f:Lib.best_name
| Inr fail -> fail.fail ())

type resolved_select =
{ src_fn : string
; dst_fn : string
Expand Down
6 changes: 6 additions & 0 deletions src/lib_db.mli
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ val interpret_lib_deps
-> Jbuild.Lib_dep.t list
-> Lib.Internal.t list * Findlib.package list * fail option

val best_lib_dep_names_exn
: t
-> dir:Path.t
-> Jbuild.Lib_dep.t list
-> string list

type resolved_select =
{ src_fn : string
; dst_fn : string
Expand Down
2 changes: 2 additions & 0 deletions src/super_context.ml
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,8 @@ module Libs = struct

let find t ~from name = find t.libs ~from name

let best_lib_dep_names_exn t ~dir deps = best_lib_dep_names_exn t.libs ~dir deps

let vrequires t ~dir ~item =
let fn = Path.relative dir (item ^ ".requires.sexp") in
Build.Vspec.T (fn, t.libs_vfile)
Expand Down
1 change: 1 addition & 0 deletions src/super_context.mli
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ val unique_library_name : t -> Lib.t -> string

module Libs : sig
val find : t -> from:Path.t -> string -> Lib.t option
val best_lib_dep_names_exn : t -> dir:Path.t -> Lib_dep.t list -> string list

val load_requires : t -> dir:Path.t -> item:string -> (unit, Lib.t list) Build.t
val load_runtime_deps : t -> dir:Path.t -> item:string -> (unit, Lib.t list) Build.t
Expand Down
6 changes: 3 additions & 3 deletions test/blackbox-tests/test-cases/meta-gen/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,17 @@
package "rewriter" (
directory = "rewriter"
description = "ppx rewriter"
requires(ppx_driver) = "bytes foobar"
requires(ppx_driver) = "foobar"
archive(ppx_driver,byte) = "foobar_rewriter.cma"
archive(ppx_driver,native) = "foobar_rewriter.cmxa"
plugin(ppx_driver,byte) = "foobar_rewriter.cma"
plugin(ppx_driver,native) = "foobar_rewriter.cmxs"
# This is what jbuilder uses to find out the runtime dependencies of
# a preprocessor
ppx_runtime_deps = "bytes foobar.baz"
ppx_runtime_deps = "foobar.baz"
# This line makes things transparent for people mixing preprocessors
# and normal dependencies
requires(-ppx_driver) = "bytes foobar.baz"
requires(-ppx_driver) = "foobar.baz"
ppx(-ppx_driver,-custom_ppx) = "./ppx.exe --as-ppx"
)
package "sub" (
Expand Down