Refactor library management #516

ghost · 2018-02-14T16:32:19Z

No description provided.

ghost · 2018-02-17T12:22:27Z

This took much longer than I expected but it's now working. I think things are a bit cleaner now, in particular all the library resolution and management is done at a single place.

Here is a summary of the changes:

Lib module

We have a new module Lib that replaces Lib, parts of Lib_db and parts of Findlib. It is used to manage all libraries (internal and extrernal). Lib.t represent a completely resolved library, i.e. where all the dependencies have been resolved. Lib.Compile is used to provide what is necessary to build the library itself. Lib.Meta provides what is necessary to generate the META file for the library.

We also have library databases represented as Lib.DB.t. A library database is simply a mapping from names to Lib.t values and and created from a resolve function that looks up a name and return a Lib.Info.t. A Lib.Info.t is the same as a Lib.t except that dependencies are not resolved.

A library database can have a parent database that is used to lookup names that are not found in the current database. In practice we have the following hierarchy:

For every scope, we have a library database that holds all the libraries of this scope. In this DB, a library can be referred by either it's name or public name
the parent of each of these databases is a database that holds all the public libraries of the workspace. In this DB libraries must be referred by their public name
the parent of this DB is for installed libraries

(1) databases are accessible via Scope.libs (Super_context.find_scope_by_{name,dir} sctx xxx)
(2) is accessible via Super_context.public_libs sctx
(3) is accessible via Super_context.installed_libs sctx

The dependencies of a library are always resolved inside the DB it is part of. When we compute a transitive closure, we check that we don't have two libraries from two different DB with the same name. So for instance linting Base should now supported.

Jbuild.Scope_info

Jbuild.Scope was renamed Jbuild.Scope_info

Scope module

This replaces Lib_db. A Scope.t is now just a pair of a Jbuild.Scope_info.t and a Lib.DB.t. Scope.DB.t is an object used to lookup scopes by either name or directory.

We no longer have an external scope or special anonymous scope. Instead one should use Super_context.installed_libs or Super_context.public_libs depending on the context.

rgrinberg · 2018-02-20T06:58:42Z

bin/main.ml

@@ -758,7 +756,7 @@ let external_lib_deps =
               in
               let missing =
                 String_map.filter externals ~f:(fun name _ ->
-                   not (Findlib.available context.findlib name))
+                   not (Findlib.mem context.findlib name))


I like the old name here a bit more. Since findlib does have a distinction between a package existing vs. being available (e.g. the exists_if stuff).

Ok, makes sense. This is the same for Lib.DB.mem actually

rgrinberg · 2018-02-20T07:09:16Z

src/build_system.ml

+            x
+          | exception exn ->
+            m.state <- Unevaluated;
+            reraise exn


Is this some bug that's being fixed? It doesn't seem related to the lib refactoring.

Yh, it just that it popped up in a test with this PR. Basically if evaluating a memoized Build.t failed, it was staying forever in Evaluating state, causing a cycle error

rgrinberg · 2018-02-20T07:27:34Z

src/odoc.ml

+    match Lib.DB.find (Scope.libs scope) lib.name with
+    | Error Not_found -> assert false
+    | Error (Hidden _) ->
+      (Utils.library_object_directory ~dir lib.name,


Would you mind explaining this case a bit? I'm not sure when an internal lib would be hidden. Also, it's still unsatisfying to go outside of the lib interface to get the object dir in that case.

It's for libraries that are optional with unavailable dependencies.

Also, it's still unsatisfying to go outside of the lib interface to get the object dir in that case.

Indeed. I suppose we could keep the Info.t in this case

rgrinberg · 2018-02-20T07:31:04Z

src/odoc.ml

-    Option.iter (Lib_db.Scope.find scope lib >>= Lib.src_dir)
-      ~f:(fun dir -> SC.load_dir sctx ~dir)
+    match Lib.DB.find lib_db lib with
+    | Error _ -> ()


A bit off topic since that's the old behavior anyway, but why are we ignoring such errors? This cames seems impossible, so shouldn't it be assert false or a Sexp.code_error?

Actually it's not impossible. We can reach this case with:

$ jbuilder build _build/default/_doc/blah/...

Currently, this will cause a no rule found for ... error. Maybe we could return something more precise

rgrinberg · 2018-02-20T07:33:27Z

src/result/result.mli

+val map  : ('a, 'error) t -> f:('a -> 'b) -> ('b, 'error) t
+val bind : ('a, 'error) t -> f:('a -> ('b, 'error) t) -> ('b, 'error) t
+
+val map_error : ('a, 'error1) t -> f:('error1 -> 'error2) -> ('a, 'error2) t


Rather than extending this module, we should consider just vendoring rresult.ml I think.

I think we should limit the vendoring to things to things that provide complex features or that are dependencies of things we vendor. Result is a very simple module. Moreover the API conventions of rresult are quite different from the rest of the Dune codebase

Lib module ---------- We have a new module Lib that replaces Lib, parts of Lib_db and parts of Findlib. It is used to manage all libraries (internal and extrernal). Lib.t represent a completely resolved library, i.e. where all the dependencies have been resolved. Lib.Compile is used to provide what is necessary to build the library itself. Lib.Meta provides what is necessary to generate the META file for the library. We also have library databases represented as Lib.DB.t. A library database is simply a mapping from names to Lib.t values and and created from a resolve function that looks up a name and return a Lib.Info.t. A Lib.Info.t is the same as a Lib.t except that dependencies are not resolved. A library database can have a parent database that is used to lookup names that are not found in the current database. In practice we have the following hierarchy: 1. For every scope, we have a library database that holds all the libraries of this scope. In this DB, a library can be referred by either it's name or public name 2. the parent of each of these databases is a database that holds all the public libraries of the workspace. In this DB libraries must be referred by their public name 3. the parent of this DB is for installed libraries (1) databases are accessible via Scope.libs (Super_context.find_scope_by_{name,dir} sctx xxx) (2) is accessible via Super_context.public_libs sctx (3) is accessible via Super_context.installed_libs sctx The dependencies of a library are always resolved inside the DB it is part of. When we compute a transitive closure, we check that we don't have two libraries from two different DB with the same name. So for instance linting Base should now supported. Jbuild.Scope_info ----------------- Jbuild.Scope was renamed Jbuild.Scope_info Scope module ------------ This replaces Lib_db. A Scope.t is now just a pair of a Jbuild.Scope_info.t and a Lib.DB.t. Scope.DB.t is an object used to lookup scopes by either name or directory. We no longer have an external scope or special anonymous scope. Instead one should use Super_context.installed_libs or Super_context.public_libs depending on the context.

ghost mentioned this pull request Feb 15, 2018

Inline tests support #338

Closed

11 tasks

ghost requested a review from rgrinberg February 17, 2018 12:22

ghost changed the title ~~[WIP] refactor library management~~ Refactor library management Feb 19, 2018

rgrinberg approved these changes Feb 20, 2018

View reviewed changes

ghost force-pushed the refactor-lib-management branch from c0f1eec to 527bb4f Compare February 20, 2018 11:23

ghost merged commit 9e4cc84 into master Feb 20, 2018

ghost mentioned this pull request Feb 22, 2018

Better (library) name resolution #401

Closed

emillon deleted the refactor-lib-management branch December 13, 2018 16:04

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor library management #516

Refactor library management #516

ghost commented Feb 14, 2018

ghost commented Feb 17, 2018

rgrinberg Feb 20, 2018

ghost Feb 20, 2018

ghost Feb 20, 2018

rgrinberg Feb 20, 2018

ghost Feb 20, 2018

rgrinberg Feb 20, 2018

ghost Feb 20, 2018

ghost Feb 20, 2018

rgrinberg Feb 20, 2018

ghost Feb 20, 2018

rgrinberg Feb 20, 2018

ghost Feb 20, 2018

Refactor library management #516

Refactor library management #516

Conversation

ghost commented Feb 14, 2018

ghost commented Feb 17, 2018

Lib module

Jbuild.Scope_info

Scope module

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