@Lupus this PR should properly detect the error that you've encountered with virtual libraries. Testing if it works for you would be quite useful for me to know.

@diml I've added a couple of commits to check for invalid implementations. Those occur when an implementation depends on a library that depends on the virtual library being implemented. This is essentially a cycle as there's only one archive when linking in the end.

I've just pinned dune to this PR as follows:

$ opam pin dune https://github.com/ocaml/dune.git#55cf5c641746a9c7cf9414a3830ed4f93207c5e2

Cloned this repo: https://github.com/Lupus/virtual-lib-issue

Created fresh switch with 4.08 OCaml, vanilla dune 2.4 gives the following error when I try to build it:

$ dune build
File "_none_", line 1: 
Error: No implementations provided for the following modules:
         A__Real_module referenced from lib/b/b.cmxa(B__Foo)

And with the pinning in place I see no change in the error message. Is my understanding correct that I should see some error about dependency cycle?

@Lupus I'd like to reproduce the bug using your example, but I don't have reason installed. Could you port your example to OCaml perhaps?

Installing reason with opam seems to be pretty cheap on effort and storage space, but here you are: https://github.com/Lupus/virtual-lib-issue/tree/ocaml

It's not about the disk space, it's just that I find the reason syntax hard to read :)

I tried your example, and I can observe the following error message:

Error: Implementation the-lib-unix.a has a dependency that uses the virtual
library the-lib.a
   - the-lib-unix.a at _build/default/lib-unix/a
-> - the-lib at _build/default/lib
-> required by executable Example in examples/dune:2

This is what is expected.

Come one, it had only 13 lines of code! :)

Okay, the error message that you show looks much better. Shouldn't pin of dune be sufficient to reproduce this? And anyways, I trust your check of new branch on the repro example, it's a minified version of real codebase that exhibited this error, so I'm pretty confident the fix should improve the error message for that case as well.

Okay, the error message that you show looks much better. Shouldn't pin of dune be sufficient to reproduce this?

That's how I produced the error message above. Which command did you use to pin?

I haven't read the whole PR, but I'm a bit worried by the complexity of this check. It seems to me that we are scanning a lot of libraries in a loop, which could have non-negligible performance implications.

Could you remind me what case we are trying to detect again?

I haven't read the whole PR, but I'm a bit worried by the complexity of this check. It seems to me that we are scanning a lot of libraries in a loop, which could have non-negligible performance implications.

I agree that the check is expensive. One of the reasons it took me so long to add this check is that I was looking for ways to optimize it or to avoid it in some cases. All of those previous approaches had problems and this is the first implementation of the check where I'm fairly confident that it works correctly and detects all the edge cases. I'd like to hear suggestions on how to optimize it, but even if the check ends up being expensive, I'd say that:

• It's still better than having no check and having these invalid archives
• Users only pay for this check if they are using virtual libraries

Could you remind me what case we are trying to detect again?

The test this PR fixes is a good example. Here's a quick example:

;; we have a virtual library
(library (name vlib) (virtual_modules vlib))
;; and a library that depends on it:
(library (name lib) (libraries vlib))
;; now we create an implementation
(library (name impl) (implements vlib) (libraries lib))
;; but the implementation above is invalid. impl cannot depend on any libraries
;; that have lib in vlib in their closure

Does this PR reports the error when we define "impl" or when we try to link an exe against "impl"?

This PR no longer includes any refactoring. The only change included is the bug fix.

However, it was decided that this check should be linear in the size of the closure rather than quadratic (in the worst case) as it is now. I will attempt to this optimization for 2.6

Fixed in main.