-
Notifications
You must be signed in to change notification settings - Fork 413
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
fix(ml-sources): check that a library is available before tracking it #10355
fix(ml-sources): check that a library is available before tracking it #10355
Conversation
bed7faa
to
c660244
Compare
This diff is way better without whitespace https://github.com/ocaml/dune/pull/10355/files?w=1 |
One issue with this PR is that it's introducing a library database look up before we're able to resolve the sources of a library. That now has the potential for cycles. E.g:
It would be better to delay this availability check until the source is needed by somebody |
This cycle is present even if I revert my diff. Are you suggesting that we're adding to it rather than causing it? |
c660244
to
cea6fcc
Compare
I changed the implementation to delay querying |
55d7708
to
0a42bd7
Compare
@rgrinberg I think this is still waiting for another look. |
I'll try and take a look again this week |
c1766de
to
027131a
Compare
Since ocaml#10307, dune started allowing libraries to share the same name. Thus, checking `enabled_if` isn't enough for libraries; dune now needs to check whether the library is available before tracking it in ml_sources Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
027131a
to
e1f08ed
Compare
Since #10307, dune started allowing libraries to share the same name.
Thus, checking
enabled_if
isn't enough for libraries; dune now needs to check whether the library is available before tracking it in ml_sources.addresses #10307 (comment)