Add an option to replace external-lib-deps command #6045

moyodiallo · 2022-08-05T13:00:39Z

This PR is motivate by dune-opam-lint. The command dune external-lib-deps is used by it and this command is removed in dune #4298. Getting the command back is not realistic, I think. Instead of getting all information by doing a static analysis of the build graph. We extract all the information we need during the build, so dynamically.

The first approach is to categorize the Data Base dependencies like Project_libs db, Public_libs db, Installed_libs db. Then, during the resolution catch all libs that searched in Installed_libs, potentially there are external libs. Easy to add but you have to propagate the build_dir in order to get the dependencies by build_dir.
The second approach, after resolving the lib we could get the necessary needed info from Lib_info. So you don't have to propagate the build_dir across many functions, but the issue is you could miss external dependency (like select rule). In the case of a dune project not all external deps will be resolved in some particular build.

In this PR the first approach was chosen, why not propagate the build_dir and get all external dependencies during the build.

rgrinberg · 2022-08-05T17:44:37Z

Thanks for explaining the implementation strategy. I still don't under the point of this dir argument though. Could you clarify how it helps us deal with conditional dependencies?

rgrinberg · 2022-08-05T17:57:58Z

I should note that using mutation and global variables is also highly suspicious. We've worked very hard to remove all mutable state in our codebase to make it memoization safe. It's very unlikely we're going to undo all that work to reimplement external-lib-deps

moyodiallo · 2022-08-05T18:19:13Z

Thanks for explaining the implementation strategy. I still don't under the point of this dir argument though. Could you clarify how it helps us deal with conditional dependencies?

The dir argument is to know a dependency belong to which package, because the name of a given package (*.opam) could be in the dir, that why we need this information.

rgrinberg · 2022-08-05T18:33:38Z

Where is the association from directory to package is done? I don't see in the code.

For that matter, I don't see why we need to be concerned with packages at all. The check works on libraries only.

moyodiallo · 2022-08-05T18:36:49Z

I should note that using mutation and global variables is also highly suspicious. We've worked very hard to remove all mutable state in our codebase to make it memoization safe. It's very unlikely we're going to undo all that work to reimplement external-lib-deps

True. This is a first shot, I did think about that. Is Memoizing them going to be better in this case ?

rgrinberg · 2022-08-05T18:46:32Z

Memoization is an optimization, so it is something we can think about if our implementation is slow. For now, I'd suggest just trying to extract the information you need by climbing the workspace and building what is needed to extract lib deps (kind of like dune describe for example). There might not be enough information, or the api is insufficient. In that case, we can tweak Lib.t (or whatever else) to provide what you need.

moyodiallo · 2022-08-05T18:52:18Z

Where is the association from directory to package is done? I don't see in the code.

For that matter, I don't see why we need to be concerned with packages at all. The check works on libraries only.

Sorry I didn't precise, those information are important to opam-dune-lint. This is trying to match as much as the result of the removed command dune external-lib-deps --sexp --unstable-by-dir @install. opam-dune-lint extract the dependencies for each package (*.opam files), from the results of --external-lib-deps or dune external-lib-deps.

emillon · 2022-09-08T13:55:21Z

Before discussing the implementation I think it's important to discuss the design of the features and the expectations we have for it.

First, the UI. It seems that it's a CLI flag to add to build and that will display the missing external libs. How is it intended to be used: just dune build --external-lib-deps? We're already displaying an error message when an external library is not found. What is the expected difference with this flag?
I think I would have expected something a separate command, or a new output in dune describe. I understand that this is less precise: we can't find dependencies of a single target and we can't see behind (select) but is it important to include external libraries that are not going to be selected by the build? But we can probably filter by package by considering only the relevant rules, which seems enough to start.
I don't understand the dir argument either. what does this enable? can we start without this? it seems to me that if we first try to collect all external lib dependencies referred to by @install or @runtest (without grouping by package or dir), we are already covering a very important gap in our tooling.

What do you think about these? thanks!

The new version of dune (from 3.0.0) removed "dune external-lib-deps" and this is a patch that work with a patch of dune from "https://github.com/moyodiallo/dune/tree/opam-dune-lint". There's already a PR(ocaml/dune#6045) in dune about this patch, but not garanty to be merged. The command may change also.

The new version of dune (from 3.0.0) removed "dune external-lib-deps" and this is a patch that work with a patch of dune from "https://github.com/moyodiallo/dune/tree/opam-dune-lint". There's already a PR(ocaml/dune#6045) in dune about this patch, but not guarantee to be merged. The command may change also.

moyodiallo · 2022-09-21T09:41:08Z

First, the UI. It seems that it's a CLI flag to add to build and that will display the missing external libs. How is it intended to be used: just dune build --external-lib-deps? We're already displaying an error message when an external library is not found. What is the expected difference with this flag?

Yep this is an issue, because the command is not expected to fail.

moyodiallo · 2022-09-21T09:50:13Z

I think I would have expected something a separate command, or a new output in dune describe. I understand that this is less precise: we can't find dependencies of a single target and we can't see behind (select) but is it important to include external libraries that are not going to be selected by the build? But we can probably filter by package by considering only the relevant rules, which seems enough to start.

Yes, It's better to have a separate command, like @rgrinberg propose a good starting point is dune describeand tweak Lib.t to get Kind_db.t. I don't think select is relevant because I went to dune build because of the idea capturing the external libs during the resolve.

moyodiallo · 2022-09-21T10:01:13Z

I don't understand the dir argument either. what does this enable? can we start without this? it seems to me that if we first try to collect all external lib dependencies referred to by @install or @runtest (without grouping by package or dir), we are already covering a very important gap in our tooling.

This argument was put to group by package for opam-dune-lint. Starting with dune describe, It won't be a big deal because we have all information needed.

moyodiallo · 2022-09-21T10:18:39Z

To summarize little bit

val resolve_name : db -> Lib_name.t -> (Status.t * Kind_db.t) Memo.t

With this modification in Lib.t and during the crawl on the workspace. We get all information about dependencies if there are externals or not.

What do you think ? (don't consider those 2 commits because, they will be reverted)

emillon · 2022-09-22T08:01:53Z

Sure, it's fine to add something to the interface.

moyodiallo · 2022-09-29T10:13:30Z

@emillon you were right about Select, it make sense to know the libraries behind it. It's included in the last changes.
Feel free about factorizing the code.

emillon · 2022-09-29T14:31:56Z

Thanks. Can you rebase and reformat this PR? I'll have some comments on it but they'll move around too much.

emillon

thanks. I think that it's going in the right direction. In addition to the comments I have 2 general remarks:

I wonder if this should be a describe target rather than a top-level command.
please add tests for this feature, including for corner cases like "optional" and CLI errors (if relevant)

emillon · 2022-09-30T09:27:45Z

bin/external_lib_deps.ml

+
+  let to_dyn kind =
+    match kind with
+    | Required -> Dyn.String "require"


Suggested change

| Required -> Dyn.String "require"

| Required -> Dyn.String "required"

emillon · 2022-09-30T09:31:26Z

bin/external_lib_deps.ml

+                    select.choices
+                    |> Memo.parallel_map ~f:(fun choice ->
+                           Lib_name.Set.to_string_list
+                             choice.Lib_dep.Select.Choice.required


style: in dune we variables parameters with their type rather than use a.M.b, so that would be (choice : Lib_dep. ...) ->

emillon · 2022-09-30T09:33:19Z

bin/external_lib_deps.ml

+                           |> Memo.parallel_map ~f:(fun name ->
+                                  let name = Lib_name.of_string name in
+                                  let+ resolved = resolve db name in
+                                  (resolved, Kind.Optional, name)))


does "optional" in this command mean to "behind a select"? that's a bit different from what is meant by (optional) in dune. for example how does this command report dependencies of a binary that's marked (optional)?

emillon · 2022-09-30T09:34:32Z

bin/external_lib_deps.ml

+            | Dune_file.Executables exes ->
+              Memo.return [ (exes.buildable.libraries, dir) ]
+            | Dune_file.Library lib ->
+              Memo.return [ (lib.buildable.libraries, dir) ]


I don't think that anything uses Memo in this part, so you can use plain list iteration and replace let* by let+ above.

emillon · 2022-09-30T09:53:52Z

bin/external_lib_deps.ml

+let run ~context_name ~external_resolved_libs ~by_dir ~sexp ~only_missing =
+  if only_missing then (
+    if by_dir || sexp then
+      User_error.raise


can you move this logic to the main term? there's effectively command line parse errors. the strategy I recommend is to define a type that defines the command can do (for example a record with a format field (sexp/plain), a boolean by_dir, etc), and let the run function be total about this.

emillon · 2022-09-30T09:54:25Z

bin/external_lib_deps.ml

+      (Sexp.pp (Sexp.List [ Sexp.Atom context_name; sexp ])))
+  else (
+    if by_dir then
+      User_error.raise [ Pp.textf "--by-dir cannot be used without --sexp" ];


BTW some of these limitations seem artificial - could we have a by-dir plain output?

I was trying keep the old interface, there's no by-dir plain output. I asked myself if the by-dir is necessary because only sexp could be ok, I think.

emillon · 2022-09-30T10:09:34Z

src/dune_rules/lib.mli

+  type t =
+    | Installed_libs
+    | Public_libs
+    | Project_libs


is the distinction between project and public libs used? I wonder if we could do without this kind field. we can check if a db is the installed_lib ones by checking if its parent is None maybe?

emillon · 2022-09-30T10:10:52Z

bin/external_lib_deps.ml

+    if not by_dir then User_error.raise [ Pp.textf "--sexp requires --by-dir" ];
+    let sexp =
+      external_resolved_libs
+      |> List.map ~f:(fun (dir, libs) ->


This is duplicated with the branch below. I would recommend defining an appropriate data structure, build it in both cases, and print it differently (using to_dyn in one case)

moyodiallo · 2022-09-30T11:27:43Z

wonder if this should be a describe target rather than a top-level command.

I don't get what you mean.

moyodiallo · 2022-10-10T15:52:04Z

The sub-command external-lib-deps is going to move in describe. Any suggestion about the spec ?

emillon · 2022-10-13T09:14:32Z

src/dune_lang/action.ml

@@ -398,6 +398,31 @@ let rec map_string_with_vars t ~f =
  | Pipe (o, ts) -> Pipe (o, List.map ts ~f:(map_string_with_vars ~f))
  | Cram sw -> Cram (f sw)

+let rec list_of_string_with_vars t =


looks like there's a bunch of unrelated code that has been pulled in?

Recycling the old tests, I find out there was lib-available:<library-name> and it's considered like an optional dependency.

When you cherry-picked old tests in a8b6272, the commit also added some changes in src/. Can you remove those?

I didn't do a cherry-pick, the tests was not consequent. So I just did a copy paste mentioning it was the all tests.

OK I understand now. I don't think that this approach is going to work, since it only collects pforms in (rule). Can we stick to the simpler version (that does not try to collect lib names in pforms) for 3.5? Otherwise we can try something that somehow folds over pforms in all stanzas, but this is going to be more involved so we can't do that before 3.6.

emillon · 2022-10-13T09:57:47Z

bin/describe.ml

+      , "Print out external libraries needed to build the project. It's an \
+         approximated set of libraries."
+      , return External_lib_deps )
+    ; ( "missing-external-lib-deps"


does dune-opam-lint require this second one? we said we'd start with the bare minimum to support that command for now.

No. I'm going to retract that.

Yeah, even with only external libs we can find missing one without dune.

rgrinberg

Okay, looks much better now. Left some remarks and comments.

rgrinberg · 2022-10-13T15:01:43Z

src/dune_rules/lib.ml

@@ -775,7 +775,7 @@ module rec Resolve_names : sig
  val resolve_dep :
    db -> Loc.t * Lib_name.t -> private_deps:private_deps -> lib Resolve.Memo.t

-  val resolve_name : db -> Lib_name.t -> Status.t Memo.t
+  val resolve_name : db -> Lib_name.t -> (Status.t * db) Memo.t


Can you explain why we need this change?

This is the last db where the lib_name where searched(or resolved) and with that, we can determine if the lib_name is external or not(external ifdb.parent is None).

~~Yeah I see your concern. I think it would it be better to expand Status.t by having External in it and remove db.~~

rgrinberg · 2022-10-13T15:02:43Z

src/dune_lang/string_with_vars.mli

@@ -10,6 +10,8 @@ open Dune_util
 (** A sequence of text and variables. *)
 type t

+val pforms : t -> Pform.t list


Where is this function being used?

Yeah Previous commit, I did forgot. I'll remove it.
Thanks.

rgrinberg · 2022-10-13T15:03:23Z

src/dune_rules/expander.mli

@@ -141,3 +141,5 @@ val expand_locks :
  base:[ `Of_expander | `This of Path.t ] -> t -> Locks.t -> Path.t list Memo.t

 val sites : t -> Sites.t
+
+val bindings : t -> value Pform.Map.t


Where is this function being used?

Same thing. Thanks.

emillon · 2022-10-13T19:06:04Z

It looks like there's still a bit of work for this feature to be stable so I'm marking this for 3.6 to avoid delaying the 3.5 release.

moyodiallo · 2022-10-13T22:31:00Z

It looks like there's still a bit of work for this feature to be stable so I'm marking this for 3.6 to avoid delaying the 3.5 release.

Just one more little thing. It's gonna be possible. 😁

moyodiallo · 2022-10-14T21:52:38Z

I did another review to delete all the parasite code that I introduced(git diff main). And I think it's good for me, unless you have another remarks or concern.

rgrinberg · 2022-10-14T22:12:27Z

src/dune_rules/lib.mli

@@ -131,6 +131,8 @@ module DB : sig

  val find_even_when_hidden : t -> Lib_name.t -> lib option Memo.t

+  val is_external : t -> Lib_name.t -> bool Memo.t


Instead of introducing this function. How about just doing a resolve first (with find_even_when_hidden). Once the library is resolved, you can check if it's external by looking at the status.

Status.t don't give the information if the lib is external or not. And expanding status it's not a good idea, I tried it.
Resolve then a check, could end up with the case when the library is not installed(not found). We want to print the information whether or not the library is installed.

The constructors Installed and Installed_private correspond to external libraries.

Why is expanding the status not a good idea?

Resolve then a check, could end up with the case when the library is not installed(not found). We want to print the information whether or not the library is installed.

I don't understand. Is there a test case that corresponds to this situation?

I said not a good Idea because I tried to have a constructor External in the status. It was conflicting whether the lib is Found or Not_found.

Yeah I see you suggestion, we doesn't need to expand the status if it's true:
Is it that, if find_even_hidden give None the lib is external and if we get a lib, we test whether it's Installed?

If a lib is not found, we can assume it is an external lib.

Is it that, if find_even_hidden give None the lib is external and if we get a lib, we test whether it's Installed?

Yes, we can do that.

If a lib is not found, we can assume it is an external lib.

That's the best we can do.

In general, we don't want to modify any existing code in lib.ml. It's pretty complicated already and we'd prefer changes that simplify things or at least add things independently. That's why I'm pushing against these changes.

I understand, that sounds good for me and it's equivalent to what I was already doing.

moyodiallo · 2022-10-15T17:36:38Z

The irony(git diff main), using find_even_with_hidden simplifies a lot.

Determining the missing library dependencies is revived under $ dune describe external-lib-deps. Signed-off-by: Alpha DIALLO <[email protected]>

rgrinberg · 2022-10-20T04:32:53Z

Thanks. I merged after a few cleanups.

moyodiallo · 2022-10-20T08:40:54Z

Thanks All of you. I learnt a lot during this PR.

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.6.0) CHANGES: - Forbid multiple instances of dune running concurrently in the same workspace. (ocaml/dune#6360, fixes ocaml/dune#236, @rgrinberg) - Allow promoting into source directories specified by `subdir` (ocaml/dune#6404, fixes ocaml/dune#3502, @rgrinberg) - Make dune describe workspace return the correct root path (ocaml/dune#6380, fixes ocaml/dune#6379, @esope) - Introduce a `$ dune ocaml top-module` subcommand to load modules directly without sealing them behind the signature. (ocaml/dune#5940, @rgrinberg) - [ctypes] do not mangle user written names in the ctypes stanza (ocaml/dune#6374, fixes ocaml/dune#5561, @rgrinberg) - Support `CLICOLOR` and `CLICOLOR_FORCE` to enable/disable/force ANSI colors. (ocaml/dune#6340, fixes ocaml/dune#6323, @MisterDA). - Forbid private libraries with `(package ..)` set from depending on private libraries that don't belong to a package (ocaml/dune#6385, fixes ocaml/dune#6153, @rgrinberg) - Allow `Byte_complete` binaries to be installable (ocaml/dune#4873, @AltGr, @rgrinberg) - Revive `$ dune external-lib-deps` under `$ dune describe external-lib-deps`. (ocaml/dune#6045, @moyodiallo) - Fix running inline tests in bytecode mode (ocaml/dune#5622, fixes ocaml/dune#5515, @dariusf) - [ctypes] always re-run `pkg-config` because we aren't tracking its external dependencies (ocaml/dune#6052, @rgrinberg) - [ctypes] remove dependency on configurator in the generated rules (ocaml/dune#6052, @rgrinberg) - Build progress status now shows number of failed jobs (ocaml/dune#6242, @Alizter) - Allow absolute build directories to find public executables. For example, those specified with `(deps %{bin:...})` (ocaml/dune#6326, @anmonteiro) - Create a fake socket file `_build/.rpc/dune` on windows to allow rpc clients to connect using the build directory. (ocaml/dune#6329, @rgrinberg) - Prevent crash if absolute paths are used in the install stanza and in recursive globs. These cases now result in a user error. (ocaml/dune#6331, @gridbugs) - Add `(glob_files <glob>)` and `(glob_files_rec <glob>)` terms to the `files` field of the `install` stanza (ocaml/dune#6250, closes ocaml/dune#6018, @gridbugs) - Allow `:standard` in the `(modules)` field of the `coq.pp` stanza (ocaml/dune#6229, fixes ocaml/dune#2414, @Alizter) - Fix passing of flags to dune coq top (ocaml/dune#6369, fixes ocaml/dune#6366, @Alizter) - Extend the promotion CLI to a `dune promotion` group: `dune promote` is moved to `dune promotion apply` (the former still works) and the new `dune promotion diff` command can be used to just display the promotion without applying it. (ocaml/dune#6160, fixes ocaml/dune#5368, @emillon)

The new version of dune (from 3.0.0) removed "dune external-lib-deps" and this is a patch that work with a patch of dune from "https://github.com/moyodiallo/dune/tree/opam-dune-lint". There's already a PR(ocaml/dune#6045) in dune about this patch, but not guarantee to be merged. The command may change also.

moyodiallo mentioned this pull request Sep 20, 2022

Fix for "dune external-lib-deps" command ocurrent/opam-dune-lint#46

Merged

moyodiallo force-pushed the external-libs branch from 2851533 to 6f8771a Compare September 29, 2022 10:07

moyodiallo force-pushed the external-libs branch from 6f8771a to 91ce790 Compare September 29, 2022 16:32

emillon reviewed Sep 30, 2022

View reviewed changes

rgrinberg marked this pull request as draft October 9, 2022 22:28

moyodiallo force-pushed the external-libs branch 2 times, most recently from 29dccd5 to aaf6032 Compare October 10, 2022 15:48

moyodiallo force-pushed the external-libs branch from aaf6032 to e097816 Compare October 12, 2022 21:09

emillon reviewed Oct 13, 2022

View reviewed changes

emillon added this to the 3.5.0 milestone Oct 13, 2022

rgrinberg requested changes Oct 13, 2022

View reviewed changes

emillon modified the milestones: 3.5.0, 3.6.0 Oct 13, 2022

moyodiallo force-pushed the external-libs branch from b9589bb to f3a71e8 Compare October 14, 2022 21:47

rgrinberg requested changes Oct 14, 2022

View reviewed changes

rgrinberg force-pushed the external-libs branch from 08f2860 to f6562c2 Compare October 20, 2022 00:23

rgrinberg marked this pull request as ready for review October 20, 2022 00:23

rgrinberg force-pushed the external-libs branch from f6562c2 to 795526b Compare October 20, 2022 00:26

feature: revive external-lib-deps

1a9eb8b

Determining the missing library dependencies is revived under $ dune describe external-lib-deps. Signed-off-by: Alpha DIALLO <[email protected]>

rgrinberg force-pushed the external-libs branch from 795526b to 1a9eb8b Compare October 20, 2022 04:32

rgrinberg merged commit 1d4a783 into ocaml:main Oct 20, 2022

moyodiallo mentioned this pull request Oct 26, 2022

Support dune 3.0 ocurrent/opam-dune-lint#45

Closed

moyodiallo mentioned this pull request Nov 2, 2022

Fix printing internal libs with the command "dune describe external-lib-deps" #6370

Merged

emillon mentioned this pull request Nov 14, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.6.0) ocaml/opam-repository#22498

Merged

	\| Required -> Dyn.String "require"
	\| Required -> Dyn.String "required"

		@@ -131,6 +131,8 @@ module DB : sig

		val find_even_when_hidden : t -> Lib_name.t -> lib option Memo.t

		val is_external : t -> Lib_name.t -> bool Memo.t

Add an option to replace external-lib-deps command #6045

Add an option to replace external-lib-deps command #6045

Conversation

moyodiallo commented Aug 5, 2022

rgrinberg commented Aug 5, 2022

rgrinberg commented Aug 5, 2022

moyodiallo commented Aug 5, 2022

rgrinberg commented Aug 5, 2022

moyodiallo commented Aug 5, 2022

rgrinberg commented Aug 5, 2022

moyodiallo commented Aug 5, 2022

emillon commented Sep 8, 2022

moyodiallo commented Sep 21, 2022

moyodiallo commented Sep 21, 2022

moyodiallo commented Sep 21, 2022

moyodiallo commented Sep 21, 2022

emillon commented Sep 22, 2022

moyodiallo commented Sep 29, 2022

emillon commented Sep 29, 2022

emillon left a 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

moyodiallo commented Sep 30, 2022 • edited Loading

moyodiallo commented Oct 10, 2022

Choose a reason for hiding this comment

moyodiallo Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moyodiallo Oct 13, 2022 • edited Loading

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

rgrinberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moyodiallo Oct 13, 2022 • edited Loading

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

emillon commented Oct 13, 2022

moyodiallo commented Oct 13, 2022 • edited Loading

moyodiallo commented Oct 14, 2022 • edited Loading

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

moyodiallo commented Oct 15, 2022

rgrinberg commented Oct 20, 2022

moyodiallo commented Oct 20, 2022

moyodiallo commented Sep 30, 2022 •

edited

Loading

moyodiallo Oct 13, 2022 •

edited

Loading

moyodiallo Oct 13, 2022 •

edited

Loading

moyodiallo Oct 13, 2022 •

edited

Loading

moyodiallo commented Oct 13, 2022 •

edited

Loading

moyodiallo commented Oct 14, 2022 •

edited

Loading