Add an option to replace external-lib-deps command

[moyodiallo]

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]

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]

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]

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]

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]

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]

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]

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]

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!

[moyodiallo]

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]

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]

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]

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]

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

[moyodiallo]

@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]

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

[moyodiallo]

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

I don't get what you mean.

[moyodiallo]

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

[emillon]

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]

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. :grin:

[moyodiallo]

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.

[moyodiallo]

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

[rgrinberg]

Thanks. I merged after a few cleanups.

[moyodiallo]

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